Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[EH] Use LLVM implementation for new Wasm EH #23469
[EH] Use LLVM implementation for new Wasm EH #23469
Changes from all commits
02891fb
2dcb7fa
d165f45
f90efe5
05b1c02
6d05b70
1ded2da
d05ec5d
2f47aff
76c7cd8
842ee68
bfeb718
21c2adf
04faed4
91204e5
06f9a9a
42ffb38
0d3119c
545402c
aa4a56d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be worthwhile to keep some way to activate the translator path for a while. Just in case, e.g. someone finds a bug, they can try the other pathway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what would be a good option for enabling this... Adding one more possible value to
WASM_LEGACY_EXCEPTIONS
(which sounds weird becauseLEGACY_EXCEPTIONS
option sounds like it should be on or off and not about what kind of non-leagcy exception we choose) or adding another option?Also I don't have any idea how widely this feature has been adopted in the first place. I haven't received a single bug reports about this feature over the last year... So I guess this is at least not widely used, if ever used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm sure it's the case that nobody has been using it. I think Firefox is the only runtime that supports it so far, so nobody would really have a good reason to, when everything supports legacy exceptions.
As for options, I don't think it matters much. I'm imagining this would basically be an undocumented debugging option that we'd eventually get rid of once we're confident that LLVM and Binaryen's exnref support both work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's only useful for us Emscripten developers and not much for users. Having an internal option for testing for us sounds fine, but I'm a little hesitant because we already have a messy soup of options for EH/SjLj.. 🥲
Anyway can that be a followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, come to think of it, I don't think it will be very useful after all... We, or more likely I, will be the only one who will run that option (for debugging, in case llvm implementation has bugs or something), and I still can do that with
wasm-opt --translate-to-exnref
on the legacy Wasm EH result.. It's only one command and it's gonna be needed only when debugging, so I don't feel that's too inconvenient. And I'd like to avoid adding one more option to the soup of EH/SjLj options...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a good point that if it's a one-command translation that would be easy enough. Is the wasm/JS interface 100% compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(even if not, if we decide to do it, a followup is fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't see why not. I checked with a simple test case, replacing only the wasm with the translated binary, and it seems to work.