[EH] Add EXCEPTION_STACK_TRACES option #18642

aheejin · 2023-02-01T00:49:21Z

This adds EXCEPTION_STACK_TRACES option, which embeds stack traces into exception objects even when ASSERTIONS is not set. This is for the users who wants exception stack traces but don't want to incur the full overhead of ASSERTIONS. Exception stack traces are enabled when either of ASSERTIONS or EXCEPTION_STACK_TRACES is true.

This currently works only for Wasm EH. The reason this option's name is not WASM_EXCEPTION_STACK_TRACES is I think this can work for Emscripten EH later if something like #18535 lands.

Fixes #18533.

cc @ravisumit33

This adds `EXCEPTION_STACK_TRACES` option, which embeds stack traces into exception objects even when `ASSERTIONS` is not set. This is for the users who wants exception stack traces but don't want to incur the full overhead of `ASSERTIONS`. Exception stack traces are enabled when either of `ASSERTIONS` or `EXCEPTION_STACK_TRACES` is true. This currently works only for Wasm EH. The reason this option's name is not `WASM_EXCEPTION_STACK_TRACES` is I think this can work for Emscripten EH later if something like emscripten-core#18535 lands. Fixes emscripten-core#18533.

sbc100 · 2023-02-01T00:50:59Z

ChangeLog.md

 -----------------------
+- In Wasm exception mode (`-fwasm-exceptions`), when
+  `EXCEPTION_STACK_TRACES` is enabled, uncaught exceptions will display stack
+  traces. Note that stack traces are enabled when either of `ASSERTIONS` or this


How about "This defaults to true when ASSERTIONS are enabled".

