[EH] Make Wasm EH rethrow print stack traces #20372
Conversation
In Wasm EH, even if we set `-sASSERTION` or `-sEXCEPTION_STACK_TRACES`, if we rethrow an exception in the code, we lose the effect of that option because previously we called `__throw_exception_with_stack_trace` only in `__cxa_throw`: https://github.com/emscripten-core/emscripten/blob/9ce7020632aa6f7578c6e40e197b800a4dd7073f/system/lib/libcxxabi/src/cxa_exception.cpp#L294-L296 This adds the same mechanism to `__cxa_rethrow` (which is used for C++ `throw;`) and `__cxa_rethrow_primary_exception` (which is used for `std::rethrow_exception`). This does not solve the problem of losing stack traces _before_ the rethrowing. libc++abi's `__cxa_rethrow` and `__cxa_rethrow_primary_exception` are implemented as throwing a pointer in the same way we first throw it and they are not aware of any metadata. This happens even in the native platform with GDB; GDB's backtrace only shows stack traces after rethrowing. We may try to fix this later by passing `exnref` (WebAssembly/exception-handling#281) to the library, but this is likely to take some time. Partially fixes emscripten-core#20301.
| @@ -644,6 +644,14 @@ void __cxa_rethrow() { | |||
| } | |||
| #ifdef __USING_SJLJ_EXCEPTIONS__ | |||
| _Unwind_SjLj_RaiseException(&exception_header->unwindHeader); | |||
| #elif __USING_WASM_EXCEPTIONS__ | |||
There was a problem hiding this comment.
How about making this #elif defined(__USING_WASM_EXCEPTIONS__) && !defined(NDEBUG) to avoid inner if/else/endif?
There was a problem hiding this comment.
How? We have
#else
_Unwind_RaiseException(&exception_header->unwindHeader);
#endifat the end so I'm not sure how to rewrite this without an inner if...
There was a problem hiding this comment.
I was thinking you could write this to avoid the duplication of the _Unwind_RaiseException(&exception_header->unwindHeader); block? And instead rely on the fall though to that else case?
There was a problem hiding this comment.
Aha, sorry, I was confused and thought NDEBUG as the opposite... Simplified it, and also fixed the existing __cxa_throw part, which I copied this code from.
| #else | ||
| // In debug mode, call a JS library function to use WebAssembly.Exception JS | ||
| // API, which enables us to include stack traces | ||
| __throw_exception_with_stack_trace(&exception_header->unwindHeader); |
There was a problem hiding this comment.
This makes me wonder why _Unwind_RaiseException itself doesn't call __throw_exception_with_stack_trace under the hood? I'm sure there is a good reason..
There was a problem hiding this comment.
I have a reason, even though am not sure how good that reason is... In native platforms libunwind is mostly used for very low level stack unwinding stuff, so I wanted to limit libunwind to low-level stuff as well, such as throwing exceptions using builtins. Interacting with JS seemed a too high level functionality for libunwind.
There was a problem hiding this comment.
I guess it depends if we think there would be other callers of _Unwind_RaiseException that don't want __throw_exception_with_stack_trace ?
There was a problem hiding this comment.
libunwind can be used by non-Emscripten toolchains, so there are others that don't want to call it. But that's gonna be the same for libc++abi... Maybe we should guard this part as not only __USING_WASM_EXCEPTIONS__ but also __EMSCRIPTEN__? Currently I haven't upstreamed any JS-related parts, including this, to upstream LLVM, so others will not be affected though.
There was a problem hiding this comment.
Yes I think it makes sense to wrap this in __EMSCRIPTEN__. We can consider folding this into _Unwind_RaiseException itself, either as part of this PR, or as a followup.
There was a problem hiding this comment.
Also, I don't see any harm in (eventually) upstreaming even the emscripten parts, assuming they are wrapped in #ifdef __EMSCRIPTEN__
test/test_other.py
Outdated
| @@ -8593,6 +8593,64 @@ def test_exceptions_stack_trace_and_message(self, wasm_eh): | |||
| for check in stack_trace_checks: | |||
| self.assertFalse(re.search(check, err), 'Expected regex "%s" to not match on:\n%s' % (check, err)) | |||
|
|
|||
| # Rethrowing exception currently loses the stack trace before the | |||
There was a problem hiding this comment.
Can this be a separate test? The existing test is already really long..
test/test_other.py
Outdated
| self.require_v8() | ||
| emcc_args += ['-fwasm-exceptions'] | ||
| else: | ||
| emcc_args += ['-fexceptions'] |
There was a problem hiding this comment.
You can self.emcc_args here and avoid passing this as emcc_args to the self.do_run calls below.
There was a problem hiding this comment.
Ah, thanks. Fixed the same pattern in the original test_exceptions_stack_trace_and_message as well.
In Wasm EH, even if we set
-sASSERTIONor-sEXCEPTION_STACK_TRACES, if we rethrow an exception in the code, we lose the effect of that option because previously we called__throw_exception_with_stack_traceonly in__cxa_throw:emscripten/system/lib/libcxxabi/src/cxa_exception.cpp
Lines 294 to 296 in 9ce7020
This adds the same mechanism to
__cxa_rethrow(which is used for C++throw;) and__cxa_rethrow_primary_exception(which is used forstd::rethrow_exception).This does not solve the problem of losing stack traces before the rethrowing. libc++abi's
__cxa_rethrowand__cxa_rethrow_primary_exceptionare implemented as throwing a pointer in the same way we first throw it and they are not aware of any metadata. This happens even in the native platform with GDB; GDB's backtrace only shows stack traces after rethrowing. We may try to fix this later by passingexnref(WebAssembly/exception-handling#281) to the library, but this is likely to take some time.
Partially fixes #20301.