-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[embind] Manage exceptions' states correctly #26519
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
96d7b56
c83c7a0
2b32153
0172b72
ccd8094
d122a1d
1d0c028
d8b4741
6400263
7cb05fb
8d75eb0
10866ac
cb69eab
8ddb43b
c808c40
0139122
1712f3b
074cb6e
fa085a0
b3a57f5
c4724df
f8c4137
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 |
|---|---|---|
|
|
@@ -11,6 +11,9 @@ | |
| var LibraryEmVal = { | ||
| // Stack of handles available for reuse. | ||
| $emval_freelist: [], | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| $emval_exception_decrefs: [], | ||
| #endif | ||
| // Array of alternating pairs (value, refcount). | ||
| // reserve 0 and some special values. These never get de-allocated. | ||
| $emval_handles: [ | ||
|
|
@@ -80,13 +83,27 @@ var LibraryEmVal = { | |
| } | ||
| }, | ||
|
|
||
| _emval_decref__deps: ['$emval_freelist', '$emval_handles'], | ||
| _emval_decref__deps: ['$emval_freelist', '$emval_handles', | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| '$emval_exception_decrefs', | ||
| #endif | ||
| ], | ||
| _emval_decref: (handle) => { | ||
| if (handle > {{{ EMVAL_LAST_RESERVED_HANDLE }}} && 0 === --emval_handles[handle + 1]) { | ||
| #if ASSERTIONS | ||
| assert(emval_handles[handle] !== undefined, `Decref for unallocated handle.`); | ||
| #endif | ||
| var value = emval_handles[handle]; | ||
| emval_handles[handle] = undefined; | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| // In case the value is a C++ exception, decrement the refcount, so the | ||
| // memory can be freed correctly | ||
| var destructor = emval_exception_decrefs[handle]; | ||
| if (destructor) { | ||
| emval_exception_decrefs[handle] = undefined; | ||
| destructor(value); | ||
| } | ||
| #endif | ||
| emval_freelist.push(handle); | ||
| } | ||
| }, | ||
|
|
@@ -392,18 +409,40 @@ ${functionBody} | |
| return delete object[property]; | ||
| }, | ||
|
|
||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| $isCppExceptionObject__deps: ['$Emval'], | ||
| $isCppExceptionObject: (object) => { | ||
| #if !DISABLE_EXCEPTION_CATCHING | ||
| return object instanceof CppException; | ||
| #else // WASM_EXCEPTIONS | ||
| return object instanceof WebAssembly.Exception; | ||
| #endif | ||
| }, | ||
| #endif | ||
|
|
||
| _emval_throw__deps: ['$Emval', | ||
| #if !WASM_EXCEPTIONS && !DISABLE_EXCEPTION_CATCHING | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| #if !DISABLE_EXCEPTION_CATCHING | ||
| '$exceptionLast', | ||
| '$ExceptionInfo', | ||
| #endif | ||
| '$incrementExceptionRefcount', | ||
| '$incrementUncaughtExceptionCount', | ||
| '$isCppExceptionObject', | ||
| #endif | ||
| ], | ||
| _emval_throw: (object) => { | ||
| object = Emval.toValue(object); | ||
| #if !WASM_EXCEPTIONS && !DISABLE_EXCEPTION_CATCHING | ||
| // If we are throwing Emcripten C++ exception, set exceptionLast, as we do | ||
| // in __cxa_throw. C++ exception will be an instance of CppEmscripten. | ||
| if (object instanceof CppException) { | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| if (isCppExceptionObject(object)) { | ||
| #if !DISABLE_EXCEPTION_CATCHING | ||
| var info = new ExceptionInfo(object.excPtr); | ||
| info.set_caught(false); | ||
| info.set_rethrown(false); | ||
| exceptionLast = object; | ||
| #endif | ||
| incrementUncaughtExceptionCount(); | ||
| incrementExceptionRefcount(object); | ||
| } | ||
|
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 think Then you can remove the Sorry to keep going back and forth on this but it seems like could simplify that code a bunch maybe? Otherwise LGTM!
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. |
||
| #endif | ||
| throw object; | ||
|
|
@@ -446,7 +485,15 @@ ${functionBody} | |
| })); | ||
| }, | ||
|
|
||
| _emval_from_current_cxa_exception__deps: ['$Emval', '__cxa_rethrow'], | ||
| _emval_from_current_cxa_exception__deps: ['$Emval', '__cxa_rethrow', | ||
| #if !DISABLE_EXCEPTION_THROWING || WASM_EXCEPTIONS | ||
| '$decrementUncaughtExceptionCount', | ||
| #endif | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| '$decrementExceptionRefcount', | ||
| '$emval_exception_decrefs', | ||
| #endif | ||
| ], | ||
| _emval_from_current_cxa_exception: () => { | ||
| try { | ||
| // Use __cxa_rethrow which already has mechanism for generating | ||
|
|
@@ -455,7 +502,16 @@ ${functionBody} | |
| // with metadata optimised out otherwise. | ||
| ___cxa_rethrow(); | ||
| } catch (e) { | ||
| return Emval.toHandle(e); | ||
| #if !DISABLE_EXCEPTION_THROWING || WASM_EXCEPTIONS | ||
| // ___cxa_rethrow incremented uncaughtExceptionCount. | ||
| // Since we caught it in JS, we need to manually decrement it to balance. | ||
| decrementUncaughtExceptionCount(); | ||
| #endif | ||
| var handle = Emval.toHandle(e); | ||
| #if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONS | ||
| emval_exception_decrefs[handle] = decrementExceptionRefcount; | ||
| #endif | ||
| return handle; | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| #include <emscripten/val.h> | ||
| #include <exception> | ||
| #include <iostream> | ||
|
|
||
| using namespace emscripten; | ||
|
|
||
| int main() { | ||
| val error = val::null(); | ||
| try { | ||
| throw std::runtime_error("test exception"); | ||
| } catch (const std::exception& e) { | ||
| // Capture the exception | ||
| error = val::take_ownership( | ||
| emscripten::internal::_emval_from_current_cxa_exception()); | ||
| } | ||
|
|
||
| std::cout << "Captured exception." << std::endl; | ||
|
|
||
| int uncaught_before = std::uncaught_exceptions(); | ||
| std::cout << "Uncaught before throw 1: " << uncaught_before << std::endl; | ||
|
|
||
| // First throw | ||
| try { | ||
| std::cout << "Throwing 1..." << std::endl; | ||
| error.throw_(); | ||
| } catch (const std::exception& e) { | ||
| std::cout << "Caught 1: " << e.what() << std::endl; | ||
| int uncaught_during = std::uncaught_exceptions(); | ||
| std::cout << "Uncaught during catch 1: " << uncaught_during << std::endl; | ||
| } | ||
|
|
||
| int uncaught_between = std::uncaught_exceptions(); | ||
| std::cout << "Uncaught between throws: " << uncaught_between << std::endl; | ||
|
|
||
| // Second throw - if refcount was messed up, this might fail/crash | ||
| try { | ||
| std::cout << "Throwing 2..." << std::endl; | ||
| error.throw_(); | ||
| } catch (const std::exception& e) { | ||
| std::cout << "Caught 2: " << e.what() << std::endl; | ||
| int uncaught_during = std::uncaught_exceptions(); | ||
| std::cout << "Uncaught during catch 2: " << uncaught_during << std::endl; | ||
| } | ||
|
|
||
| std::cout << "Done." << std::endl; | ||
| return 0; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| Captured exception. | ||
| Uncaught before throw 1: 0 | ||
| Throwing 1... | ||
| Caught 1: test exception | ||
| Uncaught during catch 1: 0 | ||
| Uncaught between throws: 0 | ||
| Throwing 2... | ||
| Caught 2: test exception | ||
| Uncaught during catch 2: 0 | ||
| Done. |
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.
Wrap this new code in
#if !DISABLE_EXCEPTION_THROWING || WASM_EXCEPTIONS?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.
It's really confusing to which one to wrap in
#if !DISABLE_EXCEPTION_THROWING || WASM_EXCEPTIONSand which one to wrap in#if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONSin this file. Apparently we only enable refcounting whenEXCEPTION_STACK_TRACESis true (which implies!DISABLE_EXCEPTION_CATCHINGso we should be using it here too?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.
Wrapped in
#if !DISABLE_EXCEPTION_CATCHING || WASM_EXCEPTIONSfor now, because in Emscripten EH refcounting requires!DISABLE_EXCEPTION_CATCHING