-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[EH] Add EXCEPTION_STACK_TRACES option #18642
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2785,9 +2785,13 @@ def get_full_import_name(name): | |
| if settings.WASM_EXCEPTIONS: | ||
| settings.REQUIRED_EXPORTS += ['__trap'] | ||
|
|
||
| # When ASSERTIONS is set, we include stack traces in Wasm exception objects | ||
| # using the JS API, which needs this C++ tag exported. | ||
| if settings.ASSERTIONS and settings.WASM_EXCEPTIONS: | ||
| # When ASSERTIONS or EXCEPTION_STACK_TRACES is set, we include stack traces in | ||
| # Wasm exception objects using the JS API, which needs this C++ tag exported. | ||
| if settings.ASSERTIONS: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have thought My guess is that adding this error and testing for it might be more complex than just supporting that config?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure who would want
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I banned the combination and added a test for that. (I tested all four combinations) |
||
| if 'EXCEPTION_STACK_TRACES' in user_settings and not settings.EXCEPTION_STACK_TRACES: | ||
| exit_with_error('EXCEPTION_STACK_TRACES cannot be disabled when ASSERTIONS are enabled') | ||
| default_setting('EXCEPTION_STACK_TRACES', 1) | ||
| if settings.EXCEPTION_STACK_TRACES and settings.WASM_EXCEPTIONS: | ||
| settings.EXPORTED_FUNCTIONS += ['___cpp_exception'] | ||
| settings.EXPORT_EXCEPTION_HANDLING_HELPERS = True | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8062,7 +8062,12 @@ def test_wasm_nope(self): | |
| out = self.run_js('a.out.js', assert_returncode=NON_ZERO) | ||
| self.assertContained('no native wasm support detected', out) | ||
|
|
||
| @requires_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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm note sure its useful but we now have ways to checking the node version used in testing (looks for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our CI is fine without this, because we use node v14, which doesn't have Wasm EH support at all, and your logic will make sure we fall back to v8. But if we upgrade our node to a version such as v18, this will start to fail on our CI, and who will do the update can be confused by the failures, so I made this changes. Another thing we can do is we leave
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now also run some tests of the very latest node tip of tree (see
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just FYI in case you want to run some wasm EH under node too... we can now.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested with several node versions:
Given this a little confusing situation, I'm tempted to use only V8 for this test for now... |
||
| # @requires_wasm_eh if this issue is fixed later. | ||
| @requires_v8 | ||
| def test_wasm_exceptions_stack_trace_and_message(self): | ||
| src = r''' | ||
| #include <stdexcept> | ||
|
|
@@ -8098,12 +8103,30 @@ def test_wasm_exceptions_stack_trace_and_message(self): | |
| 'at foo', | ||
| 'at main'] | ||
|
|
||
| # We attach stack traces to exception objects only when ASSERTIONS is set | ||
| self.set_setting('ASSERTIONS') | ||
| # Stack traces are enabled when either of ASSERTIONS or | ||
| # EXCEPTION_STACK_TRACES is enabled. You can't disable | ||
| # EXCEPTION_STACK_TRACES when ASSERTIONS is enabled. | ||
|
|
||
| # 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) | ||
|
|
||
| # 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) | ||
|
|
||
| # Not allowed | ||
| create_file('src.cpp', src) | ||
| err = self.expect_fail([EMCC, 'src.cpp', '-sASSERTIONS=1', '-sEXCEPTION_STACK_TRACES=0']) | ||
| self.assertContained('EXCEPTION_STACK_TRACES cannot be disabled when ASSERTIONS are enabled', err) | ||
|
|
||
| # 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) | ||
| for check in stack_trace_checks: | ||
| self.assertNotContained(check, err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1376,6 +1376,15 @@ class libcxxabi(NoExceptLibrary, MTLibrary, DebugLibrary): | |||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||
| includes = ['system/lib/libcxx/src'] | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def __init__(self, **kwargs): | ||||||||||||||||||||||||||||||||
| super().__init__(**kwargs) | ||||||||||||||||||||||||||||||||
| # TODO EXCEPTION_STACK_TRACES currently requires the debug version of | ||||||||||||||||||||||||||||||||
| # libc++abi, causing the debug version of libc++abi to be linked, which | ||||||||||||||||||||||||||||||||
| # increases code size. libc++abi is not a big library to begin with, but if | ||||||||||||||||||||||||||||||||
| # this becomes a problem, consider making EXCEPTION_STACK_TRACES work with | ||||||||||||||||||||||||||||||||
| # the non-debug version of libc++abi. | ||||||||||||||||||||||||||||||||
| self.is_debug |= settings.EXCEPTION_STACK_TRACES | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can also revert this I think?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so? The point of this PR is to display stack traces when if settings.ASSERTIONS:
default_setting('EXCEPTION_STACK_TRACES', 1)This doesn't make
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, yes I agree then. Can you add a comment here maybe" "
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of this: emscripten/system/lib/libcxxabi/src/cxa_exception.cpp Lines 290 to 297 in 6ac4b2a
We could've come up with a separate preprocessor
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we instead remove Or is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the comment.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
emscripten/system/lib/libunwind/src/Unwind-wasm.c Lines 50 to 56 in 36c194d
We can move the differing behavior from libc++abi to libunwind, but given that libunwind is meant to be a low-level library and our
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the debug version will affect (If that ends up an issue, we could maybe have a separate library for the function that calls out to JS, and link it first so it overrides part of libcxxabi, but hopefully that's not necessary.)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already use the debug version of libc++abi when If we want to avoid the code size increase for libc++abi when FWIW, the code sizes for aheejin@aheejin:~/emscripten/cache/sysroot/lib/wasm32-emscripten$ ls libc++abi* -al
-rw-r----- 1 aheejin primarygroup 935626 Feb 1 13:33 libc++abi.a
-rw-r----- 1 aheejin primarygroup 1295312 Feb 1 13:33 libc++abi-debug.a
-rw-r----- 1 aheejin primarygroup 1376762 Feb 1 13:34 libc++abi-debug-except.a
-rw-r----- 1 aheejin primarygroup 1310656 Feb 1 13:33 libc++abi-debug-mt.a
-rw-r----- 1 aheejin primarygroup 1396908 Feb 1 13:34 libc++abi-debug-mt-except.a
-rw-r----- 1 aheejin primarygroup 1304380 Feb 1 13:33 libc++abi-debug-mt-noexcept.a
-rw-r----- 1 aheejin primarygroup 1289086 Feb 1 13:33 libc++abi-debug-noexcept.a
-rw-r----- 1 aheejin primarygroup 1310656 Feb 1 13:33 libc++abi-debug-ww.a
-rw-r----- 1 aheejin primarygroup 1396908 Feb 1 13:34 libc++abi-debug-ww-except.a
-rw-r----- 1 aheejin primarygroup 1304380 Feb 1 13:33 libc++abi-debug-ww-noexcept.a
-rw-r----- 1 aheejin primarygroup 1016138 Feb 1 13:33 libc++abi-except.a
-rw-r----- 1 aheejin primarygroup 951196 Feb 1 13:33 libc++abi-mt.a
-rw-r----- 1 aheejin primarygroup 1036278 Feb 1 13:34 libc++abi-mt-except.a
-rw-r----- 1 aheejin primarygroup 946236 Feb 1 13:33 libc++abi-mt-noexcept.a
-rw-r----- 1 aheejin primarygroup 930950 Feb 1 13:33 libc++abi-noexcept.a
-rw-r----- 1 aheejin primarygroup 951196 Feb 1 13:33 libc++abi-ww.a
-rw-r----- 1 aheejin primarygroup 1036278 Feb 1 13:34 libc++abi-ww-except.a
-rw-r----- 1 aheejin primarygroup 946236 Feb 1 13:33 libc++abi-ww-noexcept.a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be fine to increase the code size here. But some of the motivation for this new option was to get stack traces without the extra overhead and size of all of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the TODO. |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def get_cflags(self): | ||||||||||||||||||||||||||||||||
| cflags = super().get_cflags() | ||||||||||||||||||||||||||||||||
| if not self.is_mt and not self.is_ww: | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
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.
Hmm, I think this should be "This is set to true when
ASSERTIONSis enabled", because we print exception stack traces even when-sASSERTIONS -sEXCEPTION_STACK_TRACES=0. See #18642 (comment)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.
If we ban
ASSERTIONS=0 && EXCEPTION_TRACES=1as suggested in #18642 (comment), I think the current text is OK.