Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CI leg for msys2/mingw64 using libobjc2 + gnustep-2.0 ABI #374

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

qmfrederik
Copy link
Contributor

libobjc2 now supports msys2/mingw64 and is part of the msys2 package repositories, so it's easier to add a CI job which uses that environment.

I had to make one minor change in Tests/GNUMakefile to avoid passing a -rpath flag on mingw.
The check for thread-safe +initialize in runtime will fail, but I believe #373 fixes that.

@qmfrederik qmfrederik requested a review from rfm as a code owner February 11, 2024 15:22
@qmfrederik
Copy link
Contributor Author

The relevant test failures:

D:\a\libs-base\libs-base\source\Tests\base\NSOperation\obj\basic.exe: Uncaught exception NSInvalidArgumentException, reason: [NSOperation-start] called on finished operation
2024-02-11 15:05:45.383 basic[6528:5280] WARNING your program is becoming multi-threaded, but you are using an ObjectiveC runtime library which does not have a thread-safe implementation of the +initialize method. Please see README.initialize for more information.
Failed file:     basic.m aborted without running all tests!

and

Running base/NSOperation/threads.m...
2024-02-11 15:05:45.936 threads[1916:4728] WARNING your program is becoming multi-threaded, but you are using an ObjectiveC runtime library which does not have a thread-safe implementation of the +initialize method. Please see README.initialize for more information.
Passed test:     (2024-02-11 15:05:45.920 +0000) threads.m:149 ... counter was set up
Passed test:     (2024-02-11 15:05:45.936 +0000) threads.m:154 ... operation ran
Passed test:     (2024-02-11 15:05:45.936 +0000) threads.m:155 ... operation ran in this thread
Passed test:     (2024-02-11 15:05:45.936 +0000) threads.m:156 ... operation ran in main queue
Passed test:     (2024-02-11 15:05:46.045 +0000) threads.m:165 ... operation finished
Passed test:     (2024-02-11 15:05:46.045 +0000) threads.m:166 ... operation ran
Passed test:     (2024-02-11 15:05:46.045 +0000) threads.m:167 ... operation ran in other thread
Passed test:     (2024-02-11 15:05:46.155 +0000) threads.m:176 ... operation exited
Passed test:     (2024-02-11 15:05:46.155 +0000) threads.m:177 ... operation ran
Passed test:     (2024-02-11 15:05:46.155 +0000) threads.m:178 ... operation ran in other thread
Passed test:     (2024-02-11 15:05:46.155 +0000) threads.m:179 ... operation seems to be running
D:\a\libs-base\libs-base\source\Tests\base\NSOperation\obj\threads.exe: Uncaught exception NSGenericException, reason: done
Failed file:     threads.m aborted without running all tests!

I assume both are related to #373, let me cherry-pick that into this PR

@qmfrederik
Copy link
Contributor Author

Both test failures seem to PASS_EXCEPTION tests. In both cases, it seems like the expected exception is actually trown, but somehow not caught by PASS_EXCEPTION.

@rfm
Copy link
Contributor

rfm commented Feb 11, 2024

it seems like the expected exception is actually trown, but somehow not caught

I vaguely remember having seen something like that happen with libobjc2 long ago, and I think David said it was to do with mixing C++ and ObjC exceptions somehow, but I don't recall any details.

@qmfrederik qmfrederik force-pushed the ci-mingw-libobjc2 branch 3 times, most recently from a32c9dc to 99af912 Compare February 11, 2024 21:53
@qmfrederik
Copy link
Contributor Author

Looks like this is specific to an exception being caught in NS_HANDLER and then being rethrown (which is entirely valid). If I change the code to assign the caught exception to a local variable and rethrow it outside of the handler (99af912), the tests all pass.

@davidchisnall Any idea?

@davidchisnall
Copy link
Member

There were some LLVM issues with that structure in SEH, but they were fixed. Maybe the fix doesn’t work for MinGW’s weird hybrid?

If you can make the same shape work with C++ but fail with ObjC, we can compare the code and see.

@qmfrederik
Copy link
Contributor Author