Done. Not sure if I should use 'are' or 'is' for ASSERTIONS, because it is an option name.. (Used 'is' for now. Let me know if that doesn't sound right)

sbc100 · 2023-02-01T00:52:23Z

emcc.py

+  # 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 or settings.EXCEPTION_STACK_TRACES) and \
+     settings.WASM_EXCEPTIONS:


Instead of doing this can we do:

if settings.ASSERTIONS: default_setting('EXCEPTION_STACK_TRACES', 1)

Then we can revert the other places in this PR that check both.

(I posted and deleted several comments because they were not very well thought out)

I think adding this is fine:

if settings.ASSERTIONS: default_setting('EXCEPTION_STACK_TRACES', 1)

And this can change a few places in the JS library where we do

#if ASSERTIONS || EXCEPTION_STACK_TRACES

to simply

#if EXCEPTION_STACK_TRACES

But this doesn't let me delete this

if (settings.ASSERTIONS or settings.EXCEPTION_STACK_TRACES) and \ settings.WASM_EXCEPTIONS:

because setting EXCEPTION_STACK_TRACES doesn't set ASSERTIONS. And the point of this PR is we display stack traces even when ASSERTIONS is not set.

How about just: if settings.EXCEPTION_STACK_TRACES and settings.WASM_EXCEPTIONS:?

Or are the two lines below also needed when ASSERTIONS are enabled on their own? (Why?)

About this:

if settings.ASSERTIONS: default_setting('EXCEPTION_STACK_TRACES', 1)

I think the second line should be

settings.EXCEPTION_STACK_TRACES = 1

because we have to also override user settings. For example,

emcc -fwasm-exceptions -sASSERTIONS -sEXCEPTION_STACK_TRACES=0 test.cpp

should also print stack traces. I thought about not printing stack traces in this case, but having one more combination of behaviors (assertions enabled but exception stack traces disabled) seems harder to manage and really has no point.

How about just: if settings.EXCEPTION_STACK_TRACES and settings.WASM_EXCEPTIONS:?

I think this works fine. Now the code is:

if settings.ASSERTIONS: settings.EXCEPTION_STACK_TRACES = 1 if settings.EXCEPTION_STACK_TRACES and settings.WASM_EXCEPTIONS: ...

Also I was able to change the condition in a few JS libraries to simply

#if EXCEPTION_STACK_TRACES

Or are the two lines below also needed when ASSERTIONS are enabled on their own? (Why?)

Not sure which two lines below you are talking about?

sbc100 · 2023-02-01T00:53:25Z

tools/system_libs.py


+  def __init__(self, **kwargs):
+    super().__init__(**kwargs)
+    self.is_debug |= settings.EXCEPTION_STACK_TRACES


Can also revert this I think?

I don't think so? The point of this PR is to display stack traces when ASSERTIONS is not set but EXXCEPTION_STACK_TRACES is set, and

if settings.ASSERTIONS: default_setting('EXCEPTION_STACK_TRACES', 1)

This doesn't make ASSERTIONS set.
Related: https://github.com/emscripten-core/emscripten/pull/18642/files#r1092668280

I see, yes I agree then. Can you add a comment here maybe" "EXCEPTION_STACK_TRACES currently requires the debug version of libc++" ... BTW why is that? Why do we need the debug version here?

Because of this:

emscripten/system/lib/libcxxabi/src/cxa_exception.cpp

Lines 290 to 297 in 6ac4b2a

#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);

#endif

We could've come up with a separate preprocessor define keyword probably, but I thought this was simpler. Also enabling the debug version of libc++abi had its own advantages other than the stack traces, for example, it shows abort error messages, which the release version doesn't.

Could we instead remove __throw_exception_with_stack_trace and have _Unwind_RaiseException modify its behaviour based on EXCEPTION_STACK_TRACES in JS code?

Or is _Unwind_RaiseException maybe not a JS function here?

Added the comment.

Could we instead remove __throw_exception_with_stack_trace and have _Unwind_RaiseException modify its behaviour based on EXCEPTION_STACK_TRACES in JS code?

Or is _Unwind_RaiseException maybe not a JS function here?

_Unwind_RaiseException is a function in libunwind:

emscripten/system/lib/libunwind/src/Unwind-wasm.c

Lines 50 to 56 in 36c194d

/// Called by __cxa_throw.

_LIBUNWIND_EXPORT _Unwind_Reason_Code

_Unwind_RaiseException(_Unwind_Exception *exception_object) {

_LIBUNWIND_TRACE_API("_Unwind_RaiseException(exception_object=%p)",

(void *)exception_object);

__builtin_wasm_throw(0, exception_object);

}

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 Unwind-wasm.c is mostly a stub to call wasm instruction builtins, I feel libc++abi is a better place for calling out to JS. Also I like having the debug version of libc++abi for other reasons, such as users can see the abort error messages now, without which the errors could be very confusing and there was no way to build libc++abi in debug mode, even when ASSERTIONS was on.

sbc100 · 2023-02-01T00:55:49Z

test/test_other.py

+  # 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


I'm note sure its useful but we now have ways to checking the node version used in testing (looks for shared.check_node_version in test/common.py) and ways of running them (test-node-compat in `.circleci/config.yml``). Maybe not worth the effort to make that work for this case?

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 @requires_wasm_eh as is and leave this comment for future reference, because our CI falls back to v8 and has no problem currently, and deal with the error when we update the node, but the person who's doing the update will still be confused.

We now also run some tests of the very latest node tip of tree (see test-node-compat).

Just FYI in case you want to run some wasm EH under node too... we can now.

I tested with several node versions:

Node 14.18.2 (Our default): No support of Wasm EH

Node 17.5: Hasn't implemented traceStack, so always embeds stack traces. Prints stack traces when thrown and uncaught.

Node 18.13 (LTS as of now): Same with 17.5

Node 19.5 (Current as of now): Has implemented traceStack. But now it doesn't show stack traces when exceptions are thrown and uncaught... Not sure why.

Given this a little confusing situation, I'm tempted to use only V8 for this test for now...

Cherry-picks a couple of changes from PR emscripten-core#18642.

kripken · 2023-02-01T18:42:12Z

tools/system_libs.py


+  def __init__(self, **kwargs):
+    super().__init__(**kwargs)
+    self.is_debug |= settings.EXCEPTION_STACK_TRACES


Using the debug version will affect NDEBUG. Maybe for libcxxabi that's not significant, though? But I think it's worth measuring code size just to make sure.

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

We already use the debug version of libc++abi when ASSERTIONS is set. This was enabled in #17979. Having a debug version of libc++abi also has other advantages, such as, it shows abort error messages, which the release version doesn't. I don't think people would mind much about the small code increase when they set EXCEPTION_STACK_TRACES to true..? We also discussed whether enabling the debug version of libc++abi was worth it last year in our team chat and decided it was.

If we want to avoid the code size increase for libc++abi when ASSERTIONS is not set but EXCEPTION_STACK_TRACES is set, I think we need to come up with a separate preprocessor keyword for the code snippet included in https://github.com/emscripten-core/emscripten/pull/18642/files#r1093681153.

FWIW, the code sizes for wasm32-emscripten in my emscripten cache are:

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

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 ASSERTIONS. Anyhow, this is still a big improvement over ASSERTIONS so I'd be ok with it with a TODO to investigate further optimizations in system_libs.py, if we find the need for that later. (From the .a files the debug version does look larger, but that might not matter after linking or in comparison to other libraries I guess.)

Added the TODO.

aheejin · 2023-02-01T20:05:50Z

ChangeLog.md

 -----------------------
+- In Wasm exception mode (`-fwasm-exceptions`), when
+  `EXCEPTION_STACK_TRACES` is enabled, uncaught exceptions will display stack
+  traces. This defaults to true when `ASSERTIONS` is enabled. This option is


Hmm, I think this should be "This is set to true when ASSERTIONS is enabled", because we print exception stack traces even when -sASSERTIONS -sEXCEPTION_STACK_TRACES=0. See #18642 (comment)

If we ban ASSERTIONS=0 && EXCEPTION_TRACES=1 as suggested in #18642 (comment), I think the current text is OK.

sbc100 · 2023-02-01T20:10:58Z

emcc.py

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


I would have thought ASSERTIONS and EXCEPTION_STACK_TRACES could now be considered completely authogonal, and the -sASSERTIONS=1 -sEXCEPTION_STACK_TRACES=0 would "just work". But if that is too tricky or annoying implement when we should do this here to avoid any confusion:

if 'EXCEPTION_STACK_TRACES' in user_settings and not settings.EXCEPTION_STACK_TRACES: exit_with_error('EXCEPTION_STACK_TRACES cannot be disabled when ASSSERTIONS are enabled')

My guess is that adding this error and testing for it might be more complex than just supporting that config?

I'm not sure who would want ASSERTIONS but not exception stack traces. Because ASSERTIONS builds libc++abi in debug mode anyway, it wouldn't save any code size. I don't think it will be difficult or tricky, but I wouldn't want one more combination to care for, especially when I'm not sure who would want that... I like the exiting with error you suggested better.

I banned the combination and added a test for that. (I tested all four combinations)

emcc.py

Co-authored-by: Alon Zakai <[email protected]>

aheejin requested review from kripken and sbc100 February 1, 2023 00:49

sbc100 reviewed Feb 1, 2023

View reviewed changes

kleisauke added a commit to kleisauke/emscripten that referenced this pull request Feb 1, 2023

Add logic behind the EXCEPTION_STACK_TRACES option

9b50448

Cherry-picks a couple of changes from PR emscripten-core#18642.

kripken reviewed Feb 1, 2023

View reviewed changes

Address comments

14035e9

aheejin commented Feb 1, 2023

View reviewed changes

sbc100 reviewed Feb 1, 2023

View reviewed changes

Comment

5859ff8

kripken approved these changes Feb 2, 2023

View reviewed changes

Ban the combination + test + TODO

b7e0436

kripken approved these changes Feb 2, 2023

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

Update emcc.py

446981d

Co-authored-by: Alon Zakai <[email protected]>

sbc100 approved these changes Feb 2, 2023

View reviewed changes

Typo fix

fa96c39

aheejin merged commit 3d0ff6e into emscripten-core:main Feb 2, 2023

aheejin deleted the wasm_eh_stacktrace branch February 2, 2023 22:00

kleisauke mentioned this pull request Feb 3, 2023

Support the EXCEPTION_STACK_TRACES option for Emscripten EH #18535

Merged

aheejin mentioned this pull request Feb 13, 2023

[EH] Tidy up EH stack trace and message options #18696

Merged

	#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);
	#endif

	/// Called by __cxa_throw.
	_LIBUNWIND_EXPORT _Unwind_Reason_Code
	_Unwind_RaiseException(_Unwind_Exception *exception_object) {
	_LIBUNWIND_TRACE_API("_Unwind_RaiseException(exception_object=%p)",
	(void *)exception_object);
	__builtin_wasm_throw(0, exception_object);
	}

[EH] Add EXCEPTION_STACK_TRACES option #18642

[EH] Add EXCEPTION_STACK_TRACES option #18642

Uh oh!

Conversation

aheejin commented Feb 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aheejin Feb 1, 2023 •

edited

Loading

aheejin Feb 1, 2023 •

edited

Loading