[EH] Use LLVM implementation for new Wasm EH #23469

aheejin · 2025-01-22T10:04:53Z

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 requires adding -wasmexcept and -wasmsjlj variants to MINIMAL_TASKS in embuilder.py
because they are necessary to run coreX tests in CircleCI, because dylink coreX tests in CircleCI rellies on ./embuilder build MINIMAL_PIC --pic, which is the sum of MINIMAL_TASKS and MINIMAL_PIC_TASKS.

Because a new variant is added to ExceptionLibrary, this increases SYSTEM library size from 366M to 400M, roughly by 11%. We discussed about the possibility of not shipping the exnref libraries and let them be built on demand, but given that SYSTEM include all variants, I'm not sure how we are going to do that:

emscripten/embuilder.py

Lines 174 to 177 in 73ebb91

    
           def get_system_tasks(): 
        
             system_libraries = system_libs.Library.get_all_variations() 
        
             system_tasks = list(system_libraries.keys()) 
        
             return system_libraries, system_tasks

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%) ```

site/source/docs/tools_reference/settings_reference.rst

dschuff · 2025-01-22T23:54:41Z

tools/link.py

@@ -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:


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?

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.

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.

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?

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

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?

(even if not, if we decide to do it, a followup is fine)

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.

dschuff · 2025-01-23T20:43:06Z

tools/link.py

@@ -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:


(even if not, if we decide to do it, a followup is fine)

aheejin · 2025-01-24T23:02:27Z

Because a new variant is added to ExceptionLibrary, this increases SYSTEM library size from 366M to 400M, roughly by 11%. We discussed about the possibility of not shipping the exnref libraries and let them be built on demand, but given that SYSTEM include all variants, I'm not sure how we are going to do that:

emscripten/embuilder.py

Lines 174 to 177 in 73ebb91

    
           def get_system_tasks(): 
        
             system_libraries = system_libs.Library.get_all_variations() 
        
             system_tasks = list(system_libraries.keys()) 
        
             return system_libraries, system_tasks

(I updated the PR description too)

These are necessary to run coreX tests in CircleCI, because dylink coreX tests in CircleCI rellies on `./embuilder build MINIMAL_PIC --pic`, which is the sum of `MINIMAL_TASKS` and `MINIMAL_PIC_TASKS`: https://github.com/emscripten-core/emscripten/blob/73ebb91029948e62b3a4cea9ccc8db2dd87162b5/.circleci/config.yml#L186-L190 https://github.com/emscripten-core/emscripten/blob/73ebb91029948e62b3a4cea9ccc8db2dd87162b5/embuilder.py#L90-L124

kripken

Seems ok to me to increase SDK size by a small amount here, given we can't think of a simple way to avoid it. I guess this will make removing old EH modes in the future even nicer...

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: 144652 => 144531 [-121 bytes / -0.08%] Average change: -0.08% (-0.08% - -0.08%) ```

This reverts commit 91204e5.

This reverts commit 21c2adf.

…#23469)" This reverts commit 62304e2.

aheejin requested review from sbc100 and dschuff January 22, 2025 10:04

dschuff reviewed Jan 22, 2025

View reviewed changes

Update ChangeLog

d165f45

dschuff approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'main' into lib_wasm_eh

f90efe5

aheejin requested a review from kripken January 24, 2025 23:02

aheejin added 2 commits January 24, 2025 23:03

Merge branch 'main' into lib_wasm_eh

6d05b70

kripken approved these changes Jan 24, 2025

View reviewed changes

hoodmane mentioned this pull request Jan 27, 2025

LLVM assertion failure on Wasm exceptions rust-lang/rust#135665

Open

aheejin added 14 commits January 27, 2025 19:05

Merge branch 'main' into lib_wasm_eh

1ded2da

Fix typo in ChangeLog

d05ec5d

Merge branch 'main' into lib_wasm_eh

2f47aff

Handle exnref in node_exception_flags

842ee68

Merge branch 'main' into lib_wasm_eh

bfeb718

Adds @requires_wasm_eh to test_embind_tsgen_exceptions

21c2adf

Change version condition from > to >=

04faed4

Add some logging for CI debugging

91204e5

skip test if node < 22

06f9a9a

Use config.NODE_JS instead of self.get_nodejs()

42ffb38

Revert "Add some logging for CI debugging"

0d3119c

This reverts commit 91204e5.

Revert "Adds @requires_wasm_eh to test_embind_tsgen_exceptions"

545402c

This reverts commit 21c2adf.

Merge branch 'main' into lib_wasm_eh

aa4a56d

aheejin merged commit 62304e2 into emscripten-core:main Jan 29, 2025
29 checks passed

aheejin deleted the lib_wasm_eh branch January 29, 2025 04:02

cwoffenden added a commit to cwoffenden/emscripten that referenced this pull request Jan 29, 2025

Revert "[EH] Use LLVM implementation for new Wasm EH (emscripten-core…

f32cd2b

…#23469)" This reverts commit 62304e2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EH] Use LLVM implementation for new Wasm EH #23469

[EH] Use LLVM implementation for new Wasm EH #23469

aheejin commented Jan 22, 2025 •

edited

Loading

dschuff Jan 22, 2025

aheejin Jan 23, 2025

dschuff Jan 23, 2025

aheejin Jan 23, 2025

aheejin Jan 23, 2025 •

edited

Loading

dschuff Jan 23, 2025

dschuff Jan 23, 2025

aheejin Jan 23, 2025

dschuff Jan 23, 2025

aheejin commented Jan 24, 2025

kripken left a comment •

edited

Loading

	def get_system_tasks():
	system_libraries = system_libs.Library.get_all_variations()
	system_tasks = list(system_libraries.keys())
	return system_libraries, system_tasks

[EH] Use LLVM implementation for new Wasm EH #23469

[EH] Use LLVM implementation for new Wasm EH #23469

Conversation

aheejin commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin commented Jan 24, 2025

kripken left a comment • edited Loading

Choose a reason for hiding this comment

aheejin commented Jan 22, 2025 •

edited

Loading

aheejin Jan 23, 2025 •

edited

Loading

kripken left a comment •

edited

Loading