Skip to content

[EH] Make getCppExceptionThrownValue return thrown value #17157

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

Merged
merged 1 commit into from
Jun 8, 2022
Merged

[EH] Make getCppExceptionThrownValue return thrown value #17157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 12 additions & 9 deletions src/library_exceptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,29 +398,32 @@ var LibraryExceptions = {
return Module['asm']['__cpp_exception'];
},

$getCppExceptionThrownValue__deps: ['$getCppExceptionTag'],
$getCppExceptionThrownValue__deps: ['$getCppExceptionTag', '__thrown_object_from_unwind_exception'],
$getCppExceptionThrownValue: function(ex) {
Copy link
Collaborator

@sbc100 sbc100 Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this function getCppExceptionThrownObject? Or is thrown value a thing that is different?

Copy link
Member Author

@aheejin aheejin Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah these are confusing. So far in library_exceptions.js, I tried to use 'object' for WebAssembly.Exception object thrown, and 'value' for the user-thrown value. So this function takes an object and returns the user-thrown value. This got more confusing by the param name for this function is ex while other functions' param name is obj, when they denote the same thing.

At the same time libc++abi uses 'object' to mean the user-thrown value, as you can see in the name of thrown_object_from_unwind_exception, which adds to the confusion. It's hard to rename it because that function is copied from cxa_exception.cpp, and changing the function name will make our libc++abi more confusing. I guess we should make these terms clearer.

How about this?

  • object: the user-thrown value. So we rename this function to getCppExceptionThrownObject as you suggested.
  • value: let's not use this term at all
  • ex/exception: WebAssembly.Exception. So we rename current obj params in other functions, which meant WebAssembly.Exceptions, to ex.

If you think this sound fine, I'll make this change as a followup PR.

Copy link
Collaborator

@sbc100 sbc100 Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "JSObject" or "JSException"?

Copy link
Member Author

@aheejin aheejin Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the thing you want to represent with "JSObject" or "JSException"? WebAssembly.Exception or the user-thrown value(or object)? I guess the latter? In that case it's confusing in another way because this is a Wasm exception and that sounds like this is a JS exception thrown from JS, a foreign exception..

Copy link
Collaborator

@sbc100 sbc100 Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebAssembly.Exception is what I propose to refer to as JS object. The thrown object lives in wasm memory, as does the exception object, right? They are both allocated with malloc.

Copy link
Collaborator

@sbc100 sbc100 Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example a longer name for this function might be something like getCppThrownObjectFromJSCaughtObject?

Copy link
Member Author

@aheejin aheejin Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure JSCaughtObject is clearer to someone who doesn't know this context. How about just getCppThrownObjectFromWebassemblyException, with comments clarifying what this means? This name is long but I guess at least not misleading..

Copy link
Collaborator

@sbc100 sbc100 Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds reasonable. I think if was a user I would be looking for something do with the thing that I caught.. so I guess JSCaughtObject and WebassemblyException both describe that thing.

Copy link
Member Author

@aheejin aheejin Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll explain in the comment that this is WebAssembly.Exception object that Wasm EH JS API provides.

Copy link
Member Author

@aheejin aheejin Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, will do this in a followup. Thanks for the discussion!

return ex.getArg(getCppExceptionTag(), 0);
// In Wasm EH, the value extracted from WebAssembly.Exception is a pointer
// to the unwind header. Convert it to the actual thrown value.
var unwind_ex = ex.getArg(getCppExceptionTag(), 0);
return ___thrown_object_from_unwind_exception(unwind_ex);
},

$incrementExceptionRefcount__deps: ['__increment_wasm_exception_refcount', '$getCppExceptionThrownValue'],
$incrementExceptionRefcount__deps: ['__cxa_increment_exception_refcount', '$getCppExceptionThrownValue'],
$incrementExceptionRefcount: function(obj) {
var ptr = getCppExceptionThrownValue(obj);
___increment_wasm_exception_refcount(ptr);
___cxa_increment_exception_refcount(ptr);
},

$decrementExceptionRefcount__deps: ['__decrement_wasm_exception_refcount', '$getCppExceptionThrownValue'],
$decrementExceptionRefcount__deps: ['__cxa_decrement_exception_refcount', '$getCppExceptionThrownValue'],
$decrementExceptionRefcount: function(obj) {
var ptr = getCppExceptionThrownValue(obj);
___decrement_wasm_exception_refcount(ptr);
___cxa_decrement_exception_refcount(ptr);
},

