Skip to content

Conversation

@aheejin
Copy link
Member

@aheejin aheejin commented May 21, 2024

In line with #21970, this changes __USING_WASM_SJLJ__ to __WASM_SJLJ__. We don't define this in Clang now (which we can maybe consider, but given that Clang doesn't have dedicated flags for SjLj and there don't seem to be other platforms doing similar things), but given that the affected files currently are only in Emscripten, I think we can safely change this.

In line with emscripten-core#21970, this changes `__USING_WASM_SJLJ__` to `__WASM_SJLJ__`. We don't define this in Clang now (which we can maybe consider, but given that Clang doesn't have dedicated flags for SjLj and there don't seem to be other platforms doing similar things), but given that the affected files currently are only in Emscripten, I think we can safely change this.
@aheejin aheejin requested a review from sbc100 May 21, 2024 19:57
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Do we need this macro in practice? Can we perhaps merge it with WASM_EXCEPTIONS?

@aheejin
Copy link
Member Author

aheejin commented May 21, 2024

That's a good question... I think we separated it mainly because there was time we supported Wasm EH but not Wasm SjLj so we let people use Wasm EH with Emscripten SjLj. This still works but not the recommended mode of using it anymore; we now use Wasm SjLj by default if -fwasm-exceptions is given. But you can still manually override it by -fwasm-exceptions -sSUPPORT_LONGJMP=emscripten or something.

Merging the two preprocessor means we disallow using Wasm EH with Emscripten SjLj. Would that be OK? (At this point I can't think of a reason why someone wants to use Wasm EH with Emscripten SjLj though)

@sbc100
Copy link
Collaborator

sbc100 commented May 21, 2024

That's a good question... I think we separated it mainly because there was time we supported Wasm EH but not Wasm SjLj so we let people use Wasm EH with Emscripten SjLj. This still works but not the recommended mode of using it anymore; we now use Wasm SjLj by default if -fwasm-exceptions is given. But you can still manually override it by -fwasm-exceptions -sSUPPORT_LONGJMP=emscripten or something.

Merging the two preprocessor means we disallow using Wasm EH with Emscripten SjLj. Would that be OK? (At this point I can't think of a reason why someone wants to use Wasm EH with Emscripten SjLj though)

The more configurations we can disallow the better IMO!

@aheejin
Copy link
Member Author

aheejin commented Jan 7, 2025

I think we can probably land this PR as is. What I commented was actually incorrect. We actually already have been forcing Wasm SjLj when Wasm EH is used (#18692), which predates this PR. And while replacing the three cases of __WASM_SJLJ__ here with __WASM_EXCEPTIONS__ doesn't seem to break anything, it doesn't seem to simplify anything either, because the only place we use __WASM_SJLJ__ is SjLjLibrary and the only subclass of SjLjLibrary is libcompiler_rt. So even if we replace __WASM_SJLJ__ with __WASM_EXCEPTIONS__ in libcompiler_rt these usages are separate from the usage in libcxx/libcxxabi/libunwind, and it looks more confusing because libcompiler_rt doesn't seem to be related to exceptions.

@aheejin aheejin requested a review from sbc100 January 7, 2025 21:54
@aheejin aheejin merged commit 9096999 into emscripten-core:main Jan 8, 2025
29 checks passed
@aheejin aheejin deleted the wasm_sjlj branch January 8, 2025 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants