Use C++ exceptions unconditionally for Objective-C[++] on MinGW #267
+105
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current status:
|
|
I presume that we can use the implementation from here on MinGW for the first point? Line 276 in 3c42c64 |
|
OK, I've added a dummy implementation of A lot of tests are currently failing like this:
For example: Any thoughts on what could cause that? |
|
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). |
|
From #260 (comment)
This doesn't reproduce after rebuilding, but leaving the comment here for future reference. |
|
I added additional logging and this is the current output (contradicting my previous comment): |
|
Can you step through the do-catch methods and see where the match is failing? |
qmfrederik
force-pushed
the
cxx-exceptions
branch
from
e413ec0 to
0eaea40
Compare
|
OK - some progress. The actual exception object wasn't being copied in With that, most tests seem to pass; only |
|
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): |
|
For |
|
Okay, it looks as if the first throw and catch is working. That's a good start: throwing an ObjC object and catching To remove the not-MinGW condition and then implement Can you share the LLVM IR for compiling this with the modified clang? |
|
Here's the clang change: llvm/llvm-project@d68db09 That gives this output: Looks like the application now halts earlier. I'll get you the intermediate representation in a follow-up. |
|
Here's the intermediate representation (assuming I did this right): |
|
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, 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? |
|
OK - so we were entering the This causes the test to proceed a bit further: Looks like the final block as a wrong pointer to the exception (I assume it should have been |
|
The segfault comes from the logging code which calls resulting in an invalid exception object being thrown? |
|
OK - so with llvm/llvm-project@53132da I get all tests, except for |
|
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. |
|
Will do, thanks. Here's all current changes squashed into a single commit: llvm/llvm-project@fa97083 |
|
@davidchisnall Here you go: llvm/llvm-project#77255 |
This was referenced Jan 7, 2024
davidchisnall
approved these changes
Jan 8, 2024
|
Any ideas why the uncaught exception tests are failing? Does the VEH hook not do the right thing? |
|
The hook is not implemented yet, all it does (for now) is to return 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 Where I left it at the moment:
|
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?
Well, that gets complicated. Probably only for ObjC ones, because C++ already has it defined that it calls It would be nice to call
I think the type info is embedded in the thrown object. It's a The corner case here is that you can use |
|
@davidchisnall Looks like the easiest way to do is to:
I'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 |
|
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. |
|
@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. |
There was a problem hiding this comment.
Looks mostly fine. I don’t understand the VEH bits fully (I thought those handlers happened first, why isn’t it called for every thrown exception?) so a few commends explaining the model would be nice for the next person. Also a couple of changes needed to make it work properly in multithreaded contexts.
eh_win32_mingw.m
Outdated
| extern void __cxa_rethrow(); | ||
|
|
||
| BOOL handler_installed = NO; | ||
| BOOL in_handler = NO; |
There was a problem hiding this comment.
These probably need to be thread local.
There was a problem hiding this comment.
in_handler: yes, handler_installed tracks the call to AddVectoredExceptionHandler which should called only once per process?
| objc_uncaught_exception_handler previousHandler = __atomic_exchange_n(&_objc_unexpected_exception, handler, __ATOMIC_SEQ_CST); | ||
|
|
||
| // Add a vectored exception handler to support the hook. We only need to do this once. | ||
| if (!handler_installed) { |
There was a problem hiding this comment.
This probably also needs to be atomic.
There was a problem hiding this comment.
Hmm - this is to protect against a race condition where objc_setUncaughtExceptionHandler might be called from multiple threads at the same time? I guess we could add a mutex around this block but is that a realistic scenario?
|
Please can you squash before you merge? |
qmfrederik
force-pushed
the
cxx-exceptions
branch
from
671adc3 to
f631f0c
Compare
|
@davidchisnall I left some additional notes, and I updated |
davidchisnall
approved these changes
Jan 10, 2024
I squashed but don't have merge permissions in this repo ;-) |
Fixed. |
|
Cool, thanks! |
davidchisnall
pushed a commit
to llvm/llvm-project
that referenced
this pull request
Jan 10, 2024
The GNUstep Objective C runtime (libobjc2) is adding support for the GNU ABI on Windows (more specifically, MinGW). The libobjc2 runtime uses C++ exceptions in that configuration; this PR updates clang to act accordingly. The corresponding change to libobjc2 is here: gnustep/libobjc2#267
justinfargnoli
pushed a commit
to justinfargnoli/llvm-project
that referenced
this pull request
Jan 28, 2024
The GNUstep Objective C runtime (libobjc2) is adding support for the GNU ABI on Windows (more specifically, MinGW). The libobjc2 runtime uses C++ exceptions in that configuration; this PR updates clang to act accordingly. The corresponding change to libobjc2 is here: gnustep/libobjc2#267
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative implementation of #260. Requires changes to clang - see qmfrederik/llvm-project@88ce1ba