gnustep / libobjc2

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

Added referenceCount to __cxa_exception struct and updated unwind alignment #129

Closed triplef closed 4 years ago

triplef commented 4 years ago

Updated __cxa_exception to add referenceCount fields to match lates revisions from libcxxabi and libcxxrt: https://github.com/llvm/llvm-project/blob/master/libcxxabi/src/cxa_exception.h#L30-L67 https://github.com/pathscale/libcxxrt/blob/master/src/cxxabi.h#L77-L165

Updated _Unwind_Exception structs to match latest revisions from libunwind:

The last two changes are also mirrored in this PR for libcxxrt: https://github.com/libcxxrt/libcxxrt/pull/1

Fixes ObjCXXEHInterop tests on Android x86 and x86_64, as reported in #108. However, it does not yet fix the same test on Android ARMv7.

triplef commented 4 years ago

Looks like the tests on Cirrus CI are failing, because it would need to be tested with the updated libcxxrt containing this PR: https://github.com/libcxxrt/libcxxrt/pull/1.

The tests on Azure seem to be using the C++ standard library instead.

davidchisnall commented 4 years ago

Please can you send the PR to libcxxrt/libcxxrt? We moved the canonical upstream for libcxxrt there a short time ago.

davidchisnall commented 4 years ago

I am quite nervous about committing this, because the fact that it breaks in CI means that it is probably an ABI-breaking change. I'd like to get @emaste's opinion on the correct way to handle this. We probably want to add some conditional compilation both here and in libcxxrt to allow the old ABI to remain supported, and add a check in CMake here for what the C++ runtime library does.

triplef commented 4 years ago

Thanks @davidchisnall, I’d appreciate if you can take a look at this. I can’t say I fully understand in how far libobjc2, libcxxrt, and libcxxabi, and libunwind need to be aligned, but these changes did fix ObjC++ interop for us on x86 and x86_64. The main problem was that for us sizeof(__cxa_exception) didn’t match between libobjc2 and libcxxrt.

I’ve resubmitted the libcxxrt PR to the new repo and updated the links above.

davidchisnall commented 4 years ago

I don't think working around ABI incompatibilities by installing a different version is the correct fix. We should still be doing CI on FreeBSD with the built versions, we need to make sure that we expose an option to select the ABI and, ideally, a test that checks for the correct one.

triplef commented 4 years ago

@davidchisnall I set up the FreeBSD CI now to test against both libcxxrt from FreeBSD and the latest libcxxrt master (which I think makes sense regardless of this PR – let me know if I should submit this as a separate PR). As outlined in https://github.com/libcxxrt/libcxxrt/pull/1#issuecomment-555224884, this would currently only pass against libcxxrt together with https://github.com/libcxxrt/libcxxrt/pull/1.

triplef commented 4 years ago

This was obsoleted by https://github.com/gnustep/libobjc2/commit/242442b3aa1d0e438a6893ba5d8b472c9634d293 and #132.