Skip to content
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

[EH] Make Wasm EH rethrow print stack traces #20372

Merged
merged 4 commits into from
Oct 3, 2023
Merged

[EH] Make Wasm EH rethrow print stack traces #20372

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a208c42
[EH] Make Wasm EH rethrow print stack traces
aheejin Sep 29, 2023
a7c5fe5
Address comments
aheejin Oct 2, 2023
d3f7e5b
Simplify if
aheejin Oct 2, 2023
ca2da5c
Use self.emcc_args
aheejin Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions system/lib/libcxxabi/src/cxa_exception.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,14 @@ void __cxa_rethrow() {
}
#ifdef __USING_SJLJ_EXCEPTIONS__
_Unwind_SjLj_RaiseException(&exception_header->unwindHeader);
#elif __USING_WASM_EXCEPTIONS__
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about making this #elif defined(__USING_WASM_EXCEPTIONS__) && !defined(NDEBUG) to avoid inner if/else/endif?

Copy link
Member Author

Choose a reason for hiding this comment

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

How? We have

#else
    _Unwind_RaiseException(&exception_header->unwindHeader);
#endif

at the end so I'm not sure how to rewrite this without an inner if...

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#ifdef NDEBUG
_Unwind_RaiseException(&exception_header->unwindHeader);
#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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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..

Copy link
Member Author

@aheejin aheejin Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it depends if we think there would be other callers of _Unwind_RaiseException that don't want __throw_exception_with_stack_trace ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I don't see any harm in (eventually) upstreaming even the emscripten parts, assuming they are wrapped in #ifdef __EMSCRIPTEN__

#endif
#else
_Unwind_RaiseException(&exception_header->unwindHeader);
#endif
Expand Down Expand Up @@ -769,6 +777,14 @@ __cxa_rethrow_primary_exception(void* thrown_object)
dep_exception_header->unwindHeader.exception_cleanup = dependent_exception_cleanup;
#ifdef __USING_SJLJ_EXCEPTIONS__
_Unwind_SjLj_RaiseException(&dep_exception_header->unwindHeader);
#elif __USING_WASM_EXCEPTIONS__
#ifdef NDEBUG
_Unwind_RaiseException(&exception_header->unwindHeader);
#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);
sbc100 marked this conversation as resolved.
Show resolved Hide resolved
#endif
#else
_Unwind_RaiseException(&dep_exception_header->unwindHeader);
#endif
Expand Down
58 changes: 58 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be a separate test? The existing test is already really long..

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

# rethrowing due to how rethrowing is implemented. So in the examples below
# we don't print 'bar' at the moment.
# TODO Make rethrow preserve stack traces before rethrowing?
rethrow_src1 = r'''
#include <stdexcept>

void bar() {
throw std::runtime_error("my message");
}
void foo() {
try {
bar();
} catch (...) {
throw; // rethrowing by throw;
}
}
int main() {
foo();
return 0;
}
'''
rethrow_src2 = r'''
#include <stdexcept>

void bar() {
throw std::runtime_error("my message");
}
void foo() {
try {
bar();
} catch (...) {
auto e = std::current_exception();
std::rethrow_exception(e); // rethrowing by std::rethrow_exception
}
}
int main() {
foo();
return 0;
}
'''
rethrow_stack_trace_checks = [
'std::runtime_error[:,][ ]?my message', # 'std::runtime_error: my message' for Emscripten EH
'at ((src.wasm.)?_?__cxa_rethrow|___resumeException)', # '___resumeException' (JS symbol) for Emscripten EH
'at (src.wasm.)?foo',
'at (src.wasm.)?main']

self.set_setting('ASSERTIONS', 1)
self.clear_setting('EXCEPTION_STACK_TRACES')
err = self.do_run(rethrow_src1, emcc_args=emcc_args, assert_all=True,
assert_returncode=NON_ZERO,
expected_output=rethrow_stack_trace_checks, regex=True)
self.assertNotContained('bar', err)
err = self.do_run(rethrow_src2, emcc_args=emcc_args, assert_all=True,
assert_returncode=NON_ZERO,
expected_output=rethrow_stack_trace_checks, regex=True)
self.assertNotContained('bar', err)

@requires_node
def test_jsrun(self):
print(config.NODE_JS)
Expand Down