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

Add overview on traps and JS API #93

Merged
merged 8 commits into from
Jan 10, 2020
Merged

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Dec 30, 2019

This adds overview on traps not being caught by the catch instruction
and its relationship with the JS API.
Closes #1 and closes #89.

This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes WebAssembly#1 and closes WebAssembly#89.

The `catch` instruction catches exceptions generated by the `throw` instruction,
but is not supposed to catch traps. The rationale for this is traps, which
corresponds to hardware failures in real machines, are not generally needed to
Copy link
Member

Choose a reason for hiding this comment

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

it's not always hardware failures; I think it's more generally that the failures are not locally recoverable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed to "The rationale for this is that in general traps are
not locally recoverable and are not needed to be handled in locals scopes like
try-catch.".

proposals/Exceptions.md Outdated Show resolved Hide resolved
The `catch` instruction catches foreign exceptions generated from calls to
function imports as well, including JavaScript exceptions, with a few
exceptions:
1. When an exported function called from a JavaScript frame traps, the
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it happens currently, but could there ever be a case where we want to have a RuntimeError that did not originate as a trap? i.e. Currently this constraint is that RuntimeError is never caught and relies on the existing behavior that traps get converted to RuntimeError when they unwind through JS frames.
Another possible way of stating this constraint is that the runtime implicitly tracks which exceptions originated as traps, and we say that any exception that originated as a trap does not get caught (and we just don't specify anything about the JS type of the exception). Today those ought to be equivalent. If we prefer to explicitly make the constraint refer to the JS type, then we should maybe also say somewhere (not sure exactly where) that the intention of RuntimeError is that it directly corresponds to wasm traps.

Copy link
Member Author

@aheejin aheejin Jan 6, 2020

Choose a reason for hiding this comment

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

From discussion from #89, currently the only case that we get RuntimeError when calling a function is from traps. But I think it makes sense to make the condition not explicitly tied to RuntimeError, because as you said there might be other cases causing RuntimeError in future. And as I said below, these exceptions should not be based on instanceof operator but rather on a predicate that is not observable by JS anyway. But the other question is, does it make sense to catch other kinds of RuntimeError, given that we don't catch similar kinds of errors (stack overflow and out of memory) either?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, reworded the paragraph, PTAL.

Copy link
Member

@backes backes left a comment

Choose a reason for hiding this comment

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

LGTM from an implementation perspective. Some suggestions.

proposals/Exceptions.md Outdated Show resolved Hide resolved
proposals/Exceptions.md Outdated Show resolved Hide resolved
1. In order to be consistent before and after a trap reaches a JavaScript frame,
the `catch` instruction does not catch exceptions generated from traps. This
is currently
[`WebAssembly.RuntimeError`](https://webassembly.github.io/reference-types/js-api/#exceptiondef-runtimeerror)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to be even more explicit here:
These are currently instances of WebAssembly.RuntimeError, but this detail is not used to decide whether to catch the exception or not. Implementations are supposed to specially mark non-catchable exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworded using the phrases you suggested.

aheejin and others added 3 commits January 8, 2020 00:34
@aheejin aheejin mentioned this pull request Jan 8, 2020
@aheejin aheejin merged commit 0297b94 into WebAssembly:master Jan 10, 2020
@aheejin aheejin deleted the traps_js_api branch January 10, 2020 08:46
pull bot pushed a commit to p-g-krish/v8 that referenced this pull request Mar 23, 2020
The spec was changed such that traps are not catchable in wasm:
WebAssembly/exception-handling#93

This CL implements this in V8 by adding a private symbol as a property
to all uncatchable exceptions. It also adds a number of tests.

[email protected]
[email protected]

Bug: v8:10194
Change-Id: I498531762e8876f809d3b8aeb72ccc053e0e3cd4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2113375
Commit-Queue: Clemens Backes <[email protected]>
Reviewed-by: Jakob Kummerow <[email protected]>
Cr-Commit-Position: refs/heads/master@{#66824}
ioannad pushed a commit to ioannad/exception-handling that referenced this pull request Jun 6, 2020
data.drop must update the store, not the frame. There may be multiple
copies of the frame, so any updates will only update once. We can make
sure that all copies are updated by using an indirection through data
addresses and updating the store instead.

See discussion in PR WebAssembly#92.
ioannad pushed a commit to ioannad/exception-handling that referenced this pull request Jun 6, 2020
The original commit to add a bottom type removed this test as it
would now unexpectedly validate. Now that a bottom type is removed
it would be good to keep it.
ioannad pushed a commit to ioannad/exception-handling that referenced this pull request Jun 6, 2020
This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes WebAssembly#1 and closes WebAssembly#89.
ioannad pushed a commit to ioannad/exception-handling that referenced this pull request Feb 23, 2021
This adds overview on traps not being caught by the `catch` instruction
and its relationship with the JS API.
Closes WebAssembly#1 and closes WebAssembly#89.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API for traps What counts as an exception?
3 participants