New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ocamldebug] Catch out_of_range in "list" command #977

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@yunxing
Contributor

yunxing commented Dec 16, 2016

If you type list at a step where the source info is not available, ocamldebug could crash
due to the uncaught exception. This patch catches the exception and returns an user friendly
error message instead.

[ocamldebug] Catch out_of_range in "list" command
If you type list at a step where the source info is not available, ocamldebug could crash
due to the uncaught exception. This patch catches the exception and return an user friendy
error message instead.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 17, 2016

Member

I looked at the code and the patch looks correct to me -- but then I know little about the debugger internals. I was initially worried as several places catch Out_of_range to display error messages and I wondered whether this change would affect them, but it looks like each instr_* command devises its own exception-catching strategy (Out_of_range and Not_found), and other handlers are in other commands than instr_list, so it seems fine.

Could you include a Changelog entry? It should go in a new ### Debugger: subsection of the 4.05.0 changes, for example just before the existing ### Tools: section.

Member

gasche commented Dec 17, 2016

I looked at the code and the patch looks correct to me -- but then I know little about the debugger internals. I was initially worried as several places catch Out_of_range to display error messages and I wondered whether this change would affect them, but it looks like each instr_* command devises its own exception-catching strategy (Out_of_range and Not_found), and other handlers are in other commands than instr_list, so it seems fine.

Could you include a Changelog entry? It should go in a new ### Debugger: subsection of the 4.05.0 changes, for example just before the existing ### Tools: section.

@gasche gasche added the approved label Dec 17, 2016

@yunxing

This comment has been minimized.

Show comment
Hide comment
@yunxing

yunxing Dec 17, 2016

Contributor

@gasche Thanks for looking into this. Your observation is fine, this is basically a standalone codeflow where the exception is not being caught by anyone -- which crashes the entire debugger.

Contributor

yunxing commented Dec 17, 2016

@gasche Thanks for looking into this. Your observation is fine, this is basically a standalone codeflow where the exception is not being caught by anyone -- which crashes the entire debugger.

Show outdated Hide outdated Changes
@@ -102,6 +102,9 @@ Next version (4.05.0):
it previously raised an EPIPE error, it now returns 0 like other OSes
(Jonathan Protzenko)
### Debugger:
- PR#977: Catch Out_of_range in "list" command (Yunxing)

This comment has been minimized.

@gasche

gasche Dec 17, 2016

Member

This should be GPR, not PR (which refers to mantis Problem Reports).

@gasche

gasche Dec 17, 2016

Member

This should be GPR, not PR (which refers to mantis Problem Reports).

@yunxing

This comment has been minimized.

Show comment
Hide comment
@yunxing

yunxing Dec 21, 2016

Contributor

@gasche Thanks, updated the PR.

Contributor

yunxing commented Dec 21, 2016

@gasche Thanks, updated the PR.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 23, 2016

Member

I merged in trunk, thanks!
(I manually squashed the commits together and removed the whitespace noise.)

Member

gasche commented Dec 23, 2016

I merged in trunk, thanks!
(I manually squashed the commits together and removed the whitespace noise.)

@gasche gasche closed this Dec 23, 2016

@yunxing

This comment has been minimized.

Show comment
Hide comment
@yunxing

yunxing Dec 24, 2016

Contributor

@gasche Thanks!

Contributor

yunxing commented Dec 24, 2016

@gasche Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment