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 all commits
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
17 changes: 11 additions & 6 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 @@ -287,14 +287,10 @@ __cxa_throw(void *thrown_object, std::type_info *tinfo, void (_LIBCXXABI_DTOR_FU

#ifdef __USING_SJLJ_EXCEPTIONS__
_Unwind_SjLj_RaiseException(&exception_header->unwindHeader);
#elif __USING_WASM_EXCEPTIONS__
#ifdef NDEBUG
_Unwind_RaiseException(&exception_header->unwindHeader);
#else
#elif defined(__USING_WASM_EXCEPTIONS__) && !defined(NDEBUG)
// 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);
#endif
#else
_Unwind_RaiseException(&exception_header->unwindHeader);
#endif
Expand Down Expand Up @@ -644,6 +640,10 @@ void __cxa_rethrow() {
}
#ifdef __USING_SJLJ_EXCEPTIONS__
_Unwind_SjLj_RaiseException(&exception_header->unwindHeader);
#elif defined(__USING_WASM_EXCEPTIONS__) && !defined(NDEBUG)
// 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__

#else
_Unwind_RaiseException(&exception_header->unwindHeader);
#endif
Expand Down Expand Up @@ -769,8 +769,13 @@ __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 defined(__USING_WASM_EXCEPTIONS__) && !defined(NDEBUG)
// 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);
#else
_Unwind_RaiseException(&dep_exception_header->unwindHeader);
_Unwind_RaiseException(&exception_header->unwindHeader);
#endif
// Some sort of unwinding error. Note that terminate is a handler.
__cxa_begin_catch(&dep_exception_header->unwindHeader);
Expand Down
90 changes: 80 additions & 10 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8529,7 +8529,7 @@ def test_exceptions_stack_trace_and_message(self, wasm_eh):
return 0;
}
'''
emcc_args = ['-g']
self.emcc_args = ['-g']

# Stack trace and message example for this example code:
# exiting due to exception: [object WebAssembly.Exception],Error: std::runtime_error,my message
Expand Down Expand Up @@ -8557,9 +8557,9 @@ def test_exceptions_stack_trace_and_message(self, wasm_eh):
# self.require_wasm_eh() if this issue is fixed later.
self.require_v8()

emcc_args += ['-fwasm-exceptions']
self.emcc_args += ['-fwasm-exceptions']
else:
emcc_args += ['-fexceptions']
self.emcc_args += ['-fexceptions']

# Stack traces are enabled when either of ASSERTIONS or
# EXCEPTION_STACK_TRACES is enabled. You can't disable
Expand All @@ -8568,16 +8568,14 @@ def test_exceptions_stack_trace_and_message(self, wasm_eh):
# Prints stack traces
self.set_setting('ASSERTIONS', 1)
self.set_setting('EXCEPTION_STACK_TRACES', 1)
self.do_run(src, emcc_args=emcc_args, assert_all=True,
assert_returncode=NON_ZERO, expected_output=stack_trace_checks,
regex=True)
self.do_run(src, assert_all=True, assert_returncode=NON_ZERO,
expected_output=stack_trace_checks, regex=True)

# Prints stack traces
self.set_setting('ASSERTIONS', 0)
self.set_setting('EXCEPTION_STACK_TRACES', 1)
self.do_run(src, emcc_args=emcc_args, assert_all=True,
assert_returncode=NON_ZERO, expected_output=stack_trace_checks,
regex=True)
self.do_run(src, assert_all=True, assert_returncode=NON_ZERO,
expected_output=stack_trace_checks, regex=True)

# Not allowed
self.set_setting('ASSERTIONS', 1)
Expand All @@ -8589,10 +8587,82 @@ def test_exceptions_stack_trace_and_message(self, wasm_eh):
# Doesn't print stack traces
self.set_setting('ASSERTIONS', 0)
self.set_setting('EXCEPTION_STACK_TRACES', 0)
err = self.do_run(src, emcc_args=emcc_args, assert_returncode=NON_ZERO)
err = self.do_run(src, assert_returncode=NON_ZERO)
for check in stack_trace_checks:
self.assertFalse(re.search(check, err), 'Expected regex "%s" to not match on:\n%s' % (check, err))

@parameterized({
'': (False,),
'wasm': (True,),
})
def test_exceptions_rethrow_stack_trace_and_message(self, wasm_eh):
self.emcc_args = ['-g']
if wasm_eh:
# FIXME Node v18.13 (LTS as of Jan 2023) has not yet implemented the new
# optional 'traceStack' option in WebAssembly.Exception constructor
# (https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/Exception/Exception)
# and embeds stack traces unconditionally. Change this back to
# self.require_wasm_eh() if this issue is fixed later.
self.require_v8()
self.emcc_args += ['-fwasm-exceptions']
else:
self.emcc_args += ['-fexceptions']

# Rethrowing exception currently loses the stack trace before the 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)
err = self.do_run(rethrow_src1, 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, 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