gnustep / libobjc2

Objective-C runtime library intended for use with Clang.
http://www.gnustep.org/
MIT License
426 stars 116 forks source link

MinGW: Use _Unwind_RaiseException to throw exceptions #278

Closed qmfrederik closed 4 months ago

qmfrederik commented 4 months ago

The current implementation uses Vectored Exception Handlers. This implementation is too greedy, and invokes _objc_unexpected_exception for (certain) exceptions which would be handled by the application itself.

qmfrederik commented 4 months ago

@lazka @MehdiChinoune It seems like SetUnhandledExceptionFilter does not work for msys2/mingw64 applications. Any idea what could cause this?

davidchisnall commented 4 months ago

I think the right way for this to be implemented on MinGW should match the Itanium model, where throwing an exception that doesn’t have a handler returns from the unwind library. Unfortunately, when we tried this, the unwind library did not return. Maybe that’s fixed?

MinGW’s hybrid of SEH and Itanium ABIs is quite painful and more effort to support than either, I wish they’d just use the Visual Studio ABI.

qmfrederik commented 4 months ago

Hmm, interesting. I rewrote objc_exception_throw to use _Unwind_RaiseException instead of __cxa_throw and that does seem to work (at least on mingw64, which uses stdc++).

Is something like this what you had in mind?

extern "C"
OBJC_PUBLIC
void objc_exception_throw(id object)
{
    id *exc = (id *)__cxxabiv1::__cxa_allocate_exception(sizeof(id));
    *exc = object;
    objc_retain(object);
    DEBUG_LOG("objc_exception_throw: Throwing 0x%x\n", *exc);

    __cxa_eh_globals *globals = __cxa_get_globals ();
    globals->uncaughtExceptions += 1;
    // Definitely a primary.
    __cxa_refcounted_exception *header =
        __cxa_init_primary_exception(exc, & __objc_id_type_info, eh_cleanup);
    header->referenceCount = 1;

    _Unwind_Reason_Code err = _Unwind_RaiseException (&header->exc.unwindHeader);

    // Some sort of unwinding error.  Note that terminate is a handler.
    __cxa_begin_catch (&header->exc.unwindHeader);
    DEBUG_LOG("objc_exception_throw: _Unwind_RaiseException returned 0x%x for exception 0x%x\n", err, *exc);

    if (_URC_END_OF_STACK == err && 0 != _objc_unexpected_exception)
    {
        DEBUG_LOG("Invoking _objc_unexpected_exception\n");
        _objc_unexpected_exception(object);
    }
    DEBUG_LOG("Throw returned %d\n",(int) err);
    abort();
}
davidchisnall commented 4 months ago

Yes, though I'm not sure you need the __cxa_begin_catch.

qmfrederik commented 4 months ago

@davidchisnall Here's another attempt. It comes at the expense of having to declare __cxa_exception and __cxa_refcounted_exception (to get the unwind header from the __cxa_refcounted_exception object).

The build on the clang64 fails because that's using libc++ instead of libstdc++, I'll look into that.

qmfrederik commented 4 months ago

ok - this doesn't work yet on msys/clang64 because libc++ doesn't expose __cxa_init_primary_exception, but that's coming in clang 18.1.0: https://github.com/llvm/llvm-project/commit/51e91b64d0deb6a743861c2a0fba84865250036e

qmfrederik commented 4 months ago

Hi @davidchisnall, would you have the bandwidth to review this and 297 in the near future? I'm fairly confident the gnustep-base unit test will pass if this fix goes in.

Alternatively, as a short-term workaround, I could submit these two fixes for the MSYS2 packages while we work our way through the code review here.

qmfrederik commented 4 months ago

Hi @davidchisnall, thanks for the feedback. I'll go through it in detail later. Just to clarify this comment:

This is going to break a load of things. If it were guarded behind MinGW checks, I wouldn't mind so much, but we absolutely should not be doing this with the Itanium ABI.

All of these changes are guarded by the #ifndef __MINGW32__ check in line 497, so yes, they are for MinGW only.

qmfrederik commented 4 months ago

I spent some more time looking at this, but at the moment think we're stuck between a rock and a hard place:

So the options I see are:

qmfrederik commented 4 months ago

So, we can use the runtime check to determine the exception struct size, so that simplifies things a bit. This is as far as I can take it, I'm afraid.

qmfrederik commented 4 months ago

@davidchisnall Would you mind having another look at this one?

davidchisnall commented 4 months ago

I still don’t like the fact that it’s conflating C++ runtimes with standard library implementations or all of the ifdefs. If you can put this code in a separate file that!s built only for MinGW so no one reading the code for less baroque platforms will be confused then it!s probably okay.

qmfrederik commented 4 months ago

@davidchisnall I moved the MinGW code to its own class, which I briefly considered calling objcxx_eh_baroque.c.

The definitions of the C++ ABI functions and types as well as the family of __objc_type_info structs is shared across both, so I moved that to a objcxx_eh_private.h header file. Let me know if that is acceptable or if you'd prefer another approach.