Skip to content

Make getExceptionMessage work with EXCEPTION_STACK_TRACES in Emscripten EH #26050

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
aheejin merged 10 commits into from
Jan 10, 2026
Merged

Make getExceptionMessage work with EXCEPTION_STACK_TRACES in Emscripten EH #26050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5f7aa28
Make getExceptionMessage work with ASSERTIONS
aheejin Jan 7, 2026
6115f33
Comment
aheejin Jan 9, 2026
20441a7
Merge branch 'main' into fix_eh_getmessage_assertions
aheejin Jan 9, 2026
f6989c3
Remove ASSERTIONS loop
aheejin Jan 9, 2026
8134865
Revive test
aheejin Jan 9, 2026
3a81a36
Merge branch 'main' into fix_eh_getmessage_assertions
aheejin Jan 9, 2026
b574c4a
Factor out exnToPtr
aheejin Jan 9, 2026
df4417b
Merge branch 'main' into fix_eh_getmessage_assertions
aheejin Jan 9, 2026
775e41d
typo fix
aheejin Jan 9, 2026
73f045a
Convert to single-line functions
aheejin Jan 9, 2026
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
23 changes: 17 additions & 6 deletions src/lib/libexceptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,14 +345,25 @@ var LibraryExceptions = {
},

#elif !DISABLE_EXCEPTION_CATCHING
$incrementExceptionRefcount__deps: ['__cxa_increment_exception_refcount'],
$incrementExceptionRefcount: (ptr) => ___cxa_increment_exception_refcount(ptr),
// When EXCEPTION_STACK_TRACES is set, the exception is an instance of
// CppException, whereas EXCEPTION_STACK_TRACES is unset it is a raw pointer.
$exnToPtr: (exn) => {
#if EXCEPTION_STACK_TRACES
if (exn instanceof CppException) {
return exn.excPtr;
}
#endif
Copy link
Collaborator

@sbc100 sbc100 Jan 8, 2026

Choose a reason for hiding this comment

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

Seems a bit on that ptr might be something other than a pointer. Why not expect caller to always pass a pointer here?

Copy link
Member Author

@aheejin aheejin Jan 9, 2026

Choose a reason for hiding this comment

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

Not sure if I understand. Do you mean, instead of doing the unwrapping here (and in getExceptionMessage and decrementExceptionRefcount), we should let the caller do the unwrapping? But the caller may be a user program:
https://github.com/emscripten-core/emscripten/blob/035c704d5cbeb85481eb1c3273617d21b881d674/test/test_core.py#L1553C19-L1553C45

And it can be confusing for the user to do different things depending on whether they are in ASSERTIONS mode or not.

Copy link
Collaborator

@sbc100 sbc100 Jan 9, 2026

Choose a reason for hiding this comment

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

But the user is already exposed to the difference aren't they? In one case they get a CppException object and in another case they get a pointer.

Is the idea that we are supposed to treat these two things the same a paper over the differences like this? It seems a little all. If we do want to do this consistently perhaps we should have some kind of helper/macro such as "caughtExceptionToPtr" or something like that?

Copy link
Member Author

@aheejin aheejin Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can think of this as we providing overloaded convenience methods for similar but different types. Making the users change code depending on whether they build with -O0 and -On will be at best very inconvenient.

return exn;
},

$incrementExceptionRefcount__deps: ['$exnToPtr', '__cxa_increment_exception_refcount'],
$incrementExceptionRefcount: (exn) => ___cxa_increment_exception_refcount(exnToPtr(exn)),

$decrementExceptionRefcount__deps: ['__cxa_decrement_exception_refcount'],
$decrementExceptionRefcount: (ptr) => ___cxa_decrement_exception_refcount(ptr),
$decrementExceptionRefcount__deps: ['$exnToPtr', '__cxa_decrement_exception_refcount'],
$decrementExceptionRefcount: (exn) => ___cxa_decrement_exception_refcount(exnToPtr(exn)),

$getExceptionMessage__deps: ['$getExceptionMessageCommon'],
$getExceptionMessage: (ptr) => getExceptionMessageCommon(ptr),
$getExceptionMessage__deps: ['$exnToPtr', '$getExceptionMessageCommon'],
$getExceptionMessage: (exn) => getExceptionMessageCommon(exnToPtr(exn)),

#endif
};
Expand Down
1 change: 0 additions & 1 deletion test/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,7 +1508,6 @@ def test_exceptions_rethrow_missing(self):

@with_all_eh_sjlj
def test_EXPORT_EXCEPTION_HANDLING_HELPERS(self):
self.set_setting('ASSERTIONS', 0)
Copy link
Collaborator

@sbc100 sbc100 Jan 8, 2026

Choose a reason for hiding this comment

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

Better to just remove this line. No need for the additional change below since core tests run in O0 mode with assertions enabled already.

Copy link
Member Author

@aheejin aheejin Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That may be sufficient for the testing of this PR, but then we don't get to test the case ASSERTIONS=0, when we don't use CppException wrapper, no?

In case it was not clear why we test both ASSERTIONS 0 and 1 at the bottom, I added some comments: 6115f33
Please let me know if you still think it's better to remove the loop.

Copy link
Collaborator

@sbc100 sbc100 Jan 9, 2026

Choose a reason for hiding this comment

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

But we already have the core0 and core1 configurations that test with ASSERTIONS=0 and ASSERTIONS=1. We shouldn't need to duplication that here, unless I'm missing something.

If there is a reason to test with ASSERTIONS=0 and ASSERTIONS=1 in all configurations then this should probably be @parameterized

Copy link
Member Author

@aheejin aheejin Jan 9, 2026

Choose a reason for hiding this comment

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

Ah right. Will remove it.

self.set_setting('EXPORT_EXCEPTION_HANDLING_HELPERS')

self.maybe_closure()
Expand Down