Make getExceptionMessage work with EXCEPTION_STACK_TRACES in Emscripten EH #26050
Conversation
`getExceptionMessage` didn't work with `-sASSERTIONS`, which is the default mode for `-O0`, because `getExceptionMessage` assumes the input `ptr` is a pointer, whereas when `ASSERTIONS` is set, it is an instance of `CppException`. `in/decrementExceptionRefcount` have the same problem. Now these methods check whether the pointer is a value or an instance of `CppException`. Fixes emscripten-core#17115 (comment).
|
|
||
| @with_all_eh_sjlj | ||
| def test_EXPORT_EXCEPTION_HANDLING_HELPERS(self): | ||
| self.set_setting('ASSERTIONS', 0) |
There was a problem hiding this comment.
Better to just remove this line. No need for the additional change below since core tests run in O0 mode with assertions enabled already.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah right. Will remove it.
| if (ptr instanceof CppException) { | ||
| ptr = ptr.excPtr; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Seems a bit on that ptr might be something other than a pointer. Why not expect caller to always pass a pointer here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| char const*, | ||
| ''' | ||
|
|
||
| self.do_runf('main.cpp', expected) |
There was a problem hiding this comment.
I think you still need this line.
This previous commit deleted the test itself by mistake
src/lib/libexceptions.js
Outdated
| #elif !DISABLE_EXCEPTION_CATCHING | ||
| $incrementExceptionRefcount__deps: ['__cxa_increment_exception_refcount'], | ||
| $incrementExceptionRefcount: (ptr) => ___cxa_increment_exception_refcount(ptr), | ||
| $incrementExceptionRefcount: (ptr) => { |
There was a problem hiding this comment.
Can you rename the incoming ptr param to something more meaningful?
And maybe add a little helper to get the ptr from an exception?
e.g.
$incrementExceptionRefcount: (exn) => {
___cxa_increment_exception_refcount(exnToPtr(exn));
},
aheejin
changed the title
This factors out the pointer extraction logic to `exnToPtr`.
src/lib/libexceptions.js
Outdated
| $getExceptionMessage__deps: ['$exnToPtr', '$getExceptionMessageCommon'], | ||
| $getExceptionMessage: (exn) => { | ||
| return getExceptionMessageCommon(exnToPtr(exn)); | ||
| }, |
There was a problem hiding this comment.
These can be reverted to single line functions now I think.
Otherwise lgtm!
3 participants
getExceptionMessagedidn't work withEXCEPTION_STACK_TRACES, which is also set whenASSERTIONSis set, in Emscripten EH.ASSERTIONSis the default mode for-O0`.getExceptionMessageassumes the inputptris a pointer, but whenEXCEPTION_STACK_TRACESis set, it is an instance ofCppException.in/decrementExceptionRefcounthave the same problem. Now these methods check whether the pointer is a value or an instance ofCppException.In the test,
self.set_setting('ASSERTIONS', 0)is removed. That way, we can test witASSERTIONSset incore0and unset incore1+settings.Fixes #17115 (comment).