Fix std::nothrow aborting when exceptions are disabled #20154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
M-Mueller
changed the title
M-Mueller
changed the title
sbc100
reviewed
Aug 29, 2023
sbc100
approved these changes
Aug 30, 2023
kripken
reviewed
Aug 31, 2023
RReverser
reviewed
Oct 19, 2023
Co-authored-by: Ingvar Stepanyan <me@rreverser.com>
auto-merge was automatically disabled
October 19, 2023 16:08
Head branch was pushed to by a user without write access
|
Any tips on how to make the CI pass? The failing tests seem to be unrelated to the changes and each time other tests are failing. I cannot reproduce the failures locally (though other tests fail) |
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Oct 26, 2023
This test was recently added in emscripten-core#20154 but has been failing on the emscripten-releases waterfall in thinlto modes. I'm not sure exactly why, but it seems that perhaps when the value of `data` is not actually thinLTO simple removes the call the new/malloc? I'm not sure why this wasn't also failing under full LTO.
sbc100
added a commit
that referenced
this pull request
Oct 26, 2023
This test was recently added in #20154 but has been failing on the emscripten-releases waterfall in thinlto modes. I'm not sure exactly why, but it seems that perhaps when the value of `data` is not actually thinLTO simple removes the call the new/malloc? I'm not sure why this wasn't also failing under full LTO.
aheejin
added a commit
that referenced
this pull request
Apr 3, 2024
This updates libcxx and libcxxabi to LLVM 18.1.2. Due to llvm/llvm-project#65534 we need to update both at the same time. Things to note: - Disable hardening mode: LLVM 18.1.2 adds support for "Hardening Modes" to libcxx (https://libcxx.llvm.org/Hardening.html). There are four modes: none, fast, extensive and debug. Different hardening modes make different trade-offs between the amount of checking and runtime performance. We for now disable it (i.e., set it to 'none') so that we don't have any effects on the performance. We can consider enabling it when we ever get to enable the debug version of libcxx. - Disable C++20 time zone support: LLVM 18.1.2 adds C++20 time zone support (https://libcxx.llvm.org/DesignDocs/TimeZone.html) to libcxx, which requires access IANA Time Zone Database. Currently it seems it only supports Linux: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/src/tz.cpp#L45-L49 So this excludes the two source files from build (which is done via `CMakeLists.txt` in the upstream LLVM) and sets `_LIBCPP_HAS_NO_TIME_ZONE_DATABASE` macro in `__config_site`. In future maybe we can consider implementing this in JS. - `__cxa_init_primary_exception` support: llvm/llvm-project#65534 introduces `__cxa_init_primary_exception` and uses this in libcxx. Like several other methods like the below in `cxa_exception.cpp`, https://github.com/emscripten-core/emscripten/blob/fbdd9249e939660cb0d20f00d6bc1897c2f3905e/system/lib/libcxxabi/src/cxa_exception.cpp#L264-L269 this new function takes a pointer to a destructor, and in Wasm destructor returns a `this` pointer, which requires `ifdef __USING_WASM_EXCEPTION__` on its signature. And that it is also used in libcxx means we need to guard some code in libcxx with `ifdef __USING_WASM_EXCEPTION__` for the signature difference, and we need to build libcxx with `-D__USING_WASM_EXCEPTIONS__` when Wasm EH is used. Also we add Emscripte EH's counterpart in `cxa_exception_emscripten.cpp` too. - This version fixes long-running misbehaviors of `operator new` llvm/llvm-project#69498 seems to fix some long-running misbhaviors of `operator new`, which in emscripten we tried to fix in #11079 and #20154. So while resolving the conflicts, I effectively reverted #11079 and #20154 in `libcxx/src/stdlib_new_delete.cpp` and `libcxx/src/new.cpp`. - Add default `__assertion_handler` to `libcxx/include`: llvm/llvm-project#77883 adds a way for vendors to override how we handle assertions. If a vendor doesn't want to override it, CMake will copy the provided [default template assertion handler file](https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/vendor/llvm/default_assertion_handler.in) to libcxx's generated include dir, which defaults to `SYSROOT/include/c++/v1`: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L72-L73 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L439 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/include/CMakeLists.txt#L1024 We don't use CMake, so this renames the provided `vendor/llvm/default_assertion_handler.in` to `__assert_handler` in our `libcxx/include` directory, which will be copied to `SYSROOT/include/c++/v1`. - Disable `__libcpp_verbose_abort directly` in code: In #20707 we decided to disable `__libcpp_verbose_abort` not to incur the code size increase that brings (it brings in `vfprintf` and its family). We disabled it in adding `#define _LIBCPP_AVAILABILITY_HAS_NO_VERBOSE_ABORT` in `__config_site`. But that `_NO_` macros are gone in LLVM 18.1.2, and changing it to `#define _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT 0` does not work because it is overridden by this line, unless we provide our own vendor annotations: https://github.com/llvm/llvm-project/blob/38f5596feda3276a8aa64fc14e074334017088ca/libcxx/include/__availability#L138 I asked about this in llvm/llvm-project#87012 and llvm/llvm-project#71002 (comment) (and more comments below) but didn't get an actionable answer yet. So this disables `__libcpp_verbose_abort` in the code directly for now, hopefully temporarily. Each fix is described in its own commit, except for the fixes required to resolve conflicts when merging our changes, which I wasn't able to commit separately. Fixes #21603.
impact-maker
pushed a commit
to impact-maker/emscripten
that referenced
this pull request
Apr 11, 2024
This updates libcxx and libcxxabi to LLVM 18.1.2. Due to llvm/llvm-project#65534 we need to update both at the same time. Things to note: - Disable hardening mode: LLVM 18.1.2 adds support for "Hardening Modes" to libcxx (https://libcxx.llvm.org/Hardening.html). There are four modes: none, fast, extensive and debug. Different hardening modes make different trade-offs between the amount of checking and runtime performance. We for now disable it (i.e., set it to 'none') so that we don't have any effects on the performance. We can consider enabling it when we ever get to enable the debug version of libcxx. - Disable C++20 time zone support: LLVM 18.1.2 adds C++20 time zone support (https://libcxx.llvm.org/DesignDocs/TimeZone.html) to libcxx, which requires access IANA Time Zone Database. Currently it seems it only supports Linux: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/src/tz.cpp#L45-L49 So this excludes the two source files from build (which is done via `CMakeLists.txt` in the upstream LLVM) and sets `_LIBCPP_HAS_NO_TIME_ZONE_DATABASE` macro in `__config_site`. In future maybe we can consider implementing this in JS. - `__cxa_init_primary_exception` support: llvm/llvm-project#65534 introduces `__cxa_init_primary_exception` and uses this in libcxx. Like several other methods like the below in `cxa_exception.cpp`, https://github.com/emscripten-core/emscripten/blob/fbdd9249e939660cb0d20f00d6bc1897c2f3905e/system/lib/libcxxabi/src/cxa_exception.cpp#L264-L269 this new function takes a pointer to a destructor, and in Wasm destructor returns a `this` pointer, which requires `ifdef __USING_WASM_EXCEPTION__` on its signature. And that it is also used in libcxx means we need to guard some code in libcxx with `ifdef __USING_WASM_EXCEPTION__` for the signature difference, and we need to build libcxx with `-D__USING_WASM_EXCEPTIONS__` when Wasm EH is used. Also we add Emscripte EH's counterpart in `cxa_exception_emscripten.cpp` too. - This version fixes long-running misbehaviors of `operator new` llvm/llvm-project#69498 seems to fix some long-running misbhaviors of `operator new`, which in emscripten we tried to fix in emscripten-core#11079 and emscripten-core#20154. So while resolving the conflicts, I effectively reverted emscripten-core#11079 and emscripten-core#20154 in `libcxx/src/stdlib_new_delete.cpp` and `libcxx/src/new.cpp`. - Add default `__assertion_handler` to `libcxx/include`: llvm/llvm-project#77883 adds a way for vendors to override how we handle assertions. If a vendor doesn't want to override it, CMake will copy the provided [default template assertion handler file](https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/vendor/llvm/default_assertion_handler.in) to libcxx's generated include dir, which defaults to `SYSROOT/include/c++/v1`: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L72-L73 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L439 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/include/CMakeLists.txt#L1024 We don't use CMake, so this renames the provided `vendor/llvm/default_assertion_handler.in` to `__assert_handler` in our `libcxx/include` directory, which will be copied to `SYSROOT/include/c++/v1`. - Disable `__libcpp_verbose_abort directly` in code: In emscripten-core#20707 we decided to disable `__libcpp_verbose_abort` not to incur the code size increase that brings (it brings in `vfprintf` and its family). We disabled it in adding `#define _LIBCPP_AVAILABILITY_HAS_NO_VERBOSE_ABORT` in `__config_site`. But that `_NO_` macros are gone in LLVM 18.1.2, and changing it to `#define _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT 0` does not work because it is overridden by this line, unless we provide our own vendor annotations: https://github.com/llvm/llvm-project/blob/38f5596feda3276a8aa64fc14e074334017088ca/libcxx/include/__availability#L138 I asked about this in llvm/llvm-project#87012 and llvm/llvm-project#71002 (comment) (and more comments below) but didn't get an actionable answer yet. So this disables `__libcpp_verbose_abort` in the code directly for now, hopefully temporarily. Each fix is described in its own commit, except for the fixes required to resolve conflicts when merging our changes, which I wasn't able to commit separately. Fixes emscripten-core#21603.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not super happy with the code duplication, but didn't want to touch the regular
newimplementation. This way it should have no impact on code that does not usestd::nothrow.