MinGW: Use _Unwind_RaiseException to throw exceptions

[qmfrederik]

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]

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

[davidchisnall]

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]

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]

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

[qmfrederik]

@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]

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: llvm/llvm-project@51e91b6

[qmfrederik]

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.

[davidchisnall]

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.

[davidchisnall]

We used to have a definition like this, but we removed it because different C++ runtimes had subtly different variations on it. This is why we have the thing that dynamically probes it for the fields that we need. I'm not sure that reintroducing it is a good idea.

[davidchisnall]

Not all runtimes have this, and it causes different offsets because of alignment padding.

[qmfrederik]

Correct, but MSYS2 really only ships with two runtimes -- libstdc++ and libc++.

[davidchisnall]

This is reintroducing tight coupling with C++ runtime implementations, which we had previously removed. It's also totally the wrong check, because you're checking for libc++ but libc++ can be built with libc++abi, libcxxrt, or libsupc++.

[qmfrederik]

This #ifdef is trying to determine whether you're on the mingw64 or ucrt64 MSYS2 environment (which use libstdc++ and have __cxa_init_primary_exception) or the clang64 MSYS2 environment, which doesn't have __cxa_init_primary_exception and for which we revert to the old behavior of using __cxa_throw.

I've redefined the check as #ifndef __GLIBCXX__, but not sure if that really addresses your comment.

As for the tight coupling: yes, I know, though we tried the dynamic check for the exception struct size and that didn't work.

[qmfrederik]

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]

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

• SetUnhandledExceptionFilter does not work on MSYS2.
• We had many attempts at making the dynamic probing work. That didn't work out for MSYS2.
• All we need is a variant of __cxa_throw which returns (instead of aborting) if the exception is handled, but no such variant appears to exist in the public ABI.
• The alternative is to use __cxa_init_primary_exception and _Unwind_RaiseException. __cxa_init_primary_exception returns a __cxa_refcounted_exception. _Unwind_RaiseException expects an unwind header. We must know the layout of __cxa_refcounted_exception to be able to access &header->exc.unwindHeader.
• I had considered to directly include the libstdc++ headers to avoid having to manually define these types, but those structs are opaque.

So the options I see are:

• Don't bother with unhandled exception handling on MSYS2, and just let the process abort. The easiest option.
• Carry a runtime-dependent implementation for MSYS2 as part of libobjc2, guarded within #ifdef __MINGW32__. The environments in MSYS2 are standardized: https://www.msys2.org/docs/environments/, so I think it's safe to assume you are using either libstdc++ or libc++, and we can add all 3 64-bit environments (mingw64, ucrt64 and clang64) to our CI matrix
• Carry a runtime-dependent implementation for MSYS2 as a patch within the MSYS2 package.

[qmfrederik]

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]

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

[davidchisnall]

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]

@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.