Skip to content
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

Use raise_notrace rather than raise in format.ml #1731

Merged
merged 2 commits into from Apr 20, 2018

Conversation

Projects
None yet
6 participants
@let-def
Copy link
Contributor

commented Apr 19, 2018

Calls to raise in Format can accidentally appear in backtraces, especially when formatting an error message after catching an exception.

By using raise_notrace, the backtrace is no longer clobbered and the debugger is happy.

Note: I had some failure running the testsuite (on macOS), but these didn't look related to the change, I will investigate more.

Use raise_notrace rather than raise in format.ml
Calls to raise in Format can accidentally appear in backtraces,
especially when formatting an error message after catching an exception.

By using raise_notrace, the backtrace is no longer clobbered.
@let-def

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

I just had some stalled artifacts, testsuite is happy now:

Summary:
  1657 tests passed
   85 tests skipped
    0 tests failed
    0 unexpected errors
  1742 tests considered
@gasche

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

We should have remembered to do this a long time ago! Are there other uses of exceptions for local control in the library that should receive the same treatment?

@gasche

gasche approved these changes Apr 19, 2018

Copy link
Member

left a comment

Approved; to merge if the CI passes.

(I don't think we need a review tag for this one, the change is trivial.)

@jberdine

This comment has been minimized.

Copy link

commented Apr 19, 2018

This change is great! I still wonder if there would be value in a wider solution, where exceptions raised and caught in a handler do not perturb the backtrace. Consider uses cases such as performing a symbol table lookup in a handler in order to produce a readable log message, or something similar. It's easy for such code to (invisible to the programmer) raise and handle exceptions.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

This change is great! I still wonder if there would be value in a wider solution, where exceptions raised and caught in a handler do not perturb the backtrace. Consider uses cases such as performing a symbol table lookup in a handler in order to produce a readable log message, or something similar. It's easy for such code to (invisible to the programmer) raise and handle exceptions.

Something along these lines (not exactly the same) was proposed in #260, but (my understanding is that) there was no consensus on the implementation and was eventually dropped.

@jberdine

This comment has been minimized.

Copy link

commented Apr 19, 2018

Yes, I was sorry to see local exceptions as jumps shot down, I still want them.

They would reduce the number of cases where an exception raise happens, and thereby reduce the opportunities for clobbering the backtrace. But I think that a general protection mechanism would still be valuable. Something like Infer's reraise_after but applied by the compiler so that every "passthrough" exception handler didn't need to be written in terms of some other function.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

@jberdine We expect that Flambda 2.0 (forthcoming at some point in the future) will turn local "raise_notrace" exceptions into jumps. There will still be the overhead of setting up and removing the trap frame on the stack in the initial instance---removing those operations requires substantially more analysis. However those costs should be minor.

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

Regarding "protecting backtrackes", there is also a solution on the other end, which is to protect your exception handler by saving the backtrace first thing, and then re-raising it with Printexc.raise_with_backtrace (available since 4.05).

@let-def let-def force-pushed the let-def:format_notrace branch from 10de1c4 to 59a81c7 Apr 19, 2018

@jberdine

This comment has been minimized.

Copy link

commented Apr 19, 2018

@mshinwell Good to know, thanks! I'm guessing that the overhead of setting up the trap frame will still be lower than CPS-transforming.

@gasche Yes, Infer's reraise_after is doing precisely that, just packaging the backtrace management boilerplate into a function. My point is that it is error-prone because it is easy to forget to protect any individual handler (there is nothing in the type system to help, AFAIK), and a bit tedious. The underlying question is whether we always want the protected behavior? There may be performance implications, but otherwise I'm not aware of use cases where the backtraces of exceptions raised and handled within a handler are preferred to the backtrace of the exception that caused the handler to be entered in the first place.

@let-def

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2018

@gasche I am not against looking at other modules for other potential uses of raise_notrace, however I would prefer if that goes into another PR and merge this one asap.

It is not always clear which exceptions are supposed to escape and which are not and I am not familiar with all modules, I am afraid that inappropriate use of raise_notrace would lead to even more confusion.

@jberdine

This comment has been minimized.

Copy link

commented Apr 20, 2018

(I'm sorry for derailing discussion on this PR, I did not intend to delay it.)

@trefis trefis merged commit 7b6d228 into ocaml:trunk Apr 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

I agree with @let-def's decision of doing other similar changes in dedicated PRs.
Considering all the tests passed, and that the PR was approved, I merged.

keleshev added a commit to keleshev/ocaml that referenced this pull request Aug 5, 2018

Format: Avoid using exceptions for control flow
Before the following commit:

    985dd82 Format: replace bespoke queue for Stdlib.Queue

The Format module used Empty_queue exception for control follow. This
presented a problem for formatting error messages after catching an
exception, which was addressed in GPR ocaml#1731 by using `raise_notrace`
instead of `raise`. However, the above commit ported Format to use
`Stdlib.Queue` which uses `raise`. This commit changes the code to avoid
using exceptions for control flow, thus avoiding the problem described
in the GPR.

Function `pp_skip_token` had a comment saying that the queue is never
empty when it is called, and it was tempting to rely on that.
However, I came up with a unit test that falsifies that invariant, see
`pp_skip_token.ml`. This prompted to change `pp_skip_token` to
gracefully handle the case when the queue is empty. Before, the
invariant was wrong, but the code still worked correctly because the
exception would have been caught in `advance_left`.

keleshev added a commit to keleshev/ocaml that referenced this pull request Aug 6, 2018

Format: Avoid using exceptions for control flow
Before the following commit:

    985dd82 Format: replace bespoke queue for Stdlib.Queue

The Format module used Empty_queue exception for control follow. This
presented a problem for formatting error messages after catching an
exception, which was addressed in GPR ocaml#1731 by using `raise_notrace`
instead of `raise`. However, the above commit ported Format to use
`Stdlib.Queue` which uses `raise`. This commit changes the code to avoid
using exceptions for control flow, thus avoiding the problem described
in the GPR.

Function `pp_skip_token` had a comment saying that the queue is never
empty when it is called, and it was tempting to rely on that.
However, I came up with a unit test that falsifies that invariant, see
`pp_skip_token.ml`. This prompted to change `pp_skip_token` to
gracefully handle the case when the queue is empty. Before, the
invariant was wrong, but the code still worked correctly because the
exception would have been caught in `advance_left`.

keleshev added a commit to keleshev/ocaml that referenced this pull request Aug 7, 2018

Format: avoid using exceptions for control flow
Before the following commit:

    985dd82 Format: replace bespoke queue for Stdlib.Queue

The Format module used Empty_queue exception for control follow. This
presented a problem for formatting error messages after catching an
exception, which was addressed in GPR ocaml#1731 by using `raise_notrace`
instead of `raise`. However, the above commit ported Format to use
`Stdlib.Queue` which uses `raise`. This commit changes the code to avoid
using exceptions for control flow, thus avoiding the problem described
in the GPR.

Function `pp_skip_token` had a comment saying that the queue is never
empty when it is called, and it was tempting to rely on that.
However, I came up with a unit test that falsifies that invariant, see
`pp_skip_token.ml`. This prompted to change `pp_skip_token` to
gracefully handle the case when the queue is empty. Before, the
invariant was wrong, but the code still worked correctly because the
exception would have been caught in `advance_left`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.