emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.67k stars 3.29k forks source link

[EH] Add __cxa_init_primary_exception to cxa_noexception.cpp #22027

Closed aheejin closed 4 months ago

aheejin commented 4 months ago

tl;dr:

This adds __cxa_init_primary_exception to cxa_noexception.cpp.

Other targets build cxa_exception.cpp in -fignore-exceptions mode and build cxa_noexception.cpp only in -fno-exeptions mode. But we build cxa_noexception.cpp in -fignore-exceptions mode, which means it needs the definition for __cxa_init_primary_exception for linking to succeed when no EH argument is given (which means -fignore-exceptions).


Long version (Feel free to skip):

Background:

After #21638, __cxa_init_primary_exception was added in libcxxabi: https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxxabi/src/cxa_exception.cpp#L209-L226 https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxxabi/src/cxa_exception_emscripten.cpp#L155-L162

Currently the files containing __cxa_init_primary_exception, cxa_exception.cpp and cxa_exception_emscripten.cpp, are only compiled when any of the EH mode is specified. cxa_exception.cpp is compiled when Wasm EH is selected, and cxa_exception_emscripten.cpp is compiled when Emscripten EH is selected: https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/tools/system_libs.py#L1599-L1608

and this function is called from make_exception_ptr in libcxx: https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxx/include/__exception/exception_ptr.h#L87-L99

And make_exception_ptr is called from std::promise's destructor: https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxx/include/future#L1161-L1168


Bug:

Currently any program that calls std::promise's destructor without specifying any exception-related arguments fails, saying undefined symbol: __cxa_init_primary_exception. Not specifying any exception arguments, meaning not specifying any among -fno-exceptions, -fwasm-exceptions, -fexceptions, or -sDISABLE_EXCEPTION_CATCHING=0, defaults to -fignore-exceptions, which allows throwing but not catching.


Analysis:

The callsite of __cxa_init_primary_exception in make_exception_ptr is guarded with #ifndef _LIBCPP_HAS_NO_EXCEPTIONS, so it seems it is supposed to be included only when exceptions are enabled. This _LIBCPP_HAS_NO_EXCEPTIONS is defined when __cpp_exceptions is defined: https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxx/include/__config#L644-L646

And that __cpp_exceptions is defined in clang, when -fcxx-exceptions is given: https://github.com/llvm/llvm-project/blob/be566d2eacdaed972b90d2eeb1e66d732c9fe7c1/clang/lib/Frontend/InitPreprocessor.cpp#L638-L639

And that -fcxx-exceptions can be specified in command line, but it is also programmatically added here: https://github.com/llvm/llvm-project/blob/be566d2eacdaed972b90d2eeb1e66d732c9fe7c1/clang/lib/Driver/ToolChains/Clang.cpp#L371-L388

You can see it is added by default unless the arch is XCore or PS4/PS5, which means for Wasm it has been always added so far, unless -fno-exceptions is explicitly specified.

So I tried checking the arguments there in Clang and adding -fcxx-exceptions only if either of -fwasm-exceptions or -enable-emscripten-cxx-exceptions is given. But this fails when none of EH is selected (which means -fignore-exceptions), because if -fcxx-exceptions is not added, we can't use keywords like throw.

So basically the problem is, other targets build cxa_exception.cpp in -fignore-exceptions mode and build cxa_noexception.cpp only in -fno-exeptions mode. But we build cxa_noexception.cpp in -fignore-exceptions mode, which means it needs the definition for __cxa_init_primary_exception for linking to succeed, because _LIBCPP_HAS_NO_EXCEPTIONS cannot be defined in -fignore-exceptions (because we couldn't disable -fcxx-exceptions above in Clang to use throw)

So this adds __cxa_init_primary_exception to cxa_noexception.cpp.

aheejin commented 4 months ago

Not sure. I've been upstreaming the changes that are likely necessary for upstream CMake users, and unless they use our system_libs.py, this is not gonna be necessary.

Also, cxa_noexceptions.cpp contains other emscripten-only functions too. Do you think we should upstream all these? https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxxabi/src/cxa_noexception.cpp#L52-L77

mzukovec commented 4 months ago

Thanks for looking into this!

sbc100 commented 4 months ago

Not sure. I've been upstreaming the changes that are likely necessary for upstream CMake users, and unless they use our system_libs.py, this is not gonna be necessary.

Also, cxa_noexceptions.cpp contains other emscripten-only functions too. Do you think we should upstream all these?

https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxxabi/src/cxa_noexception.cpp#L52-L77

I guess my question is, is this __cxa_init_primary_exception (or the need for it) emscripten-specific? How come calls to it are being generated by the wasm backend but not other backends?

aheejin commented 4 months ago

I guess my question is, is this __cxa_init_primary_exception (or the need for it) emscripten-specific? How come calls to it are being generated by the wasm backend but not other backends?

Calls to __cxa_init_primary_exception are generated in other backends too. But they use cxa_exceptions.cpp in case of -fignore-exceptions so it's not a problem. They use cxa_noexceptions.cpp only when -fno-exceptions is given, in which case _LIBCPP_HAS_NO_EXCEPTIONS is defined and the calls to __cxa_init_primary_exception are not compiled: https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxx/include/__exception/exception_ptr.h#L87-L99

But we use cxa_noexception.cpp in case of -fignore-exceptions, which is our default. That's why I added the function there.

sbc100 commented 4 months ago

I guess my question is, is this __cxa_init_primary_exception (or the need for it) emscripten-specific? How come calls to it are being generated by the wasm backend but not other backends?

Calls to __cxa_init_primary_exception are generated in other backends too. But they use cxa_exceptions.cpp in case of -fignore-exceptions so it's not a problem. They use cxa_noexceptions.cpp only when -fno-exceptions is given, in which case _LIBCPP_HAS_NO_EXCEPTIONS is defined and the calls to __cxa_init_primary_exception is not compiled:

https://github.com/emscripten-core/emscripten/blob/28c10a1c5e2862edadd68ab627478204ae96d134/system/lib/libcxx/include/__exception/exception_ptr.h#L87-L99

But we use cxa_noexception.cpp in case of -fignore-exceptions, which is our default. That's why I added the function there.

I see.. so the issue stems from the different/special way that we handle -fignore-exceptions. Might be worth adding a comment here to that effect.

aheejin commented 4 months ago

I see.. so the issue stems from the different/special way that we handle -fignore-exceptions. Might be worth adding a comment here to that effect.

22034