[EH] Support stack traces for Wasm exceptions #17979

aheejin · 2022-10-03T23:38:13Z

This embeds stack traces into WebAssembly.Exception objects when ASSERTIONS is set. To do this, we now have a separate debug version of libc++abi, whose __cxa_throw doesn't call libunwind's _Unwind_RaiseException, which uses Wasm's throw instruction directly to throw exceptions, but rather calls out to a helper JS function that creates and throws WebAssembly.Exception. That JS function uses the optional stack property of WebAssembly.Exception constructor to attach stack traces to objects.

Without ASSERTIONS set, when an exception is thrown and uncaught, we only see something like

exiting due to exception: [object WebAssembly.Exception]

And this is what we've seen so far as well.

With this patch, when ASSERTIONS is set, we see stack traces like

exiting due to exception: [object WebAssembly.Exception],Error
    at __cxa_throw (wasm://wasm/009a7c9a:wasm-function[1551]:0x24367)
    at bar() (wasm://wasm/009a7c9a:wasm-function[12]:0xf53)
    at foo() (wasm://wasm/009a7c9a:wasm-function[19]:0x154e)
    at __original_main (wasm://wasm/009a7c9a:wasm-function[20]:0x15a6)
    at main (wasm://wasm/009a7c9a:wasm-function[56]:0x25be)
    at test.js:833:22
    at callMain (test.js:4567:15)
    at doRun (test.js:4621:23)
    at run (test.js:4636:5)

Fixes #17466.

This embeds stack traces into `WebAssembly.Exception` objects when `ASSERTION` is set. To do this, we now have a separate debug version of libc++abi, whose `__cxa_throw` doesn't call libunwind's `_Unwind_RaiseException`, which uses Wasm's `throw` instruction directly to throw exceptions, but rather calls out to a helper JS function that creates and throws `WebAssembly.Exception`. That JS function uses the optional `stack` property of `WebAssembly.Exception` constructor to attach stack traces to objects. Without `ASSERTION` set, when an exception is thrown and uncaught, we only see something like ``` exiting due to exception: [object WebAssembly.Exception] ``` And this is what we've seen so far as well. With this patch, when `ASSERTION` is set, we see stack traces like ``` exiting due to exception: [object WebAssembly.Exception],Error at ___throwCppWebAssemblyException (test.js:1139:13) at __cxa_throw (wasm://wasm/009a7c9a:wasm-function[1551]:0x24367) at bar() (wasm://wasm/009a7c9a:wasm-function[12]:0xf53) at foo() (wasm://wasm/009a7c9a:wasm-function[19]:0x154e) at __original_main (wasm://wasm/009a7c9a:wasm-function[20]:0x15a6) at main (wasm://wasm/009a7c9a:wasm-function[56]:0x25be) at test.js:833:22 at callMain (test.js:4567:15) at doRun (test.js:4621:23) at run (test.js:4636:5) ``` Fixes emscripten-core#17466.

sbc100

Nice!

src/library_exceptions.js

sbc100 · 2022-10-03T23:52:45Z

system/lib/libcxxabi/src/cxa_exception.cpp

+#else
+    // In debug mode, call a JS library function to use WebAssembly.Exception JS
+    // API, which enables us to include stack traces
+    __throwCppWebAssemblyException(&exception_header->unwindHeader, true);


What about calling this _Unwind_RaiseException_WithStacktrace? Since its basically the same function as _Unwind_RaiseException? Maybe it doesn't need a second argument if its always called with true?

_Unwind_ functions are the naming convention of libunwind, and this function is in library_exceptions.js.. So I named it like this. On the other hand, library_exception.js's (or other library JS files') naming schemes are kind of mixed... Some are camelCase, some are snake_case, and some are with two underscores, some are with one underscore, some are without... So it was kind of not very easy to name it. Should it have leading underscores or not?

The reason I added the boolean option is in case users want to directly use that function to throw WebAssembly.Exception from JS, in which case they may or may not want the stack traces.

But we don't have such users yet do we? So maybe we can keep the code simple for now and make it more complex later if it proves useful, and also keep this function private/internal.

Also, a user who doesn't want a stacktrace could/should presumably just call the existing _Unwind_RaiseException function.

Hmm, given that we decided to use this as an internal function for the moment, will rename this to __throw_exception_with_stack_trace. (Not sure about JS library naming schemes, but it looks internal functions are likely to be in snake_case...) Let me know if you have a better idea.

src/library_exceptions.js

sbc100 · 2022-10-04T12:19:04Z

system/lib/libcxxabi/src/cxa_exception.cpp

+#else
+    // In debug mode, call a JS library function to use WebAssembly.Exception JS
+    // API, which enables us to include stack traces
+    __throwCppWebAssemblyException(&exception_header->unwindHeader, true);


But we don't have such users yet do we? So maybe we can keep the code simple for now and make it more complex later if it proves useful, and also keep this function private/internal.

Also, a user who doesn't want a stacktrace could/should presumably just call the existing _Unwind_RaiseException function.

sbc100 · 2022-10-04T12:20:25Z

src/library_exceptions.js

-  // Given an WebAssembly.Exception object, returns the actual user-thrown
-  // C++ object address in the Wasm memory.
+  // Throw a WebAssembly.Exception object with the C++ tag. If traceStack is
+  // true, includes the stack traces within the exception object.
  // WebAssembly.Exception is a JS object representing a Wasm exception,
  // provided by Wasm JS API:
  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Exception


Perhaps add something like. "In release builds this function is not needed and the native _Unwind_RaiseException is used instead"

To add to that, perhaps we can put the entire function within #if ASSERTIONS? That would both further clarify it is for debug builds, and also we'd get a link error if we try to use it by mistake in the future.

(Though from discussion below perhaps we want to allow this to be used optionally in release builds too. In that case ignore my comment here.)

@sbc100 Added the comment.
@kripken Put the #if ASSERTIONS guard around it.

kripken

Nice!

lgtm % ongoing discussions

kripken · 2022-10-04T17:15:31Z

src/library_exceptions.js

-  // Given an WebAssembly.Exception object, returns the actual user-thrown
-  // C++ object address in the Wasm memory.
+  // Throw a WebAssembly.Exception object with the C++ tag. If traceStack is
+  // true, includes the stack traces within the exception object.
  // WebAssembly.Exception is a JS object representing a Wasm exception,
  // provided by Wasm JS API:
  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Exception


To add to that, perhaps we can put the entire function within #if ASSERTIONS? That would both further clarify it is for debug builds, and also we'd get a link error if we try to use it by mistake in the future.

(Though from discussion below perhaps we want to allow this to be used optionally in release builds too. In that case ignore my comment here.)

kripken · 2022-10-04T17:21:18Z

The docs here are worth updating after this: (doesn't need to be in this PR)

https://emscripten.org/docs/porting/exceptions.html

In two-phase unwinding, the first phase is the search phase (`_UA_SEARCH_PHASE`) and the second one is the cleanup phase (`_UA_CLEANUP_PHASE`). The search phase searches up the stack to see if there is a matching catch handler, and if it finds one, it caches the result. And in the second cleanup phase, it retrieves the cached result (to avoid doing same work twice) and unwinds the stack. Wasm does not do the two-phase unwinding; it only has a single phase. We used `_UA_CLEANUP_PHASE` for this single phase, so in Wasm the cleanup phase is supposed to the search. So we several many custom `#ifdef`s to use the code guarded by `_UA_SEARCH_PHASE`, for example: https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L771-L776 https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L850-L855 These are apparently gone in emscripten-core#14288, which replaced many `if`s with `assert`s. This in effect removed our special handling for `_UA_CLEANUP_PHASE`; there are several `assert`s that asserts the current phase is `_UA_SEARCH_PHASE`, while Wasm is in `_UA_CLEANUP_PHASE`. But this has not caused problems so far because we have built libc++abi with `-NDEBUG`, so all assertions were no-op. https://github.com/emscripten-core/emscripten/blob/40fb7d2071e439f1de614898b88518df582faa94/tools/system_libs.py#L1366 But this is now a problem because emscripten-core#17979 adds a debug build of libc++abi, which enables assertions. Come to think of it, I'm not sure why I decided to use `_UA_CLEANUP_PHASE` for our single phase in the first place. If we use `_UA_SEARCH_PHASE`, we can remove more our custom code and reduce the difference between our port and the upstream library.

In two-phase unwinding, the first phase is the search phase (`_UA_SEARCH_PHASE`) and the second one is the cleanup phase (`_UA_CLEANUP_PHASE`). The search phase searches up the stack to see if there is a matching catch handler, and if it finds one, it caches the result. And in the second cleanup phase, it retrieves the cached result (to avoid doing same work twice) and unwinds the stack. Wasm does not do the two-phase unwinding; it only has a single phase. We used `_UA_CLEANUP_PHASE` for this single phase, so in Wasm the cleanup phase is supposed to the search. So we several many custom `#ifdef`s to use the code guarded by `_UA_SEARCH_PHASE`, for example: https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L771-L776 https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_personality.cpp#L850-L855 These are apparently gone in #14288, which replaced many `if`s with `assert`s. This in effect removed our special handling for `_UA_CLEANUP_PHASE`; there are several `assert`s that asserts the current phase is `_UA_SEARCH_PHASE`, while Wasm is in `_UA_CLEANUP_PHASE`. But this has not caused problems so far because we have built libc++abi with `-NDEBUG`, so all assertions were no-op. https://github.com/emscripten-core/emscripten/blob/40fb7d2071e439f1de614898b88518df582faa94/tools/system_libs.py#L1366 But this is now a problem because #17979 adds a debug build of libc++abi, which enables assertions. Come to think of it, I'm not sure why I decided to use `_UA_CLEANUP_PHASE` for our single phase in the first place. If we use `_UA_SEARCH_PHASE`, we can remove more our custom code and reduce the difference between our port and the upstream library.

After emscripten-core#17979, when `ASSERTIONS` is set, uncaught exceptions carry stack traces that are printed to the screen for debugging help. This adds an exception message to the exception objects in addition to that. The message it adds is produced by `__get_exception_message` function: https://github.com/emscripten-core/emscripten/blob/f6c46570e3780e52050bf822a07b342ec4bdddbe/system/lib/libcxxabi/src/cxa_exception_emscripten.cpp#L75-L111 If an exception is a subclass of `std::exception`, it returns `std::exception::what()`. If not, it just prints its type. If an exception is uncaught and its type is a subclass of `std::exception`, now we print the `what()` message along with the stack trace when `ASSERTION` is set. In case our exception is `std::runtime_error` and the message is "my exception", when uncaught, this prints: ``` exiting due to exception: [object WebAssembly.Exception],Error: std::runtime_error,my exception at __cxa_throw (wasm://wasm/009a7c9a:wasm-function[1551]:0x24367) ... ``` Fixes emscripten-core#6330.

After #17979, when `ASSERTIONS` is set, uncaught exceptions carry stack traces that are printed to the screen for debugging help. This adds an exception message to the exception objects in addition to that. The message it adds is produced by `__get_exception_message` function: https://github.com/emscripten-core/emscripten/blob/f6c46570e3780e52050bf822a07b342ec4bdddbe/system/lib/libcxxabi/src/cxa_exception_emscripten.cpp#L75-L111 If an exception is a subclass of `std::exception`, it returns `std::exception::what()`. If not, it just prints its type. If an exception is uncaught and its type is a subclass of `std::exception`, now we print the `what()` message along with the stack trace when `ASSERTIONS` is set. In case our exception is `std::runtime_error` and the message is "my exception", when uncaught, this prints: ``` exiting due to exception: [object WebAssembly.Exception],Error: std::runtime_error,my exception at __cxa_throw (wasm://wasm/009a7c9a:wasm-function[1551]:0x24367) ... ``` Fixes #6330.

aheejin requested review from kripken and sbc100 October 3, 2022 23:38

sbc100 approved these changes Oct 3, 2022

View reviewed changes

aheejin added 3 commits October 4, 2022 00:35

Remove JS function name from stack trace

1b16d80

Fix comment too

0cfb81d

Merge branch 'main' into wasmeh_stack_trace

42e7991

sbc100 reviewed Oct 4, 2022

View reviewed changes

kripken approved these changes Oct 4, 2022

View reviewed changes

Address comments

a4fab86

sbc100 approved these changes Oct 4, 2022

View reviewed changes

Typo fix

842ef0f

kripken approved these changes Oct 4, 2022

View reviewed changes

aheejin mentioned this pull request Oct 5, 2022

[EH] Use _UA_SEARCH_PHASE in personality function (NFC) #17991

Merged

aheejin added 8 commits October 5, 2022 15:15

Merge branch 'main' into wasmeh_stack_trace

79ae2e5

Add libc++abi-debug-noexcept to MINIMAL_TASKS

6449f7d

Add libc++abi-debug to MINIMAL_TASKS

2c165e1

ChangeLog

007130a

Fix dynamic linking

34e346d

Add libc++abi-debug-mt-noexcept to MINIMAL_PIC_TASKS

eb8ad2a

Merge branch 'main' into wasmeh_stack_trace

16d4c00

Add libc++abi-debug-mt to MINIMAL_PIC_TASKS

f9654ea

aheejin merged commit 3e53d5c into emscripten-core:main Oct 6, 2022

aheejin deleted the wasmeh_stack_trace branch October 6, 2022 06:28

aheejin mentioned this pull request Oct 6, 2022

[EH] Print message for uncaught exceptions #18003

Merged

aheejin mentioned this pull request Feb 1, 2023

[EH] Add EXCEPTION_STACK_TRACES option #18642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[EH] Support stack traces for Wasm exceptions #17979

[EH] Support stack traces for Wasm exceptions #17979

Uh oh!

aheejin commented Oct 3, 2022 •

edited

Loading

Uh oh!

sbc100 left a comment

Uh oh!

Uh oh!

sbc100 Oct 3, 2022

Uh oh!

aheejin Oct 4, 2022

Uh oh!

sbc100 Oct 4, 2022

Uh oh!

aheejin Oct 4, 2022

Uh oh!

Uh oh!

sbc100 Oct 4, 2022

Uh oh!

sbc100 Oct 4, 2022

Uh oh!

kripken Oct 4, 2022

Uh oh!

aheejin Oct 4, 2022

Uh oh!

kripken left a comment

Uh oh!

kripken Oct 4, 2022

Uh oh!

kripken commented Oct 4, 2022

Uh oh!

Uh oh!

[EH] Support stack traces for Wasm exceptions #17979

[EH] Support stack traces for Wasm exceptions #17979

Uh oh!

Conversation

aheejin commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Oct 4, 2022

Uh oh!

Uh oh!

aheejin commented Oct 3, 2022 •

edited

Loading