Skip to content

Use builtin __wasm_exception_handling__ macro. NFC #25285

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Use builtin __wasm_exception_handling__ macro. NFC #25285

wants to merge 1 commit into from

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Sep 16, 2025

No description provided.

sbc100
sbc100 commented Sep 16, 2025
View reviewed changes
tools/system_libs.py
# The code used to interpret exceptions during terminate
# is not compatible with emscripten exceptions.
cflags.append('-DLIBCXXABI_SILENT_TERMINATE')
elif self.eh_mode == Exceptions.WASM_LEGACY:
Copy link
Collaborator Author

@sbc100 sbc100 Sep 16, 2025

Choose a reason for hiding this comment

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

Is there some reason why this one is only comparing with WASM_LEGACY and not WASM?
Copy link
Member

@aheejin aheejin Sep 17, 2025

Choose a reason for hiding this comment

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

I think I forgot to update it. But anyway this is not necessary because we define it in the clang preprocessor: https://github.com/llvm/llvm-project/blob/82acc31fd4b26723b61dae76da120c0c649d0b97/clang/lib/Frontend/InitPreprocessor.cpp#L1034-L1035
So if we are to change the name we have to update this too.
@sbc100 sbc100 requested a review from aheejin September 16, 2025 18:33
aheejin
aheejin approved these changes Sep 17, 2025
View reviewed changes
Copy link
Member

@aheejin aheejin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why do you want to change it? To be consistent with __wasm__?
I don't mind changing it, but this is upstreamed, so we have to change it there as well. The instances are

aheejin@aheejin:~/llvm-git/clang$ grep __WASM_EXCEPTIONS__ * -R lib/Frontend/InitPreprocessor.cpp: Builder.defineMacro("__WASM_EXCEPTIONS__"); test/CodeGenCXX/wasm-eh.cpp:// PREPROCESSOR: #define __WASM_EXCEPTIONS__ 1 aheejin@aheejin:~/llvm-git/libcxxabi$ grep __WASM_EXCEPTIONS__ * -R src/cxa_personality.cpp:#if !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#else // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS_ src/cxa_personality.cpp:#endif // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS_ src/cxa_personality.cpp:#if defined(__USING_SJLJ_EXCEPTIONS__) || defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#if defined(__USING_SJLJ_EXCEPTIONS__) || defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#else // !__USING_SJLJ_EXCEPTIONS__ && !__WASM_EXCEPTIONS__ src/cxa_personality.cpp:#endif // !__USING_SJLJ_EXCEPTIONS__ && !__WASM_EXCEPTIONS__ src/cxa_personality.cpp:#if defined(__USING_SJLJ_EXCEPTIONS__) || defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#if !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#else // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS_ src/cxa_personality.cpp:#endif // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS_ src/cxa_personality.cpp:#if !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#else // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS_ src/cxa_personality.cpp:#endif // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS_ src/cxa_personality.cpp:#if !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__WASM_EXCEPTIONS__) src/cxa_personality.cpp:#endif // !__USING_SJLJ_EXCEPTIONS__ && !__WASM_EXCEPTIONS__ src/cxa_personality.cpp:#ifdef __WASM_EXCEPTIONS__ src/cxa_personality.cpp:#ifdef __WASM_EXCEPTIONS__ aheejin@aheejin:~/llvm-git/libunwind$ grep __WASM_EXCEPTIONS__ * -R src/Unwind-wasm.c:#ifdef __WASM_EXCEPTIONS__ src/Unwind-wasm.c:#endif // defined(__WASM_EXCEPTIONS__)

Landing this before changing https://github.com/llvm/llvm-project/blob/82acc31fd4b26723b61dae76da120c0c649d0b97/clang/lib/Frontend/InitPreprocessor.cpp#L1034-L1035 will break the waterfall.
tools/system_libs.py
# The code used to interpret exceptions during terminate
# is not compatible with emscripten exceptions.
cflags.append('-DLIBCXXABI_SILENT_TERMINATE')
elif self.eh_mode == Exceptions.WASM_LEGACY:
Copy link
Member

@aheejin aheejin Sep 17, 2025

Choose a reason for hiding this comment

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

I think I forgot to update it. But anyway this is not necessary because we define it in the clang preprocessor: https://github.com/llvm/llvm-project/blob/82acc31fd4b26723b61dae76da120c0c649d0b97/clang/lib/Frontend/InitPreprocessor.cpp#L1034-L1035
So if we are to change the name we have to update this too.
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 17, 2025

