[EH] Use LLVM implementation for new Wasm EH #23469
Conversation
This adds a new `WASM` mode in `ExceptionLibrary`, which uses the new standard Wasm (exnref) EH, adds a library variant for it, and make the tests use the new LLVM implementation instead of the Binaryen translator. The CI won't pass until llvm/llvm-project#123915 lands.
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (1) test expectation files were updated by running the tests with `--rebaseline`: ``` other/codesize/test_codesize_cxx_except_wasm.size: 144708 => 144587 [-121 bytes / -0.08%] Average change: -0.08% (-0.08% - -0.08%) ```
| @@ -431,10 +431,6 @@ def check_human_readable_list(items): | |||
| extras = settings.BINARYEN_EXTRA_PASSES.split(',') | |||
| passes += [('--' + p) if p[0] != '-' else p for p in extras if p] | |||
|
|
|||
| # Run the translator to the standardized EH instructions. | |||
| if not settings.WASM_LEGACY_EXCEPTIONS: | |||
There was a problem hiding this comment.
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.
Not sure what would be a good option for enabling this... Adding one more possible value to WASM_LEGACY_EXCEPTIONS (which sounds weird because LEGACY_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.
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.
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.
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.
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.
(even if not, if we decide to do it, a followup is fine)
There was a problem hiding this comment.
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?
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.
| @@ -431,10 +431,6 @@ def check_human_readable_list(items): | |||
| extras = settings.BINARYEN_EXTRA_PASSES.split(',') | |||
| passes += [('--' + p) if p[0] != '-' else p for p in extras if p] | |||
|
|
|||
| # Run the translator to the standardized EH instructions. | |||
| if not settings.WASM_LEGACY_EXCEPTIONS: | |||
There was a problem hiding this comment.
(even if not, if we decide to do it, a followup is fine)
This adds a new
WASMmode inExceptionLibrary, which uses the new standard Wasm (exnref) EH, adds a library variant for it, and make the tests use the new LLVM implementation instead of the Binaryen translator.The CI won't pass until llvm/llvm-project#123915 lands.