Closed qmfrederik closed 6 months ago
qmfrederik
commented
6 months ago Current status:
objc_setUncaughtExceptionHandler is not implemented
davidchisnall
commented
6 months ago I presume that we can use the implementation from here on MinGW for the first point?
qmfrederik
commented
6 months ago OK, I've added a dummy implementation of objc_setUncaughtExceptionHandler. I assume we'll need a different implementation for _objc_unhandled_exception_filter?
A lot of tests are currently failing like this:
terminate called after throwing an instance of '@id'
For example:
# ./Test/ExceptionTest.exe
terminate called after throwing an instance of '@id'Any thoughts on what could cause that?
davidchisnall
commented
6 months ago Can you trace into the C++ personality function? If clang is correctly generating the LSDA, then this should work. Actually, try sticking a breakpoint on:
And:
These should be being called by the C++ personality function to check for matches.
The error message you're seeing is the one that happens if the C++ runtime throws an exception and nothing catches it (unwind reaches the end of the stack).
qmfrederik
commented
6 months ago From https://github.com/gnustep/libobjc2/pull/260#issuecomment-1878568187
The try/catch clause in objc_msgSend.m seems to be working, although the assert assert((TestCls == e) && "Exceptions propagate out of +initialize"); is failing; if I disable that assert the test passes.
Thanks! Looks like we're getting close. Propagating out of +initialize requires that they're thrown through C frames with attribute((cleanup)). Can you check whether that works with MinGW with just C and C++ (call C++ -> C -> C++, use attribute((cleanup)) in C, check you can throw an exception out)? It's possible that we need to add an extra compiler flag to the compilation unit that handles initialize.
It's a bit surprising to me that the exception seems to be thrown and code continues executing.
This doesn't reproduce after rebuilding, but leaving the comment here for future reference.
qmfrederik
commented
6 months ago I added additional logging and this is the current output (contradicting my previous comment):
vagrant@DESKTOP-RNTVKUC MINGW64 ~/git/libobjc2/build
# ./Test/objc_msgSend.exe
gnustep::libobjc::__objc_id_type_info::__do_catch: Entering
Assertion failed: (TestCls == e) && "Exceptions propagate out of +initialize", file C:/tools/msys64/home/vagrant/git/libobjc2/Test/objc_msgSend.m, line 186
vagrant@DESKTOP-RNTVKUC MINGW64 ~/git/libobjc2/build
# ./Test/ExceptionTest.exe
gnustep::libobjc::__objc_id_type_info::__do_catch: Entering
Assertion failed: object_getClass(x) == [Test class], file C:/tools/msys64/home/vagrant/git/libobjc2/Test/ExceptionTest.m, line 40Can you step through the do-catch methods and see where the match is failing?
qmfrederik
commented
6 months ago OK - some progress. The actual exception object wasn't being copied in objc_exception_throw - that should now be implemented, see https://github.com/gnustep/libobjc2/pull/267/files#diff-5803607d1fc7fe6f76ab1f6e98c4c6e5b373acb8bb6c103cd3fccaef42abd215R527-R528
With that, most tests seem to pass; only ExceptionTest and UnexpectedException fail:
94% tests passed, 10 tests failed out of 178
Total Test time (real) = 26.78 sec
The following tests FAILED:
3 - alias_legacy (Failed)
4 - alias_legacy_optimised (Failed)
35 - ExceptionTest (Failed)
36 - ExceptionTest_optimised (Failed)
37 - ExceptionTest_legacy (Failed)
38 - ExceptionTest_legacy_optimised (Failed)
155 - UnexpectedException (Failed)
156 - UnexpectedException_optimised (Failed)
157 - UnexpectedException_legacy (Failed)
158 - UnexpectedException_legacy_optimised (Failed)w.r.t. ExceptionTest failing, it looks like this may be related to rethrowing an exception
Here's the output (with a bunch of additional logging statements):
throw: Throwing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0x5465eea8 of type Test
gnustep::libobjc::__objc_id_type_info::__do_catch: Entering
gnustep::libobjc::__objc_id_type_info::__do_catch: Type info @id
gnustep::libobjc::__objc_id_type_info::__do_catch: got id type info for 0x5465eea8, returning true
eh_cleanup: Releasing exception 0x5465eea8
rethrow_id: Rethrowing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0x5465eea8 of type Test
gnustep::libobjc::__objc_class_type_info::__do_catch: Entering
gnustep::libobjc::__objc_class_type_info::__do_catch: Type info @id
gnustep::libobjc::__objc_class_type_info::__do_catch: object is an id type or class type and we're in Apple compat mode
gnustep::libobjc::__objc_class_type_info::__do_catch: got type Test
gnustep::libobjc::__objc_class_type_info::__do_catch: found is YES
gnustep::libobjc::__objc_class_type_info::__do_catch: found matching type
gnustep::libobjc::__objc_class_type_info::__do_catch: returning 1; *obj is 0x5465eea8
terminate called after throwing an instance of '@id'
__do_upcast: Entering
eh_cleanup: Releasing exception 0x5465eea8For objc_setUncaughtExceptionHandler: it looks like SetUnhandledExceptionFilter does not work on mingw64 but AddVectoredExceptionHandler does. There's not a lot of information on the subject; e.g. https://github.com/ocaml/ocaml/pull/938 or https://sourceforge.net/p/mingw-w64/support-requests/29/ (from 2010!)
davidchisnall
commented
6 months ago Okay, it looks as if the first throw and catch is working. That's a good start: throwing an ObjC object and catching id is the simplest case. The second throw looks as if it's going through the same code paths as first, which I don't think is correct. Rethrowing should be done via __cxa_rethrow. The simplest thing would probably be to modify this:
To remove the not-MinGW condition and then implement objc_exception_rethrow to just call __cxa_throw.
Can you share the LLVM IR for compiling this with the modified clang?
qmfrederik
commented
6 months ago Here's the clang change: https://github.com/llvm/llvm-project/commit/d68db09a39222aac18e6a8e1e3e55b099d75b765 And the libobjc2 change: https://github.com/gnustep/libobjc2/pull/267/commits/4970a82f7a0ed37b0f87ffe42e4cdbc52df3d654
That gives this output:
throw: Throwing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0x9db0eea8 of type Test
objc_exception_rethrow: Entering
terminate called without an active exceptionLooks like the application now halts earlier. I'll get you the intermediate representation in a follow-up.
qmfrederik
commented
6 months ago Here's the intermediate representation (assuming I did this right):
davidchisnall
commented
6 months ago Hmm, it looks as if clang isn't emitting the begin and end catch calls. In a finally block, I think it should be calling the _Unwind_rethrow thing (I think it was before the last change?). If I'm reading the IR correctly, @finally is emitted as a cleanup (which seems right) and so isn't rethrowing, it's resuming unwinding.
I'm a bit confused because it looks as if the finally code should be emitting the begin / end carch blocks:
Can you see what's going on there?
qmfrederik
commented
6 months ago OK - so we were entering the if(usesSEHExceptions) block which does not emit the try/catch blocks. Looks like we want a slightly different setup from the existing ones, where try/catch blocks are emitted and objc_exception_rethrow is called, like this: https://github.com/llvm/llvm-project/commit/b98340685303f18931b2e197aea3d13192373c48 ?
This causes the test to proceed a bit further:
throw: Throwing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0xfe10eea8 of type Test
objc_exception_rethrow: Entering
gnustep::libobjc::__objc_id_type_info::__do_catch: Entering
gnustep::libobjc::__objc_id_type_info::__do_catch: Type info @id
gnustep::libobjc::__objc_id_type_info::__do_catch: got id type info for 0xfe10eea8, returning true
rethrow_id: Rethrowing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0xfe10eea8 of type Test
gnustep::libobjc::__objc_class_type_info::__do_catch: Entering
gnustep::libobjc::__objc_class_type_info::__do_catch: Type info @id
gnustep::libobjc::__objc_class_type_info::__do_catch: object is an id type or class type and we're in Apple compat mode
gnustep::libobjc::__objc_class_type_info::__do_catch: got type Test
gnustep::libobjc::__objc_class_type_info::__do_catch: found is YES
gnustep::libobjc::__objc_class_type_info::__do_catch: found matching type
gnustep::libobjc::__objc_class_type_info::__do_catch: returning 1; *obj is 0xfe10eea8
eh_cleanup: Releasing exception 0xfe10eea8
rethrow_test: Rethrowing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0xfe10eea8 of type Test
eh_cleanup: Releasing exception 0xfe10eea8
rethrow_catchall: Rethrowing exception
objc_exception_throw: Entering
objc_exception_throw: Throwing exception 0xfe10ef70 of type (null)
Segmentation faultLooks like the final block as a wrong pointer to the exception (I assume it should have been 0xfe10eea8 but we have 0xfe10ef70 instead)? Also, I've seen to different definitions for objc_exception_rethrow : void objc_exception_rethrow(void*) and void objc_exception_rethrow(void)?
qmfrederik
commented
6 months ago The segfault comes from the logging code which calls object_getClass; if I disable the logging code the test will fail on assert(object_getClass(x) == [Test class]); all of this I assume because something goes wrong here:
@catch(...)
{
@throw;
}resulting in an invalid exception object being thrown?
qmfrederik
commented
6 months ago OK - so with https://github.com/llvm/llvm-project/commit/53132dab6c8dfcfd0d35efa53f4208b1b6dac388 I get all tests, except for UnexpectedException, to pass. ExceptionReThrowFn was set to objc_exception_throw instead of objc_exception_rethrow, which probably explained the behavior we've seen earlier.
davidchisnall
commented
6 months ago Nice! Can you add a clang test and raise a PR? I’ll review it.
You can base the test on this:
https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenObjC/exceptions-nonfragile.m
Pass some different target triples and runtime versions and make sure that it does the right thing in each case. You can run a single test with llvm-lit.
qmfrederik
commented
6 months ago Will do, thanks. Here's all current changes squashed into a single commit: https://github.com/llvm/llvm-project/commit/fa97083a5e87929fc1cf652cb9563e31a74457dd
qmfrederik
commented
6 months ago @davidchisnall Here you go: https://github.com/llvm/llvm-project/pull/77255
davidchisnall
commented
6 months ago Any ideas why the uncaught exception tests are failing? Does the VEH hook not do the right thing?
qmfrederik
commented
6 months ago The hook is not implemented yet, all it does (for now) is to return EXCEPTION_CONTINUE_SEARCH.
I haven't had the time yet to read up on the semantics of Vectored Exception Handlers (the documentation seems sparse) and the expected behavior of objc_setUncaughtExceptionHandler.
Where I left it at the moment:
objc_setUncaughtExceptionHandler be called for all exceptions (including C++) or only ObjC exceptions? I assume only for ObjC exceptions as it has an id exception argument? How can we reliably know the exception is an ObjC exception? Do we need to pass additional metadata to __cxa_throw (not just an id)?davidchisnall
commented
6 months ago Seems VEHs are invoked whenever an exception is raised (not just when it is unhandled); so probably need to do some filtering?
I think they're called before SEH, which is why I was unsure how it worked. I'm not sure if you get an unhandled-exception exception that you can catch with VEH?
Should the callback provided by objc_setUncaughtExceptionHandler be called for all exceptions (including C++) or only ObjC exceptions?
Well, that gets complicated. Probably only for ObjC ones, because C++ already has it defined that it calls std::terminate on unwind failure.
It would be nice to call std::set_terminate and wrap the existing handler in one that calls the ObjC handler if we're in the middle of an ObjC exception throw, but I don't think that's actually possible.
I assume only for ObjC exceptions as it has an id exception argument? How can we reliably know the exception is an ObjC exception? Do we need to pass additional metadata to __cxa_throw (not just an id)
I think the type info is embedded in the thrown object. It's a std::type_info subclass and, for ObjC, we use exactly one so you can just check if the id type info is thrown.
The corner case here is that you can use throw in Objective-C++ to throw an Objective-C object. This will throw it with one of the other type infos. I don't know whether I'd expect that to call the unhandled handler or not. Worth checking what macOS does.
qmfrederik
commented
6 months ago @davidchisnall Looks like the easiest way to do is to:
__cxa_rethrow() (guarded by __cxa_current_exception_type()) to rethrow the exception@catch(id) to get the underlying exceptionI've pushed a commit which implements this.
Feels a bit clunky (but it's similar to what the Apple runtime does); but I couldn't find an easier way to get this information. I believe ex->ExceptionInformation[0] is a _Unwind_Exception, but that only exposes an exception_class and not the actual exception?
davidchisnall
commented
6 months ago That’s probably easier. The unwind exception holds a pointer to the C++ exception, which holds the type info and has the object appended, but different C++ runtimes have subtly different versions of this structure. Given that this is a very slow path (basically only ever used to pop up a diagnostic window when a thread crashes) it’ probably fine for it to do the rethrow thing.
qmfrederik
commented
6 months ago @davidchisnall I've made some last changes to this PR:
With that, all tests on MinGW now pass (100% tests passed, 0 tests failed out of 98):
I think this PR is now in a pretty good shape, let me know if there's anything else you need from me.
davidchisnall
commented
6 months ago Please can you squash before you merge?
qmfrederik
commented
6 months ago @davidchisnall I left some additional notes, and I updated UnexpectedException.m to assert that handler is invoked for unhandled exceptions only.
qmfrederik
commented
6 months ago Please can you squash before you merge?
I squashed but don't have merge permissions in this repo ;-)
davidchisnall
commented
6 months ago Please can you squash before you merge?
I squashed but don't have merge permissions in this repo ;-)
Fixed.
qmfrederik
commented
6 months ago Cool, thanks!
Alternative implementation of #260. Requires changes to clang - see https://github.com/qmfrederik/llvm-project/commit/88ce1ba8d714c5f56e3697151119a85a4fba