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 ARC and ObjC++ variants of ExceptionTest #138

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

triplef
Copy link
Member

@triplef triplef commented Dec 2, 2019

The ObjC++ tests fail for me on FreeBSD 12.0 in the following way, which I think exposes issues in ObjC++ EH / ARC.

* thread #1, name = 'ExceptionTestObj', stop reason = signal SIGABRT
  * frame #0: 0x000000080047b45a libc.so.7`_thr_kill + 10
    frame #1: 0x0000000800479844 libc.so.7`_raise + 52
    frame #2: 0x00000008003ec079 libc.so.7`abort + 73
    frame #3: 0x00000008004692c1 libc.so.7`__assert + 81
    frame #4: 0x000000000020272d ExceptionTestObjCXX`rethrow_id(void) at ExceptionTestObjCXX.mm:36
    frame #5: 0x000000000020277d ExceptionTestObjCXX`rethrow_test(void) at ExceptionTestObjCXX.mm:44
    frame #6: 0x000000000020289d ExceptionTestObjCXX`rethrow_catchall(void) at ExceptionTestObjCXX.mm:61
    frame #7: 0x00000000002029a4 ExceptionTestObjCXX`main at ExceptionTestObjCXX.mm:84
    frame #8: 0x000000000020211b ExceptionTestObjCXX`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76
* thread #1, name = 'ExceptionTestObj', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x0000000800276774 libobjc.so.4.6`__objc_block_trampoline_end_sret at objc_msgSend.x86-64.S:308
    frame #1: 0x000000080027b827 libobjc.so.4.6`retain(obj=0x00000008008b4080) at arc.mm:307
    frame #2: 0x000000080027b730 libobjc.so.4.6`::objc_retain(obj=0x00000008008b4080) at arc.mm:587
    frame #3: 0x00000000002026e7 ExceptionTestObjCXX_arc`rethrow_id(void) at ExceptionTestObjCXX_arc.mm:35
    frame #4: 0x000000000020278d ExceptionTestObjCXX_arc`rethrow_test(void) at ExceptionTestObjCXX_arc.mm:44
    frame #5: 0x00000000002028dd ExceptionTestObjCXX_arc`rethrow_catchall(void) at ExceptionTestObjCXX_arc.mm:61
    frame #6: 0x0000000000202a04 ExceptionTestObjCXX_arc`main at ExceptionTestObjCXX_arc.mm:84
    frame #7: 0x000000000020211b ExceptionTestObjCXX_arc`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76

I also saw that the existing ObjC++ tests were not run against the legacy ABI – does that not support ObjC++ EH? Let me know if I should exclude the new ObjC++ tests from the legacy tests.

@triplef
Copy link
Member Author

triplef commented Dec 2, 2019

Added another ObjC++ test which throws small NSString objects, leading to the following backtrace:

* thread #1, name = 'NestedExceptions', stop reason = signal SIGSEGV: invalid address (fault address: 0x20)
  * frame #0: 0x000000080027b0ef libobjc.so.4.6`objc_test_class_flag(aClass=0x0000000000000000, flag=objc_class_flag_fast_arc) at class.h:384
    frame #1: 0x000000080027a139 libobjc.so.4.6`autorelease(obj=0xcac4000000000014) at arc.mm:425
    frame #2: 0x000000080027a593 libobjc.so.4.6`::objc_autorelease(obj=0xcac4000000000014) at arc.mm:532
    frame #3: 0x000000080027a85d libobjc.so.4.6`::objc_retainAutorelease(obj=0xcac4000000000014) at arc.mm:592
    frame #4: 0x00000000002014e5 NestedExceptionsObjCXX_arc`throwException(void) at NestedExceptionsObjCXX_arc.mm:7
    frame #5: 0x0000000000201541 NestedExceptionsObjCXX_arc`main at NestedExceptionsObjCXX_arc.mm:18
    frame #6: 0x000000000020111b NestedExceptionsObjCXX_arc`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76

Not sure if these are separate issues or not.