Oh, if __WASM_EXCEPTIONS__ is defined by the pre-processor then maybe I'll instead just remove the setting of it here
@sbc100 sbc100 closed this Sep 17, 2025
@sbc100 sbc100 deleted the wasm_exception_handling_macro branch September 17, 2025 20:30
@aheejin
Copy link
Member

aheejin commented Sep 17, 2025

Oh, if __WASM_EXCEPTIONS__ is defined by the pre-processor then maybe I'll instead just remove the setting of it here

But don't you need the library part still?
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 17, 2025

Oh, if __WASM_EXCEPTIONS__ is defined by the pre-processor then maybe I'll instead just remove the setting of it here

But don't you need the library part still?

Which part are you referring to?
@aheejin
Copy link
Member

aheejin commented Sep 18, 2025

That you changed #ifdef __WASM_EXCEPTIONS__ to #ifdef __wasm_exception_handling__ in libraries.
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 18, 2025

That you changed #ifdef __WASM_EXCEPTIONS__ to #ifdef __wasm_exception_handling__ in libraries.

On further consideration (and on finding out that __WASM_EXCEPTIONS__ is builtin) I don't think we should do that.

__wasm_exception_handling__ is the feature flag which I think can be enabled even when wasm C++ exception handling is not enabled, eight?
@aheejin
Copy link
Member

aheejin commented Sep 18, 2025

Not sure if I understand. You mean we should keep __WASM_EXCEPTIONS__ in libraries? That I don't particularly mind, but what do you mean that it is a builtin? It is a preprocessor defined in clang when -fwasm-exceptions is used. Also not sure what you mean by __wasm_exception_handling__ is a feature flag that can be enabled when C++ EH is not used.

(Also llvm/llvm-project#159143 currently uses __wasm_exception_handling__)
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 18, 2025

Not sure if I understand. You mean we should keep __WASM_EXCEPTIONS__ in libraries? That I don't particularly mind, but what do you mean that it is a builtin? It is a preprocessor defined in clang when -fwasm-exceptions is used.

By "builtin" I mean "defined by the compiler", yes.

Also not sure what you mean by __wasm_exception_handling__ is a feature flag that can be enabled when C++ EH is not used.

I mean could enable the wasm exception handling feature while not enabled wasm C++ EH, couldn't you? At least there seem to be two difference compiler-defined macros at play here.

(Also llvm/llvm-project#159143 currently uses __wasm_exception_handling__)

Yes, because I want to define those tags whenever wasm exceptions handling is enabled, regardless of -fwasm-exceptions which, for example, might not make sense when compiling a .S file.
@aheejin
Copy link
Member

aheejin commented Sep 19, 2025

Do you mean we should keep two different preprocessors, __was_exception_handling__ for Wasm EH feature and __WASM_EXCEPTIONS for -fwasm-exceptions? That seems confusing... Can't we use a single one for that? Why would we need a preprocessor for Wasm EH feature? If you'd like to have a preprocessor that can also work for SjLj, we have __WASM_SJLJ__ for it. If we add one more preprocessor, we have three prepreocessors for Wasm EH/SjLj.

We talked about merging __WASM_EXCEPTION__ and __WASM_SJLJ__ but decided not to do it in #21971.
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 19, 2025

Do you mean we should keep two different preprocessors, __was_exception_handling__ for Wasm EH feature and __WASM_EXCEPTIONS for -fwasm-exceptions? That seems confusing... Can't we use a single one for that? Why would we need a preprocessor for Wasm EH feature? If you'd like to have a preprocessor that can also work for SjLj, we have __WASM_SJLJ__ for it. If we add one more preprocessor, we have three prepreocessors for Wasm EH/SjLj.

I believe each wasm feature gets its own preprocess definition. This means that __wasm_exception_handling__ is enabled automatically whenever the wasm EH is available. For example, this can be defined in C files (not just C++ files) or assembly files or even in C++ files with with -fno-exceptions. I think this makes sense because in theory wasm EH could be used for things other than C++ EH.

__WASM_EXCEPTION__ on the other hand seems to be specifically related to the C++ EH model and can be controlled via different, flags right?

If you think this is not needed then I suggest you propose removing one of them. Removing __wasm_exception_handling__ seems hard since that is automatic, based on the wasm feature set that is enabled.

We talked about merging __WASM_EXCEPTION__ and __WASM_SJLJ__ but decided not to do it in #21971.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@sbc100 @aheejin