$getExceptionMessage__deps: ['__get_exception_message', 'free', '$getCppExceptionThrownValue'],
$getExceptionMessage: function(ptr) {
$getExceptionMessage: function(obj) {
Copy link
Member Author

@aheejin aheejin Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by fix: the variable names (obj and ptr) were swapped in the previous code.

// In Wasm EH, the thrown object is a WebAssembly.Exception. Extract the
// thrown value from it.
var obj = getCppExceptionThrownValue(ptr);
var utf8_addr = ___get_exception_message(obj);
var ptr = getCppExceptionThrownValue(obj);
var utf8_addr = ___get_exception_message(ptr);
var result = UTF8ToString(utf8_addr);
_free(utf8_addr);
return result;
Expand Down
45 changes: 8 additions & 37 deletions system/lib/libcxxabi/src/cxa_exception_emscripten.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,27 +33,26 @@ thrown_object_from_cxa_exception(__cxa_exception* exception_header) {
// Get the exception object from the unwind pointer.
// Relies on the structure layout, where the unwind pointer is right in
// front of the user's exception object
static inline __cxa_exception* cxa_exception_from_exception_unwind_exception(
static inline __cxa_exception* cxa_exception_from_unwind_exception(
Copy link
Member Author

@aheejin aheejin Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was named cxa_exception_from_exception_unwind_exception because it was copied from cxa_exception.cpp. I'm not sure why we need to repeat exception, so I shortened the name. The same with the function below.

Copy link
Collaborator

@sbc100 sbc100 Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is more correct we should maybe propose this as an upstream change? I do think there is some value in matching these names with the upstream ones... but I don't feel strongly about it.

_Unwind_Exception* unwind_exception) {
return cxa_exception_from_thrown_object(unwind_exception + 1);
}

static inline void* thrown_object_from_exception_unwind_exception(
static inline void* thrown_object_from_unwind_exception(
_Unwind_Exception* unwind_exception) {
__cxa_exception* exception_header =
cxa_exception_from_exception_unwind_exception(unwind_exception);
cxa_exception_from_unwind_exception(unwind_exception);
return thrown_object_from_cxa_exception(exception_header);
}

extern "C" {

void* __thrown_object_from_unwind_exception(
_Unwind_Exception* unwind_exception) {
return thrown_object_from_unwind_exception(unwind_exception);
}

char* __get_exception_message(void* thrown_object, bool terminate=false) {
#if __USING_WASM_EXCEPTIONS__
// In Wasm EH, the thrown value is 'unwindHeader' field of __cxa_exception. So
// we need to get the real pointer thrown by user.
thrown_object = thrown_object_from_exception_unwind_exception(
static_cast<_Unwind_Exception*>(thrown_object));
#endif
__cxa_exception* exception_header =
cxa_exception_from_thrown_object(thrown_object);
const __shim_type_info* thrown_type =
Expand Down Expand Up @@ -95,34 +94,6 @@ char* __get_exception_terminate_message(void *thrown_object) {
return __get_exception_message(thrown_object, true);
}

#if __USING_WASM_EXCEPTIONS__
void __cxa_increment_exception_refcount(void* thrown_object) _NOEXCEPT;
void __cxa_decrement_exception_refcount(void* thrown_object) _NOEXCEPT;

// These are wrappers for __cxa_increment_exception_refcount and
// __cxa_decrement_exception_refcount so that these can be called from JS. When
// you catch a Wasm exception from JS and do not rethrow it, its refcount is
// still greater than 0 so memory is leaked; users call JS library functions
// that call these to fix it.

void __increment_wasm_exception_refcount(
void* unwind_exception) _NOEXCEPT {
// In Wasm EH, the thrown value is 'unwindHeader' field of __cxa_exception. So
// we need to get the real pointer thrown by user.
void* thrown_object = thrown_object_from_exception_unwind_exception(
static_cast<_Unwind_Exception*>(unwind_exception));
__cxa_increment_exception_refcount(thrown_object);
}

void __decrement_wasm_exception_refcount(
void* unwind_exception) _NOEXCEPT {
// In Wasm EH, the thrown value is 'unwindHeader' field of __cxa_exception. So
// we need to get the real pointer thrown by user.
void* thrown_object = thrown_object_from_exception_unwind_exception(
static_cast<_Unwind_Exception*>(unwind_exception));
__cxa_decrement_exception_refcount(thrown_object);
}
#endif // __USING_WASM_EXCEPTIONS__
}

#endif // __USING_EMSCRIPTEN_EXCEPTIONS__ || __USING_WASM_EXCEPTIONS__
2 changes: 1 addition & 1 deletion tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1640,7 +1640,7 @@ def test_exception_message(self):
self.set_setting('EXPORTED_FUNCTIONS', ['_main', 'getExceptionMessage', '___get_exception_message'])
if '-fwasm-exceptions' in self.emcc_args:
exports = self.get_setting('EXPORTED_FUNCTIONS')
self.set_setting('EXPORTED_FUNCTIONS', exports + ['___cpp_exception', '___increment_wasm_exception_refcount', '___decrement_wasm_exception_refcount'])
self.set_setting('EXPORTED_FUNCTIONS', exports + ['___cpp_exception', '___cxa_increment_exception_refcount', '___cxa_decrement_exception_refcount', '___thrown_object_from_unwind_exception'])

# FIXME Temporary workaround. See 'FIXME' in the test source code below for
# details.
Expand Down