-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[EH] Make std::terminate() work with EH #16921
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
Conversation
`noexcept` function shouldn't throw, so `noexcept` function code generation is to `invoke` every function call in those functions and in case they throw, call `std::terminate`. This codegen comes from clang and native platforms do this too. So in wasm, they become something like ```wasm try function body catch_all call std::terminate end ``` `std::terminate` calls `std::__terminate`. Both of `std::terminate` and `std::__terminate` are `noexcept` now. So that means their code is structured like that, which sounds like self-calling, but normally no function calls in those functions should ever throw, so that's fine. But in our case, `abort` ends up throwing, which is a problem. The function body of `__terminate` eventually calls JS `abort`, and ends up here: https://github.com/emscripten-core/emscripten/blob/970998b2670a9bcf39d31e2b01db571089955add/src/preamble.js#L605-L623 This ends up throwing a JS exception. This is basically just a foreign exception from the wasm perspective, and is caught by `catch_all`, and calls `std::terminate` again. And the whole process continues until the call stack is exhausted. What emscripten-core#9730 tried to do was throwing a trap, because Wasm `catch`/`catch_all` don't catch traps. Traps become `RuntimeError`s after they hit a JS frame. To be consistent, we decided `catch`/`catch_all` shouldn't catch them after they become `RuntimeError`s. That's the reason emscripten-core#9730 changed the code to throw not just any random thing but `RuntimeError`. But somehow we decided that we make that trap distinction not based on `RuntimeError` class but some hidden field (WebAssembly/exception-handling#89 (comment)). This PR removes `noexcept` from `std::terminate` and `std::__terminate`'s signatures so that the cleanup that contains `catch_all` is not generated for those two functions. So now the JS exception thrown by `abort()` will unwind the stack, which is different from native, but that can be considered OK because I don't think users expect `abort` to preserve the stack intact? Fixes emscripten-core#16407.
This is an alternative approach to #16910. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions:
- Are there downsides to removing
noexcept
? I'd guess it prevents some LLVM optimizations, maybe mostly in LTO, but probably[[noreturn]]
is good enough anyhow? - Can we get rid of the try-catch for noexcept functions in release builds? I understand from you that they appear in native builds too. But it seems like they handle an error case by converting one type of error (an exception thrown where one shouldn't be) into another (a terminate), and I'd hope that a release build would prefer to not add overhead for that. I guess in theory an exception could be thrown that is caught by the application higher up, so the try-catch converts potentially incorrect internal operations into a clear immediate terminate, but still, that feels odd to me in a release build - it feels like assertions used in debug builds.
@@ -45,7 +45,11 @@ unexpected_handler get_unexpected() noexcept; | |||
typedef void (*terminate_handler)(); | |||
terminate_handler set_terminate(terminate_handler f ) noexcept; | |||
terminate_handler get_terminate() noexcept; | |||
#ifdef __USING_WASM_EXCEPTIONS__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add a comment here, or just point people to libcxxabi for an explanation. Unless that's obvious enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure if it would be OK to copy-paste the same comment four times. Did that anyway.
I don't think it would harm optimizations. The frontend doesn't think
But that will make behavior different. void foo() noexcept {
throw 3;
}
int main() {
try {
foo();
} catch (...) {
printf("I shouldn't be printed");
}
} "I shouldn't be printed" will not be printed in debug builds but will be printed in release builds. But I became more skeptical about this approach; maybe we shouldn't run destructors after |
Oh, right, this is more general than terminate... Yes, my suggestion wasn't useful then. I lean towards #16910 too, but I don't have a strong preference. |
Closing in favor of #16910. |
noexcept
function shouldn't throw, sonoexcept
function codegeneration is to
invoke
every function call in those functions and incase they throw, call
std::terminate
. This codegen comes from clangand native platforms do this too. So in wasm, they become something like
std::terminate
callsstd::__terminate
. Both ofstd::terminate
andstd::__terminate
arenoexcept
now. So that means their code isstructured like that, which sounds like self-calling, but normally no
function calls in those functions should ever throw, so that's fine. But
in our case,
abort
ends up throwing, which is a problem.The function body of
__terminate
eventually calls JSabort
, and endsup here:
emscripten/src/preamble.js
Lines 605 to 623 in 970998b
This ends up throwing a JS exception. This is basically just a foreign
exception from the wasm perspective, and is caught by
catch_all
, andcalls
std::terminate
again. And the whole process continues until thecall stack is exhausted.
What #9730 tried to do was throwing a trap, because Wasm
catch
/catch_all
don't catch traps. Traps becomeRuntimeError
safter they hit a JS frame. To be consistent, we decided
catch
/catch_all
shouldn't catch them after they becomeRuntimeError
s. That's the reason #9730 changed the code to throw notjust any random thing but
RuntimeError
. But somehow we decided that wemake that trap distinction not based on
RuntimeError
class but somehidden field
(WebAssembly/exception-handling#89 (comment)).
This PR removes
noexcept
fromstd::terminate
andstd::__terminate
's signatures so that the cleanup that containscatch_all
is not generated for those two functions. So now the JSexception thrown by
abort()
will unwind the stack, which is differentfrom native, but that can be considered OK because I don't think users
expect
abort
to preserve the stack intact?Fixes #16407.