@triplef
Copy link
Member Author

triplef commented Apr 28, 2020

Unfortunately it looks like these are still failing after the fix for #146, but the issue here might also be completely unrelated.

@davidchisnall
Copy link
Member

These were failing for a different reason. The Nested tests were also failing because you're calling -dealloc on an NSConstantString...

davidchisnall added a commit that referenced this pull request May 2, 2020
This makes no difference to existing tests, but is needed for the ones
in #138
@davidchisnall
Copy link
Member

I have opened two pull requests (#158 and #159) that fix these for me. Please can you rebase once they are merged, so we can run these tests in CI and check the fixes worked everywhere?

davidchisnall added a commit that referenced this pull request May 2, 2020
This makes no difference to existing tests, but is needed for the ones
in #138
@davidchisnall
Copy link
Member

It looks as if there's still an issue with libsupc++, though this is now working with libcxxrt...

@davidchisnall
Copy link
Member

It looks as if some of the more complex cases are causing problems because clang is emitting __cxa_begin_catch instead of objc_begin_catch for Objective-C++ try / catch blocks. That means that state can get out of sync between the Objective-C and C++ runtime libraries. The simplest solution may be to just always throw Objective-C exceptions as C++ exceptions...

davidchisnall added a commit that referenced this pull request May 2, 2020
libsupc++ is more aggressive about internal consistency checks than
libcxxrt, so we need to be more careful in the interop.  The tests from
PR #138 now pass for me on Debian with libsupc++.
@davidchisnall davidchisnall mentioned this pull request May 2, 2020
davidchisnall added a commit that referenced this pull request May 2, 2020
libsupc++ is more aggressive about internal consistency checks than
libcxxrt, so we need to be more careful in the interop.  The tests from
PR #138 now pass for me on Debian with libsupc++.
@davidchisnall
Copy link
Member

Looks as if these tests all pass now with the Itanium ABI, but they're failing with the Win32 version. That's a bit surprising, because the Win32 EH implementation is a very thin wrapper around C++ exceptions on Windows, so it's probably caused by bugs in clang.

@DHowett-MSFT, did you ever have time to look at the clang handling of @finally? I think this may be related.

@triplef triplef force-pushed the objcxx-exception-tests branch 2 times, most recently from 0857f8f to b6ef305 Compare May 2, 2020 18:37
@triplef
Copy link
Member Author

triplef commented May 2, 2020

Thank you for looking into this! I’ve rebased the branch on the lastest master.

These were failing for a different reason. The Nested tests were also failing because you're calling -dealloc on an NSConstantString...

Good catch, fixed.

@DHowett-MSFT, did you ever have time to look at the clang handling of @finally? I think this may be related.

Is this a general issue with @finally, or specific to Windows?

@davidchisnall
Copy link
Member

@DHowett-MSFT, did you ever have time to look at the clang handling of @finally? I think this may be related.

Is this a general issue with @finally, or specific to Windows?

I think the funclets for cleanups are incorrect in the SEH codegen (Windows only).

@davidchisnall
Copy link
Member

Please can you update this branch? Some of the SEH stuff may have been fixed in a newer clang build.

@triplef
Copy link
Member Author

triplef commented Dec 17, 2020

I rebased the branch, but looks like there are still some issue.

@davidchisnall
Copy link
Member

Very odd set of failures! Windows has a completely different EH model to everything else, so I'm not surprised if there are tests that fail there but not anywhere else. With the new test matrix; however, this appears fine with libcxxrt on FreeBSD and with libsupc++ on Linux but not with libc++abi on Linux. I wonder if it's the same bug and, if so, if it's easier to debug the Linux failure than the Windows one...

@davidchisnall
Copy link
Member

Looks as if the non-ARC variant is failing because it's missing a double dereference on the thrown object with libc++abi. @ngrewe?

Adds ARC and ObjC++ variants of ExceptionTest.
Adapted to throw short NSString objects which should fit in a small object, as it seems to expose a different ARC issue.
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.

2 participants