Support for libc++abi #152
Conversation
Enables using libc++abi: gnustep/libobjc2#152
There was a problem hiding this comment.
Thank you @ngrewe for putting this into a PR! We’ve been using this updated branch for a couple days now on various machines for Android and we haven’t seen any issues.
Unfortunately I don’t feel qualified to really review this, as I know next to nothing about the C++ ABI. I did look through the changes and didn’t see anything standing out to me (just a couple small spelling/grammar issues in the comments – let me know if I should report these), so in that way and as it’s working well for us: lgtm.
Yes please! Thanks for having a look at this... |
|
Oh. I realised just now that Github foobared my last comment quite rigorously 😄 – anyways, thanks again for the just as rigorous copy editing! |
CMake/CMakeLists.txt
Outdated
| if (CXX_ABI_IS_GNU) | ||
| add_definitions(-DCXX_ABI_IS_GNU=1) | ||
| else () | ||
| add_definitions(-DCXX_ABI_IS_GNU=0) |
There was a problem hiding this comment.
Please can you make this path something like CXX_ABI_IS_LLVM? I find this naming slightly confusing because we already support MSVC EH and GNU-flavoured Itanium EH, so having not-GNU mean LLVM-flavoured-Itanium is a bit odd.
azure-pipelines.yml
Outdated
| @@ -3,24 +3,43 @@ jobs: | |||
| displayName: Ubuntu-16.04 | |||
| pool: | |||
| vmImage: ubuntu-16.04 | |||
There was a problem hiding this comment.
I bumped this to 18.04, so may have caused some merge conflicts for you here. Sorry!
|
Thank you very much for working on this. Looking at the vtable layouts, it appears that we ought to be able to just use the GNU one, as long as we don't depend on any symbols. The GNU vtable contains two functions to check if a type is a pointer or function, these are noop1 and noop2 in the LLVM layout and so we can provide the GNU definitions. The third function in both is The fourth function is only present in the GNU definition, so there's no conflict if we provide it. Can we get away with defining our own It would also be nice, if possible, to make this a dynamic check. I am working on doing an initialisation pass that throws a C++ exception through a frame with a personality function that inspects the C++ exception. It would be nice to use the same (or similar) logic to handle this. |
|
#156 has the initial draft of the auto-detection. |
|
FYI: We are now correctly detecting the exception class, but libc++abi support will still require having the correct vtable layout for the catch helpers. |
|
With the latest set of EH fixes, I removed the dependency on a specific vtable layout in I believe that the libc++abi version, because it doesn't call |
|
It would be great to see this updated with the latest changes from David. @ngrewe is there anything I can do to help with this? |
|
I took a stab at merging master into the libc++ branch: Unfortunately it crashes in both on ARM and x86 in __do_catch() when throwing an exception in an ObjC++ file, so I am probably missing some modifications to make it work with the latest EH changes in master: Unfortunately I’m mostly flying in the dark here as I don’t really understand the changes, but if it helps I’d be happy to work on this further if someone can point me in the right direction. FWIW I also had to do add some modifications to get the compilation of eh_trampoline.cc working in a cross-compilation environment with the Android CMake toolchain file, which I think sets CMAKE_CXX_FLAGS as a string instead of a list – I am not sure if this is portable as-is: triplef@2af2a6b We’re currently still using the |
|
@ngrewe, any chance you'll have time to look at merging this with the new infrastructure for better supporting different C++ standard library implementations? We now create a function with a custom personality function and throw a known C++ exception through it the first time a C++ exception is thrown, so we can figure out the layout of the C++ exception objects. |
|
Thanks for pinging me! Things had gotten terribly busy in my neck of the woods and this slipped off the radar. I'll pick it back up sometime this week. |
|
Thanks @ngrewe! Please let me know if there’s anything I can test or help with. |
Thanks so much, @davidchisnall… that was spot on and probably saved me a lot of poking at random things! I think basic detection at runtime is now working fine. I'm now stripping out the old compile-time detection bits from this branch. |
|
Thank you very much @ngrewe! I’m traveling this week and next, but I’m looking forward to try this out as soon as I return. |
|
I've gotten this PR to be delightfully minimal now. Unfortunately, this is now also experiencing the problem with ceilf being undefined, as described in #181 😕 |
|
We probably need to unconditionally link libm if it exists. |
| @@ -49,6 +49,9 @@ class type_info2 : public std::type_info | |||
| virtual bool __do_catch(const type_info *thrown_type, | |||
| void **thrown_object, | |||
| unsigned outer) const { return true; } | |||
| virtual bool __do_upcast( | |||
There was a problem hiding this comment.
Do we actually need this test anymore? I think we're now detecting the C++ runtime at run time, so we should work with any C++ runtime.
There was a problem hiding this comment.
We now work with every runtime, but we still need this test to find out whether there is a usable runtime or if we need to link the full stdlib.
There was a problem hiding this comment.
This change here is needed to prevent a dependency on symbols from the GNU runtime, btw.
|
It looks as if we're linking libc++, not libc++abi. This means that we get libc++'s definition of |
It was actually linked unconditionally in the CI configuration (an oversight on my part), and we also had some cross-talk between the two builds so that the libstdc++ build would pick up libc++abi and try to link it with the GNU C++ ABI. Both should now be fixed (the latter by only trying to libc++abi if libc++ is among the
I've changed the build script to do that. |
|
After getting cross-compilation to work again (#183, not specific to this branch), I am now getting the following errors when building for armeabi-v7a (arm64 and x86/x64 work fine, I’ll test these in a bit): These seem to be defined in libunwind.a for armeabi-v7a. Do we maybe need to link that manually? |
|
I tried explicitly linking libunwind, but I’m still getting the same linker error. Any thoughts? |
|
I tried this in the Android x86 emulator. Throwing and catching an ObjC++ exception works fine the first time, but the second time crashes with this backtrace: This is the log output I am getting: |
Interesting. Is this two separate exceptions being thrown? It looks like we are entering the code path where we let C++ rethrow the exception instead of throwing it ourselves. That fails because the runtime is not aware of any enclosing exception handler. Can you make available the reproducer? |
So just to confirm: There are versions of libc++ (not libc++abi) in the wild which are not already linked against libm? I've been testing mostly with the variant from the llvm repositories for Debian/Ubuntu, and that has that right already:
That would make for a simpler build script, and I'm all for that. |
|
About the |
I think that's the only way that we'd end up with teh build failures that we were seeing.
Yes, I think so. |
This can be removed once gnustep/libobjc2#152 is merged.
|
With #183 out of the way (and after merging master) I’m able to build this for Android on all architectures except armeabi-v7a, where unfortunately I still get the following linker error: These symbols are defined in libunwind.a, but I’ve tried a bunch of things (including explicitly adding -lunwind) and cannot figure out how to get this working. I compared the linker invocations between the last version that was still working for me (d521f61) and this, and there are only two changes:
I also looked through all the changes since that last working release but don’t see what change could be causing this. Any ideas? |
|
The fact that these symbols have parameter types in a linker message suggests that they are C++ mangled names, but they should be C symbols. I think we're probably missing an |
|
You’re a magician! 😀 This patch fixes it: diff --git a/unwind-arm.h b/unwind-arm.h
index 7e2038b..724624e 100644
--- a/unwind-arm.h
+++ b/unwind-arm.h
@@ -1,5 +1,9 @@
#include <stdint.h>
+#ifdef __cplusplus
+extern "C" {
+#endif
+
/**
* ARM-specific unwind definitions. These are taken from the ARM EHABI
* specification.
@@ -207,3 +211,7 @@ _Unwind_Reason_Code name(_Unwind_State state,\
(dst)->barrier_cache = (src)->barrier_cache; \
(dst)->cleanup_cache = (src)->cleanup_cache; \
(dst)->pr_cache = (src)->pr_cache;
+
+#ifdef __cplusplus
+}
+#endifIs it ok if I merge master into this branch and add this change, or should I leave that to @ngrewe? |
|
Can you rebase the branch, rather than merge from master, so we have vaguely comprehensible history? |
|
I tried rebasing, but I’m running into multiple conflicts as the changes from this branch are interleaved with changes in master. Given that the current changes of this branch over master are pretty compact, the most straightforward way might be to squash all changes, but I’d like to leave that to @ngrewe or at least get his consent as we’d loose all history of the changes in this branch. |
|
I think I'd even prefer squashing prior to merging this... |
|
It definitely wants a squash at some point before merging, but it also needs updating for @triplef's fix and a for CI to run. |
|
@ngrewe I’d be happy to squash and rebase this branch (keeping you as the author) if that helps, just let me know. I could also pull out the added ObjCXXEHInteropTwice test into its own PR to fix later as to unblock merging this. Apart from that, I think the only open issue is the following:
Does the following patch look ok to you? diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1d2342a..16928b7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -295,10 +295,6 @@ if (ENABLE_OBJCXX)
list (FIND CMAKE_CXX_IMPLICIT_LINK_LIBRARIES "c++" _libcxx_index)
if (${_libcxx_index} GREATER -1)
test_cxx(c++abi false)
- if (CXX_RUNTIME)
- # When linking libc++abi, try to find libm for linking
- find_library(M_LIBRARY m)
- endif()
endif()
endif (NOT CXX_RUNTIME)
@@ -327,6 +323,9 @@ if (ENABLE_OBJCXX)
MAIN_DEPENDENCY eh_trampoline.cc)
list(APPEND libobjc_ASM_SRCS eh_trampoline.s)
list(APPEND libobjc_CXX_SRCS objcxx_eh.cc)
+
+ # Find libm for linking, as some versions of libc++ don't link against it
+ find_library(M_LIBRARY m)
endif ()
endif (ENABLE_OBJCXX) |
|
Looks good to me, thanks! |
|
It would also be good to add something to |
Extends C++ interop with autodetected support for libc++abi in addition to the existing support for libcxxrt and libsubc++. Fixes #142.
…imes sequentially. This originally came up as an issue with libc++abi support (#152), but is not specific to that ABI.
|
I have squashed the changes, minus the failing test for throwing the same exception twice which was extracted into #188. As for adding this to
|
|
|
|
Sorry, I misunderstood your question. |
|
Most excellent, thank you both! |
…imes sequentially. This originally came up as an issue with libc++abi support (#152), but is not specific to that ABI.
As discussed in #152, use the function defined in the Itanium C++ ABI to check whether the thrown exception is the current caught C++ exception and needs rethrowing via `__cxa_rethrow()`.
…imes sequentially. This originally came up as an issue with libc++abi support (#152), but is not specific to that ABI.
As discussed in #152, use the function defined in the Itanium C++ ABI to check whether the thrown exception is the current caught C++ exception and needs rethrowing via `__cxa_rethrow()`.
…imes sequentially. (#188) This originally came up as an issue with libc++abi support (#152), but is not specific to that ABI. * Use the C++ runtime to check for uncaught C++ exceptions. As discussed in #152, use the function defined in the Itanium C++ ABI to check whether the thrown exception is the current caught C++ exception and needs rethrowing via `__cxa_rethrow()`. Co-authored-by: Niels Grewe <grewe@ocean-insights.com> Co-authored-by: David Chisnall <gnustep@theravensnest.org>
Okay, this is my attempt to fix #142 — I'm still not entirely happy with it, but I think this is now at least in a state where it doesn't break anything else and feels reasonably robust (all things considered). So now seems like a good time as any to solicit feedback.
The first part of this is detection of whether the compiler will use a libc++ flavoured EH ABI or the GNU one. It turns out that this is quite difficult and this PR unfortunately relies on secondary signals to determine which one to use: In general, we will assume that being able to compile
typeinfo_test.ccwith a certain C++ ABI runtime library is a good indication for the ABI: if either libcxxrt and libsupc++ work, this means that we should use the GNU ABI, if libc++abi works, we should use the LLVM one.But that leaves the case where we need to link the standard library in order to get the runtime bits and pieces. To handle this case, we try to compile the
typeinfo_non_gnu.cctest programme, which is set up in a way that it will fail with libstdc++, in which case we assume that we working in a libc++ environment (MSVC is already dealt with beforehand). We also have to keep in mind that there are environments (e.g. FreeBSD), where we use libc++ and still use the GNU ABI, which happens to work because we prefer the result from checking for the C++ runtime libraries.Using these CMake tests, we set up the
CXX_ABI_IS_GNUmacro, which changes a couple of things if it happens to be0:CLNGC++(as opposed toGNUC++for the GNU ABI)can_catchmember as per the LLVM ABI.std::typeinfoclass is changed quite drastically. I consider this the most ugly part of the entire PR: libc++abi made a few implementation decisions that make it hard for libobjc2 to support it. Firstly, it intersperses a__shim_type_infoclass betweenstd::typeinfoand the concrete type info implementations. This is the class defining thecan_catch()member (the GNU equivalent,__do_catch(), is declared directly onstd::typeinfo). Secondly, that class doesn't have a constructor, presumably because the implementors thought that the compiler would always emit the different typeinfo instances directly. For our flavour of ObjC++, this is true only when catching a specific class. For__objc_id_type_info, we call the base class constructor to set up the instance. To support this, thestd::typeinfodeclaration for libc++ includes various bits and pieces that allow us to use its initialiser and still keep the vtable in a shape that it can be used as if inheriting from__shim_type_infoSince that's comparatively hacky, an equally important part of this is the extension of the CI pipeline, which now also builds and tests the libc++ support on Linux (I originally developed and tested this using a OpenBSD VM, but it doesn't seem that there are any free CI services allowing you to build on OpenBSD). That necessitated moving the builds into containers because otherwise there would be cross-pollution between the builds 🙄. Also, since the Cirrus CI build seems to have stopped working with FreeBSD 12.0, I took the liberty of bumping that to 12.1.
Please let me know what you think, I envisage that this will need a few more iterations before it is ready to merge…
Thanks!
Niels