It looks somewhat similar to this: https://lists.gnu.org/archive/html/discuss-gnustep/2017-12/msg00083.html / https://lists.gnu.org/archive/html/discuss-gnustep/2017-12/msg00110.html:

The [NSException raise] method just calls @throw self when
_NATIVE_OBJC_EXCEPTIONS is defined

It does, but [NSException raise] is another function (well, method, but after
the call to objc_msgSend it’s a function). The exception is thrown from
there, not from the stack frame containing the @catch block.

It turns out that this is the crucial bit: throwing an Objective-C exception
through a C++ catch (or ObjC++ @catch) block was broken. This is pretty
uncommon, but we now have a test for it and it appears to be passing.

I'll try create a simple program which can reproduce the issue. Meanwhile, 99af912 seems to be a workaround, perhaps that provides some additional insight?

@qmfrederik
Copy link
Contributor Author

qmfrederik commented Feb 12, 2024

Just leaving some notes here:

This issue looks very similar to the issue which was reported in 2017. The small repro can be used to reproduce the issue, and re-throwing the exception out of the @catch block fixes the issue -- see test code below.

Looks like the original fix was gnustep/libobjc2@7bd78e5. There was a repo in libobjc2 (gnustep/libobjc2#50), but not sure that test is still part of the test suite. I'll try to look into that later.

// Build dependencies:
// pacman -S mingw-w64-x86_64-gnustep-base mingw-w64-x86_64-gnustep-make mingw-w64-x86_64-clang mingw-w64-x86_64-lld
//
// Compile with:
// clang $(gnustep-config --objc-flags) $(gnustep-config --base-libs) repo.m

#import <Foundation/Foundation.h>

@interface MinRep2 : NSObject {
}
- (void)poke;
@end

@implementation MinRep2
- (void)poke {
  NSAutoreleasePool *pool = [NSAutoreleasePool new];
  NS_DURING {
    [NSException raise:@"foo" format:@"bar"];
  } NS_HANDLER {
    [localException retain];
    [pool release];
    [[localException autorelease] raise];
  } NS_ENDHANDLER
  [pool release];
}

- (void)poke2 {
  NSException* caughtException = nil;
  NSAutoreleasePool *pool = [NSAutoreleasePool new];
  NS_DURING {
    [NSException raise:@"foo" format:@"bar"];
  } NS_HANDLER {
    [localException retain];
    [pool release];
    caughtException = localException;
    // [[localException autorelease] raise];
  } NS_ENDHANDLER
  [pool release];
  [caughtException raise];
}
@end

int main()
{
  id obj = [MinRep2 new];
  
  @try
  {
    [obj poke2];
  }
  @catch (NSException * localException)
  {
    fprintf(stderr, "[obj poke2]: Caught an exception.\n");
  }

  @try
  {
    [obj poke];
  }
  @catch (NSException * localException)
  {
    fprintf(stderr, "[obj poke]: Caught an exception.\n");
  }
}

@davidchisnall
Copy link
Member

That's unlikely to be the cause, since on MinGW we don't hit any of those code paths: we just pretend that we're throwing C++ exceptions all of the time.

@qmfrederik
Copy link
Contributor Author

@davidchisnall I've verified locally that this is because the vectored exception handler which is used on mingw64 gets invoked for (certain?) exceptions which would be caught by the normal application flow. This results in _objc_unexpected_exception being called and the application being terminated.

I wasn't able to find a way within _objc_vectored_exception_handler to determine whether the exception was actually uncaught or not; and SetUnhandledExceptionFilter does not appear to work on mingw64.

I've opened https://github.com/gnustep/libobjc2/pull/278/files to remove the vectored exception handler from libobjc2 on mingw64.

@qmfrederik
Copy link
Contributor Author

@rfm This was fixed via gnustep/libobjc2#278, which has been backported to the version of libobjc2 which is in MinGW. The remaining CI failure is, I believe, unrelated and being tracked by #367.

So - I think this is good to go. Really cool to see all libs-base tests passing on MinGW/libobjc2/clang, thanks @davidchisnall for your patience with me during the process.

@rfm rfm merged commit 648f3e2 into gnustep:master Mar 7, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants