Skip to content

Update exception handling support to current proposal #1596

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

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

takikawa
Copy link
Contributor

This PR updates the support of exception handling to the latest proposal (that is compatible with future 2-phase exception handling) described in WebAssembly/exception-handling#137 and WebAssembly/exception-handling#143.

  • Adds back tagged catch $e, catch_all, and rethrow N from a previous version of wabt, but with updates to match the current spec (e.g., catch_all shares an opcode with else, rethrow's depth indexes only catch blocks, etc).
  • Adds unwind and delegate instructions.
  • Removes exnref and br_on_exn.
  • Updates relevant tests.

There are some details that could still change (e.g., maybe how delegate's depth is validated), but I'd be happy to submit further PRs if the spec details change.

Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for doing this! Sorry for the long delay on review

  * Restores code for catch, catch_all, and rethrow from a previous
    version of the proposal, modified to match the latest spec.
  * Adds additional tests and modify existing ones.
  * Remove exnref/br_on_exn tests that will start failing.
@dschuff
Copy link
Member

dschuff commented Feb 8, 2021

I just noticed (after the discussion in #1604, thanks for pointing it out) that this PR doesn't actually solve the problem that made me notice the opcode problem, (because I was only using the disassembler). Namely wasm-objdump prints opcode 0x5 as else rather than catch-all even in the context of try. Presumably it's just going directly from opcode.def, (which doesn't have catch_all in this PR). But maybe let's not fix this until we decide whether we want to keep the opcodes as they are or change them.

@aheejin
Copy link
Member

aheejin commented Feb 9, 2021

Thanks @takikawa! By the way can I merge this? If the opcode is the issue we can fix it later.

@takikawa
Copy link
Contributor Author

takikawa commented Feb 9, 2021

@aheejin Sure, no issues on my end with having it merged. AFAIK I've addressed all the comments in review. Thanks!

I'd also be happy to submit a PR afterwards if the opcode changes.

@aheejin aheejin merged commit 036a632 into WebAssembly:master Feb 9, 2021
@takikawa takikawa deleted the new-eh-proposal branch February 9, 2021 20:18
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.

4 participants