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.

Sign up for GitHub

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

Jump to bottom

Start to add support for new version of exception handling proposal #152

Merged
merged 1 commit into from
Dec 9, 2020
Merged

Start to add support for new version of exception handling proposal #152

merged 1 commit into from
Dec 9, 2020

Conversation

takikawa
Copy link
Contributor

@takikawa takikawa commented Dec 1, 2020

Recently the Wasm CG agreed on changing the exception proposal to a different design (due to wanting to accommodate 2-phase exception handling in the future) that doesn't use exnref and has tag arguments for catch instructions. There are some slides explaining this in this spec issue and a new draft spec explainer.

This pull request starts to add support for this in parsing and in validation. Specifically, it adds catch_all and unwind, removes br_on_exn, and revises catch (new tag argument) and rethrow (new index argument) to match the new semantics.

I'm submitting this PR as I'm also working on adding exceptions support in SpiderMonkey and updated parsing support from the wast crate will be helpful for making the SM tests work.

This doesn't yet add the new proposed delegate instruction (because no opcode has been proposed yet), and also doesn't remove exnref yet (can be done later in a separate patch).

Since other tools like wabt haven't been updated for the new semantics yet, this PR also disables some of those tests (but adds local ones).

Also the new spec isn't final too, so maybe some instructions will keep changing. Happy to follow up with further PRs to keep current.

@takikawa takikawa mentioned this pull request Dec 1, 2020
Closed
@yurydelendik
Copy link
Contributor

Is it possible to update tests (instead of disabling them)? Do you know what is a timeline to change LLVM and other tooling to match a new spec?

@alexcrichton
Copy link
Member

I think there's some CI failures as well, but otherwise looks great, thanks!

@takikawa takikawa force-pushed the new-eh-semantics-with-validation branch from af3e961 to a608502 Compare December 1, 2020 19:01
@takikawa
Copy link
Contributor Author

takikawa commented Dec 1, 2020

I believe I've fixed the CI issues with a push earlier today, but it appears that the Github CI infra is having some trouble right now so it might take a bit for this to show up.

@yurydelendik Since the tests are in wabt, I think to update them I'd have to go update the wabt implementation as well (which would be nice to do but wasn't on my critical path at the moment). Re: other tooling, I'm not sure. So far I don't think wabt, binaryen, or LLVM have switched over to the new proposal yet.

@takikawa takikawa mentioned this pull request Dec 2, 2020
Merged
@takikawa takikawa force-pushed the new-eh-semantics-with-validation branch from a608502 to 9e81c84 Compare December 2, 2020 19:31
@takikawa
Copy link
Contributor Author

takikawa commented Dec 2, 2020

I've rebased and pushed again so it merges cleanly on top of #154, and CI should be good too.

alexcrichton
alexcrichton reviewed Dec 2, 2020
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks! Just one small stylistic nit

crates/wasmparser/src/operators_validator.rs Outdated Show resolved Hide resolved
@takikawa
Add support for new exception handling proposal
f810d09 
The exception handling proposal switched its design from
an exnref-based approach with first-class exception reference
values to a version with tagged catch blocks and implicit
exception references.

This commit adds support for this new semantics and revises
tests to match.

It does not yet remove exnref, as that can be done in a separate
patch.

Note: it does not add the `delegate` instruction in the new
proposal yet, as it does not yet have an opcode assigned.
@takikawa takikawa force-pushed the new-eh-semantics-with-validation branch from 9e81c84 to f810d09 Compare December 2, 2020 20:14
alexcrichton
alexcrichton approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

@yurydelendik do you want to review as well?

@takikawa
Copy link
Contributor Author

takikawa commented Dec 8, 2020

Hi just wanted to give a friendly ping to see if there's anything else I should address on this.

BTW, should I also add a commit to bump crate versions as this adds new AST types?

yurydelendik
yurydelendik approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

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

Looks good on wasmparser side too. Thank you for the patch.

@alexcrichton
Copy link
Member

Oops sorry didn't see @yurydelendik's approval from earlier!

@alexcrichton alexcrichton merged commit 4bb7869 into bytecodealliance:main Dec 9, 2020
@takikawa takikawa deleted the new-eh-semantics-with-validation branch December 9, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

@yurydelendik yurydelendik yurydelendik 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.

None yet
3 participants
@takikawa @yurydelendik @alexcrichton