Don't run binaryen postprocessing for Emscripten EH/SjLj #12399
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now that we are renaming invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, using Emscripten EH or SjLj does not need Binaryen's postprocessing. This makes Emscripten not enable Binaryen postprocessing using wasm-emscripten-finalize when we are using them, and deletes all special handling for `emscripten_longjmp_jmpbuf`, given that Emscripten will not see it. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081
|
The CI is not gonna pass until we land these companion LLVM and Binaryen patches: |
kripken
approved these changes
Oct 1, 2020
| @@ -123,7 +123,7 @@ | |||
| "setjmp": ["setThrew", "realloc", "testSetjmp", "saveSetjmp"], | |||
| "longjmp": ["setThrew", "realloc", "testSetjmp", "saveSetjmp"], | |||
| "siglongjmp": ["setThrew", "realloc", "testSetjmp", "saveSetjmp"], | |||
| "emscripten_longjmp_jmpbuf": ["setThrew", "realloc", "testSetjmp", "saveSetjmp"], | |||
| "emscripten_longjmp": ["setThrew", "realloc", "testSetjmp", "saveSetjmp"], | |||
There was a problem hiding this comment.
to be sure I understand, now the LLVM backend will never emit emscripten_longjmp_jmpbuf any more, but it does emit emscripten_longjmp?
tests/test_other.py
Outdated
| fail([], legalization_message) | ||
| fail(['-sWASM_BIGINT'], longjmp_message) | ||
| fail(['-sSUPPORT_LONGJMP=0'], legalization_message) |
There was a problem hiding this comment.
i think this line is not needed any more
aheejin
added a commit
to aheejin/emscripten
that referenced
this pull request
Oct 7, 2020
This was going to be a part of emscripten-core#12399, but commiting this single line first will enable rolling of the corresponding LLVM (https://reviews.llvm.org/D88697) and Binaryen (WebAssembly/binaryen#3191) patches successfully without us needing to disable any Emscripten tests or do a double rolling.
aheejin
added a commit
that referenced
this pull request
Oct 7, 2020
This was going to be a part of #12399, but commiting this single line first will enable rolling of the corresponding LLVM (https://reviews.llvm.org/D88697) and Binaryen (WebAssembly/binaryen#3191) patches successfully without us needing to disable any Emscripten tests or do a special rolling.
aheejin
added a commit
to llvm/llvm-project
that referenced
this pull request
Oct 7, 2020
Renaming for some Emscripten EH functions has so far been done in wasm-emscripten-finalize tool in Binaryen. But recently we decided to make a compilation/linking path that does not rely on wasm-emscripten-finalize for modifications, so here we move that functionality to LLVM. Invoke wrappers are generated in LowerEmscriptenEHSjLj pass, but final wasm types are not available in the IR pass, we need to rename them at the end of the pipeline. This patch also removes uses of `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass, replacing that with `emscripten_longjmp`. `emscripten_longjmp_jmpbuf` is lowered to `emscripten_longjmp`, but previously we generated calls to `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass because it takes `jmp_buf*` instead of `i32`. But we were able use `ptrtoint` to make it use `emscripten_longjmp` directly here. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081 Companions: WebAssembly/binaryen#3191 emscripten-core/emscripten#12399 Reviewed By: dschuff, tlively, sbc100 Differential Revision: https://reviews.llvm.org/D88697
aheejin
added a commit
to WebAssembly/binaryen
that referenced
this pull request
Oct 11, 2020
Now that we are renaming invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, this deletes all related renaming routines and relevant tests. Depends on #3192. Addresses: #3043 and #3081 Companions: https://reviews.llvm.org/D88697 emscripten-core/emscripten#12399
|
Awesome! |
aheejin
added a commit
to aheejin/emscripten
that referenced
this pull request
Oct 11, 2020
We used to require Binaryen postprocessing for EH and longjmp and have `fail` requirements for those features in `other.test_immutable_after_link`. I deleted those requirements in emscripten-core#12399, but it could have been better if I didn't just delete them but changed them to `ok` requirements. This PR adds them.
aheejin
added a commit
that referenced
this pull request
Oct 11, 2020
We used to require Binaryen postprocessing for EH and have a `fail` requirement for it in `other.test_immutable_after_link`. I deleted the requirement in #12399, but it could have been better if I didn't just delete it but changed it to an `ok` requirement. This PR adds that.
arichardson
pushed a commit
to arichardson/llvm-project
that referenced
this pull request
Mar 24, 2021
Renaming for some Emscripten EH functions has so far been done in wasm-emscripten-finalize tool in Binaryen. But recently we decided to make a compilation/linking path that does not rely on wasm-emscripten-finalize for modifications, so here we move that functionality to LLVM. Invoke wrappers are generated in LowerEmscriptenEHSjLj pass, but final wasm types are not available in the IR pass, we need to rename them at the end of the pipeline. This patch also removes uses of `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass, replacing that with `emscripten_longjmp`. `emscripten_longjmp_jmpbuf` is lowered to `emscripten_longjmp`, but previously we generated calls to `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass because it takes `jmp_buf*` instead of `i32`. But we were able use `ptrtoint` to make it use `emscripten_longjmp` directly here. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081 Companions: WebAssembly/binaryen#3191 emscripten-core/emscripten#12399 Reviewed By: dschuff, tlively, sbc100 Differential Revision: https://reviews.llvm.org/D88697
mem-frob
pushed a commit
to draperlaboratory/hope-llvm-project
that referenced
this pull request
Oct 7, 2022
Renaming for some Emscripten EH functions has so far been done in wasm-emscripten-finalize tool in Binaryen. But recently we decided to make a compilation/linking path that does not rely on wasm-emscripten-finalize for modifications, so here we move that functionality to LLVM. Invoke wrappers are generated in LowerEmscriptenEHSjLj pass, but final wasm types are not available in the IR pass, we need to rename them at the end of the pipeline. This patch also removes uses of `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass, replacing that with `emscripten_longjmp`. `emscripten_longjmp_jmpbuf` is lowered to `emscripten_longjmp`, but previously we generated calls to `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass because it takes `jmp_buf*` instead of `i32`. But we were able use `ptrtoint` to make it use `emscripten_longjmp` directly here. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081 Companions: WebAssembly/binaryen#3191 emscripten-core/emscripten#12399 Reviewed By: dschuff, tlively, sbc100 Differential Revision: https://reviews.llvm.org/D88697
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Now that we are renaming invoke wrappers and
emscripten_longjmp_jmpbufin the wasm backend, using Emscripten EH or SjLj does not need
Binaryen's postprocessing. This makes Emscripten not enable Binaryen
postprocessing using wasm-emscripten-finalize when we are using them,
and deletes all special handling for
emscripten_longjmp_jmpbuf, giventhat Emscripten will not see it.
Addresses:
WebAssembly/binaryen#3043
WebAssembly/binaryen#3081
Companions:
https://reviews.llvm.org/D88697
WebAssembly/binaryen#3191