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.

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] Add new test setting for new Wasm EH #21906

Merged
merged 7 commits into from
May 8, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented May 7, 2024

with_all_eh_sjlj and with_all_sjlj decorators (previously with_both_eh_sjlj and with_both_sjlj but recently got renamed) currently tests two modes: Emscripten EH and Wasm EH (pre-2023). We decided to switch to a new version of the Wasm EH proposal in Oct 2023, so this adds a new test setting for the new Wasm EH proposal in those decorators.

Currently we have two parameters:

  • ``: Emscripten EH
  • wasm: Wasm EH (pre-2023)

This changes it to these three parameters:

  • emscripten: Emscripten EH
  • wasm: Wasm EH (pre-2023)
  • wasm_exnref: Wasm EH (New proposal adopted on Oct 2023)

To use the new mode in the command line, you use -fwasm-exceptions as in the old Wasm EH, but add -sWASM_EXNREF additionally.

This currently uses the Binaryen translator that translates old Wasm EH instruction to the new ones
(https://github.com/WebAssembly/binaryen/blob/main/src/passes/TranslateEH.cpp) at the end of the Binaryen pipeline to produce new binaries.

`with_all_eh_sjlj` and `with_all_sjlj` decorators (previously
`with_both_eh_sjlj` and `with_both_sjlj` but recently got renamed)
currently tests two modes: Emscripten EH and Wasm EH (pre-2023). We
decided to switch to a new version of the Wasm EH proposal in Oct 2023,
so this adds a new test setting for the new Wasm EH proposal in those
decorators.

Currently we have two parameters:
- ``: Emscripten EH
- `wasm`: Wasm EH (pre-2023)

This changes it to these three parameters:
- `emscripten`: Emscripten EH
- `wasm`: Wasm EH (pre-2023)
- `new_wasm`: Wasm EH (New proposal adopted on Oct 2023)

To use the new mode in the command line, you use `-fwasm-exceptions` as
in the old Wasm EH, but add `-sEXPERIMENTAL_NEW_WASM_EXCEPTIONS`
additionally.

This currently uses the Binaryen translator that translates old Wasm EH
instruction to the new ones
(https://github.com/WebAssembly/binaryen/blob/main/src/passes/TranslateEH.cpp)
at the end of the Binaryen pipeline to produce new binaries.
@aheejin aheejin requested review from kripken, sbc100 and dschuff May 7, 2024 21:05
@aheejin
Copy link
Member Author

aheejin commented May 7, 2024

The CI will pass once llvm/llvm-project#91299 rolls into Chromium CI.

@dschuff
Copy link
Member

dschuff commented May 7, 2024

Sorry to bikeshed the naming, but I think a name that describes the feature itself is better than just describing it in relation to the other feature (e.g. wasm_exnref instead of new_wasm)

@dschuff
Copy link
Member

dschuff commented May 7, 2024

otherwise LGTM though.

test/common.py Outdated
# - Emscripten EH + Emscripten SjLj
# - Wasm EH + Wasm SjLj
# - Wasm EH + Wasm SjLj (Phase 3, to be deprecated)
# - Wasm EH _ Wasm SjLj (New, experimental)
Copy link
Member

Choose a reason for hiding this comment

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

What does _ mean here? (after "EH ")

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to type +... Will fix.

@aheejin
Copy link
Member Author

aheejin commented May 7, 2024

Sorry to bikeshed the naming, but I think a name that describes the feature itself is better than just describing it in relation to the other feature (e.g. wasm_exnref instead of new_wasm)

My thought was mostly that this new mode will be merged into wasm anyway so I didn't think too much about the name. But (whether they will be merged or not later) being specific in the name sounds good to me too. Do you think it is better to change the option name to -sWASM_EXNREF or something than the verbose -sNEW_EXPERIMENAL_WASM_EXCEPTIONS?

@dschuff
Copy link
Member

dschuff commented May 7, 2024

Sorry to bikeshed the naming, but I think a name that describes the feature itself is better than just describing it in relation to the other feature (e.g. wasm_exnref instead of new_wasm)

My thought was mostly that this new mode will be merged into wasm anyway so I didn't think too much about the name. But (whether they will be merged or not later) being specific in the name sounds good to me too. Do you think it is better to change the option name to -sWASM_EXNREF or something than the verbose -sNEW_EXPERIMENAL_WASM_EXCEPTIONS?

True; although the timeline for how long this option will have to exist is pretty unclear. Most users aren't going to want to use exnref until it has been supported in all browsers for quite a while, so even if exnref went to phase 4 tomorrow, I expect it to be at least a year until we would think about making exnref the default behavior for fwasm-exceptions. But in the meantime there will be 2 options that coexist.
So... yeah, I think it probably does make sense to have a better name.

@aheejin
Copy link
Member Author

aheejin commented May 8, 2024

Changed the option name to -sWASM_EXNREF and the test parameter postfix to _wasm_exnref.

@aheejin aheejin merged commit 619f08d into emscripten-core:main May 8, 2024
29 checks passed
@aheejin aheejin deleted the new_eh_setting branch May 8, 2024 23:15
aheejin added a commit to aheejin/emscripten that referenced this pull request May 8, 2024
The testing mode for the new Wasm EH (exnref) was added in
`with_all_eh_sjlj` and `with_all_sjlj` in emscripten-core#21906. But this adds the new
Wasm EH test mode for more tests that are not covered by those
decorators.
aheejin added a commit that referenced this pull request May 10, 2024
The testing mode for the new Wasm EH (exnref) was added in
`with_all_eh_sjlj` and `with_all_sjlj` in #21906. But this adds the new
Wasm EH test mode for more tests that are not covered by those
decorators.
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.

3 participants