[EH/SjLj] Support Wasm EH + Wasm SjLj #16072
Conversation
Now that llvm/llvm-project@eb675e9 has landed, we can use Wasm EH and Wasm SjLj together. This PR - enables Wasm EH + Wasm SjLj combination which was previously banned - unifies `@with_both_exception_handling` and `@with_both_sjlj_handling` into `@with_both_eh_sjlj`. Given that we don't really have a plan to test Wasm EH + Emscripten SjLj combination going forward, which was meant to be only a temporary measure during the period we have only Wasm EH but not Wasm SjLj, I don't think it's worth the hassle of keeping three different decorators like `@with_both_exception_handling`, `@with_both_sjlj_handling`, `@with_both_eh_sjlj`. - adds `@with_both_eh_sjlj` to several more tests that mix EH + SjLj - adds one more test that mixes EH + SjLj - duplicates `test_longjmp` into `test_longjmp_stadnalone`. I couldn't find a way to attach two decorators (`@with_both_eh_sjlj and `@also_with_standalone_wasm`) to a single test. Fixes emscripten-core#15404.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Should we also test just wasm EH without longjmp, and longjmp without EH, somewhere? Probably not in that decorator that runs all the time, but a separate test might be good. Unless we already have one?
| int main() { | ||
| int jmpval = setjmp(buf); | ||
| if (jmpval != 0) { | ||
| printf("setjmp returned for the second time\n"); |
There was a problem hiding this comment.
Is this reached? I don't see a longjmp in the file.
There was a problem hiding this comment.
No it's not gonna be reached; as well with printf("inner catch: caught %d\n");. These tests check if compilation of mixed setjmp and try-catch work because LLVM backend does heavy transformation on functions that has setjmp calls, so I think it's fine that this line is not reached.
There was a problem hiding this comment.
Ok, maybe add a comment then, that this just checks compilation, and it is never reached?
Added |
| int main() { | ||
| int jmpval = setjmp(buf); | ||
| if (jmpval != 0) { | ||
| printf("setjmp returned for the second time\n"); |
There was a problem hiding this comment.
Ok, maybe add a comment then, that this just checks compilation, and it is never reached?
Now that llvm/llvm-project@eb675e9 has landed, we can use Wasm EH and Wasm SjLj together. This PR - enables Wasm EH + Wasm SjLj combination which was previously banned - unifies `@with_both_exception_handling` and `@with_both_sjlj_handling` into `@with_both_eh_sjlj`. Given that we don't really have a plan to test Wasm EH + Emscripten SjLj combination going forward, which was meant to be only a temporary measure during the period we have only Wasm EH but not Wasm SjLj, I don't think it's worth the hassle of keeping three different decorators like `@with_both_exception_handling`, `@with_both_sjlj_handling`, `@with_both_eh_sjlj`. - adds `@with_both_eh_sjlj` to several more tests that mix EH + SjLj - adds one more test that mixes EH + SjLj - duplicates `test_longjmp` into `test_longjmp_stadnalone`. I couldn't find a way to attach two decorators (`@with_both_eh_sjlj and `@also_with_standalone_wasm`) to a single test. - adds `test_longjmp_with_and_without_exceptions` and `test_exceptions_with_and_without_longjmp`. Fixes #15404.
2 participants
Now that
llvm/llvm-project@eb675e9
has landed, we can use Wasm EH and Wasm SjLj together.
This PR
@with_both_exception_handlingand@with_both_sjlj_handlinginto
@with_both_eh_sjlj. Given that we don't really have aplan to test Wasm EH + Emscripten SjLj combination going forward,
which was meant to be only a temporary measure during the period we
have only Wasm EH but not Wasm SjLj, I don't think it's worth the
hassle of keeping three different decorators like
@with_both_exception_handling,@with_both_sjlj_handling,@with_both_eh_sjlj.@with_both_eh_sjljto several more tests that mix EH + SjLjtest_longjmpintotest_longjmp_stadnalone. I couldn'tfind a way to attach two decorators (
@with_both_eh_sjljand@also_with_standalone_wasm) to a single test.Fixes #15404.