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

[SjLj] __USING_WASM_SJLJ__ -> __WASM_SJLJ__ #21971

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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!

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.

None yet

2 participants