Skip to content

Aid debugging the debugger#9057

Merged
dra27 merged 4 commits intoocaml:trunkfrom
dra27:debugging-the-debugger
Apr 17, 2020
Merged

Aid debugging the debugger#9057
dra27 merged 4 commits intoocaml:trunkfrom
dra27:debugging-the-debugger

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Oct 19, 2019

The failure in #9043 consisted of a helpful Uncaught exception: Not_found! This PR updates the debugger to use Printexc.raise_with_backtrace in its handlers and also removes the use of the deprecated Printexc.catch surrounding main.

(not for 4.10)

dra27 added 2 commits October 19, 2019 10:38
@dra27
Preserve backtraces in debugger
e2f278c 
Add Primitives.cleanup which allows handlers for unexpected exceptions
to cleanup and reraise the exception with its backtrace.
@dra27
Remove use of Printexc.catch from debugger
37acb3b 
Allow the runtime to display details of any uncaught exception (with
backtraces, if enabled). Unix.handle_unix_error is still used to convert
errors from system calls to a less unmeaningful form.
gasche
gasche approved these changes Oct 19, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally good (and correctness-preserving) so here is a preliminary seal of approval.

I have two remarks:

  • I think the cleanup would be nicer with cleanup : exn -> (unit -> 'a) -> 'a than your style of of cleanup : exn -> ('a -> 'b) -> 'a -> 'b. Your callsites are less pleasant to read (even in the cases where they are more concise), and the type is more complex than it needs to be.

  • I wonder why you haven't used the new and shiny Fun.protect function. It would be a slightly more invasive patch (you have to factor out the commonalities in the success and failure path), but it would result in nicer code.

I don't know how excited you are about working more on this PR; I think the slightly-nicer API is really easy to do (if you agree it's a good idea; otherwise it is just bothersome), but the second point is more work and it's fine to not do it.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Oct 19, 2019

I'd simply forgotten about Fun.protect, having checked that I wasn't forgetting about a function already in Printexc!

dra27 added 2 commits October 19, 2019 11:47
@dra27
Don't ever call stop_user_input in line_loop
d0bab08 
The caller is now always responsible for calling stop_user_input, rather
than only responsible for calling it on error.
@dra27
Use Fun.protect where possible
9eeead7
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Oct 19, 2019

How's that? There are remaining cases where Fun.protect isn't appropriate as the code is doing special cleanup at that point, so I simplified Primitives.cleanup as requested.

It's now a bit more of a reviewing challenge than it was before! I think I have preserved the same logic - care needed to ensure stop_user_input is called in the right places still. I altered Command_line.line_loop to never call it just as it that then means it's not called twice but also because it does seem strange that the function was responsible for calling stop_user_input on success but the caller was responsible for it on error.

@gasche
Copy link
Copy Markdown
Member

gasche commented Oct 19, 2019

I agree that the previous handler of user input state in line_loop was not nice, but I don't like the new code either. Before the success path had a nice bracketing structure with the same function calling resume_user_input and then stop_user_input. Now this bracketing is lost and the stopping is duplicated in several different places, at (I think) the wrong abstraction level.

I think that a better solution would be to move stop_user_input back inside the line_loop function, ensuring that it is stopped in both the success and failure cases by using Fun.protect again (around the try while true do ... done with Exit -> () block I guess). This means that the loop will run under two nested protect, one setup by the caller and one by the callee, but this is just fine -- protect composes correctly and there are no performance requirements here.

Unrelated note: many of the use of protect in this file correspond to what Misc.protect_refs does better, but I just realized that Misc.protect_refs does not properly protect the backtrace, so it cannot be used here. I'll send a separate PR to change Misc to use protect internally.

@gasche
Copy link
Copy Markdown
Member

gasche commented Oct 19, 2019

I reviewed the other commits: they are fine and correct, but I think it would be nicer to rebase before merging because there is some back-and-forth. We could have two code commits, one for the user-input business (the trickier to review) and one for the rest (local/simple changes).

gasche added a commit to gasche/ocaml that referenced this pull request Oct 20, 2019
@gasche
Misc.protect_refs: use Fun.protect to protect the backtrace
8f7c8bf 
Currently Fun.protect and Misc.try_finally can be used in code that
tries carefully to preserve the first-failure backtrace, but
Misc.protect_refs cannot. This PR fixes the discrepancy. See ocaml#9057 for
a use-case.

(no change entry needed)
@gasche gasche mentioned this pull request Oct 20, 2019
Merged
gasche added a commit to gasche/ocaml that referenced this pull request Oct 20, 2019
@gasche
Misc.protect_refs: use Fun.protect to protect the backtrace
0d31f8f 
Currently Fun.protect and Misc.try_finally can be used in code that
tries carefully to preserve the first-failure backtrace, but
Misc.protect_refs cannot. This PR fixes the discrepancy. See ocaml#9057 for
a use-case.

(no change entry needed)
gasche added a commit to gasche/ocaml that referenced this pull request Oct 30, 2019
@gasche
Misc.protect_refs: use Fun.protect to protect the backtrace
956fcdc 
Currently Fun.protect and Misc.try_finally can be used in code that
tries carefully to preserve the first-failure backtrace, but
Misc.protect_refs cannot. This PR fixes the discrepancy. See ocaml#9057 for
a use-case.

See the GPR ( ocaml#9060 ) for an
in-depth discussion of the potential performance impact of this
change.
gasche added a commit to gasche/ocaml that referenced this pull request Nov 7, 2019
@gasche
Misc.protect_refs: use Fun.protect to protect the backtrace
f5252e9 
Currently Fun.protect and Misc.try_finally can be used in code that
tries carefully to preserve the first-failure backtrace, but
Misc.protect_refs cannot. This PR fixes the discrepancy. See ocaml#9057 for
a use-case.

See the GPR ( ocaml#9060 ) for an
in-depth discussion of the potential performance impact of this
change.
anmolsahoo25 pushed a commit to anmolsahoo25/ocaml that referenced this pull request Dec 9, 2019
@gasche @anmolsahoo25
Misc.protect_refs: use Fun.protect to protect the backtrace
c906db2 
Currently Fun.protect and Misc.try_finally can be used in code that
tries carefully to preserve the first-failure backtrace, but
Misc.protect_refs cannot. This PR fixes the discrepancy. See ocaml#9057 for
a use-case.

See the GPR ( ocaml#9060 ) for an
in-depth discussion of the potential performance impact of this
change.
@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Mar 18, 2020

@dra27 do you want to clean up the history, or can we just squash everything?

You'll have to resolve the conflict in any case.

@dra27 dra27 force-pushed the debugging-the-debugger branch from 13fb0de to 9eeead7 Compare April 17, 2020 06:54
@dra27 dra27 merged commit c294505 into ocaml:trunk Apr 17, 2020
@dra27 dra27 deleted the debugging-the-debugger branch April 17, 2020 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gasche gasche gasche approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dra27 @gasche @damiendoligez