Support _objc_unexpected_exception hook on Win32

[triplef]

This adds support for the _objc_unexpected_exception hook on Windows as discussed in #219.

GNUstep Base setting _objc_unexpected_exception worked fine in my testing and callUncaughtHandler() is called with these changes, contrary to what @davidchisnall you said about exposing global variables from a DLL. Not sure if I’m missing something there.

The only issue I ran into was that setting the exception filter via SetUnhandledExceptionFilter() from a constructor block didn’t work for me, I think because the VC runtime sets a handler itself at startup in pre_cpp_initialization(), which at least for me was done after the constructor was called. I ended up setting it at the top of objc_exception_throw() instead, which I’m not really happy with unless we could restore the original handler afterwards, but I’m not sure if we get any callback if the exception is actually handled. I’d appreciate any thoughts here to improve this.

This article has some more details about how the CRT uses SetUnhandledExceptionFilter() internally.

Fixes #219.

[triplef]

I added a test for the _objc_unexpected_exception hook (passed on both 32- and 64-bit Windows).

[triplef]

I found one strange issue while working on this further: when running any process throwing an unhandled ObjC exception I always get an access violation like this (with varying addresses) after RtlRaiseException() is called. However when I run without a debugger this doesn’t seem to happen. It’s easily reproducible using the UnexpectedException test. Any idea what could be going on here? The issue seems to come from a std::string getting released (see second screenshot), although it seems to me like this one should be released before RtlRaiseException()?

The issue doesn't seem to be related to my changes here though, as it also happens without them.

[davidchisnall]

Not sure what's going on there. I found trying to debug this code in Visual Studio often didn't work because it installed hooks into a bunch of places in the unwind machinery that caused it to not work properly.

[triplef]

I rebased this to get the latest CI builds and the cross builds are all failing the new UnexpectedException test. Is _objc_unexpected_exception not supported on ARM? I thought it was.

[triplef]

I was looking at this again to try to find a place where we can install our exception handler in a way so it doesn’t get overwritten by the CRT.

SetUnhandledExceptionFilter() is called as part of the runtime’s pre_cpp_initialization() (installed via .CRT$XCAA), which unfortunately gets called after __attribute__((constructor)) blocks and static struct constructors.

@davidchisnall can you think of another way we can ensure the CRT gets initialized before we install our handler? Here’s the stack trace leading up to setting the exception handler in the runtime:

    KernelBase.dll!SetUnhandledExceptionFilter()    Unknown
    app.exe!__scrt_set_unhandled_exception_filter() Line 102    C++
    app.exe!pre_cpp_initialization() Line 222   C++
    ucrtbased.dll!_initterm(void(*)() * first, void(*)() * last) Line 22    C++
    app.exe!__scrt_common_main_seh() Line 258   C++
    app.exe!__scrt_common_main() Line 331   C++
    app.exe!WinMainCRTStartup(void * __formal) Line 17  C++
    kernel32.dll!BaseThreadInitThunk()  Unknown
    ntdll.dll!RtlUserThreadStart()   Unknown

Alternatively what do you think of exposing a library function for users to call to manually install the handler?

[davidchisnall]

Have you looked at the way we register the calls to the entry points? We put our init code in the .CRT$XCLz section. The .CRT section contains function pointers to init functions and relies on the fact that subsections are sorted. The XCL prefix is for libraries, which run before user code. The z suffix means 'after all the others please'. You can try sticking a global variable that points to a function that registers the hook in a global in a section with a name like this (use the section attribute) and see where it can live to not get clobbered?

[triplef]

You mean like this?

void _objc_install_unhandled_exception_filter(void)
{
    LPTOP_LEVEL_EXCEPTION_FILTER previousExceptionFilter = SetUnhandledExceptionFilter(&_objc_unhandled_exception_filter);
    if (previousExceptionFilter != &_objc_unhandled_exception_filter) {
        originalUnhandledExceptionFilter = previousExceptionFilter;
    }
}

__attribute__((section(".CRT$XCLz"), used)) static void (*__objc_init)(void) = _objc_install_unhandled_exception_filter;

Unfortunately that also gets called before pre_cpp_initialization().

[davidchisnall]

I believe XCU is used for user code (not library) initialisation, so XCUa might work?

[triplef]

That still gets called before pre_cpp_initialization() unfortunately. I also tried XCUz, and verified we don’t get called at all when I comment out the variable – just to make sure I’m not doing something silly.

[davidchisnall]

What about ZZZz?

[triplef]

ZZZz doesn’t seem to get called at all.

I’m not sure if this approach will work at all, since if I understand this correctly all of these pointers in the .CRT sections get called on DLL load, and for me ucrtbased.dll executing the code shown above gets loaded after objc.dll. So we would have to somehow delay setting the exception hander until after, not sure if that’s even possible?

That’s why I was wondering if we should expose a function for the user to call to register the handler. In fact I just realized that the Apple runtime has objc_setUncaughtExceptionHandler() instead of the _objc_unexpected_exception function pointer. What do you think about switching to that? GNUstep Base supports both and will detect the availability during configure.

[triplef]

I’ve implemented the objc_setUncaughtExceptionHandler() API, let me know what you think.

I left the hook defined for non-Windows platforms and just added a note about deprecation. Happy to remove it completely as well.

[triplef]

I just realized that GNUstep Base expects this new function to be in objc/objc-exception.h like in the Apple runtime. Would it make sense to add that header, or should I patch that config check instead (e.g. using __has_include)?

[davidchisnall]

It’s a good idea to put it in the header. Happy to remove the global unconditionally on Windows and deprecate it elsewhere.

[triplef]

Sounds good, I’ll make that change.

What should we do about the crashes on the cross-build ARM CI targets – any idea what could be going on there? If it’s ok I could just disable the new test on ARM for now so we can merge this, and open a ticket about it. I’m curious about it because we do use the exception hook on Android ARM and it’s working fine for us there.

[davidchisnall]

What should we do about the crashes on the cross-build ARM CI targets – any idea what could be going on there? If it’s ok I could just disable the new test on ARM for now so we can merge this, and open a ticket about it. I’m curious about it because we do use the exception hook on Android ARM and it’s working fine for us there

No, that's a bit worrying, since the test that's failing is for the functionality that you've modified. Can you try debugging it locally? That test was passing last time it ran on the main branch, so it looks like a regression.

[triplef]

The failing "UnexpectedException" test was added with this PR, I believe the tests just didn’t cover the exception hook before. Since I only touched the Windows implementation I don’t think it’s a new issue but I’ll try to figure out how to debug it locally.

[davidchisnall]

If it was already broken on Arm, happy to temporarily disable the test until someone can look at it. There are some differences in the Arm EH ABI that may account for it. I'm not sure if it actually unwinds if it runs out of stack, we may need to call the handler from the personality function on Arm.

[davidchisnall]

Huh, I thought we had an atomic.h that wrapped the builtins. Maybe I just used the compiler builtins directly elsewhere since we can rely on being built with a clang-compatible compiler.

[triplef]

Thanks for your help @davidchisnall!

[davidchisnall]

Thanks for actually doing all of the work! I just heckle...