[PATCH] D15440: [libc++abi] Use libgcc and libgcc_s to provide _Unwind symbols instead of libgcc_eh.a

[Saleem Abdulrasool via cfe-commits]

compnerd added inline comments.

================
Comment at: cmake/config-ix.cmake:45
@@ -44,3 +44,3 @@
 check_library_exists(pthread pthread_once "" LIBCXXABI_HAS_PTHREAD_LIB)
-check_library_exists(gcc_eh _Unwind_GetRegionStart "" LIBCXXABI_HAS_GCC_EH_LIB)
+check_library_exists(gcc_s __gcc_personality_v0 "" LIBCXXABI_HAS_GCC_S_LIB)
 check_library_exists(c __cxa_thread_atexit_impl ""
----------------
EricWF wrote:
> compnerd wrote:
> > EricWF wrote:
> > > compnerd wrote:
> > > > Might be nice to extend this further to allow building against clang_rt.builtins.  We could of course do that as a follow up if you prefer.
> > > I don't think the clang_rt.builtin libraries are *ever* along the library search path. Maybe we could detect if clang accepts `--rtlib <name>`? (I don't know the flag off hand).
> > Pretty close with the flag: it is `--rtlib=` if you like the 2 dash version.  Hmm, though the problem with the symbol we are using for gcc_s here is the crus of the issue I think.  The problem is that if clang_rt.builtins is used, `__gcc_personality_v0` will be defined since the symbol is the C personality routine.  Perhaps a compromise may be to change the symbol we use to detect gcc_s.  However, given that this was the original behavior, Im not as concerned, since it has worked for most people so far (and its easy to modify).
> Another problem is that `--rtlib=compiler-rt` doesn't work when `-nodefaultlibs` is given.  We can try and manually find libclang_rt.builtins-<target>.a but that requires us to 1) find clangs library directory 2) Explicitly deduce the name of the actual library file based on the target triple.
> 
> I've done this in libc++ for some more extreme cases but it is not fun.
> 
> I would like to support using clang_rt.builtins but we might need to add some compiler support to help us find it.
We should definitely fix that: `gcc -print-file-name=libgcc.a` should be rewritten to `clang -print-file-name=libclang_rt.buitlins-${ARCH}.a` and we can use that :-).  Ill see if I can fix that.

================
Comment at: src/CMakeLists.txt:37
@@ +36,3 @@
+
+remove_flags(-Wl,-z,defs)
+
----------------
EricWF wrote:
> compnerd wrote:
> > EricWF wrote:
> > > compnerd wrote:
> > > > Do we need to worry about an alternative spelling of `-z defs`?
> > > Not sure. I only considered the spellings used within the LLVM source tree `llvm/cmake/modules/HandleLLVMOptions.cmake`.
> > This is a change in the original behavior.  I think it may be safer to add the `-z defs` and `-zdefs` spellings as well if you want the no undefined symbols behavior.  At least on solaris, I believe that `-no-undefined` is also going to cause this to be emitted.
> > This is a change in the original behavior.
> 
> Yes it is. libc++abi.so used to resolve the missing _Unwind symbols in libgcc_eh. However I'm hesitant to use `remove_flags` more than we need to because it's really dumb. For example calling `remove_flags(-pedantic)` on "-Wno-pedantic -pedantic-errors -pedantic" will result in the string "-Wno- -errors".  For this reason I think its safest to only handle the spelling LLVM uses.
> 
> Users shouldn't be passing any spelling of "-Wl,-zdefs" to the libc++abi build. I feel like this is one of those instances of "Doctor it hurts when I do this!".
> 
> Has this swayed your opinion at all? 
It has, only in a slightly different direction.  Why not use `--allow-shlib-undefined` instead?  That way we can insert the flag, which would actually mean that we wouldn't need to filter, and by adding it to the end, we don't need to worry about the flags from LLVM/users.

================
Comment at: src/CMakeLists.txt:62
@@ -44,1 +61,3 @@
+# first and last library on the link line. This behavior mimics clang's
+# behavior.
 set(libraries ${LIBCXXABI_CXX_ABI_LIBRARIES})
----------------
EricWF wrote:
> compnerd wrote:
> > Why does gcc_s need to be first?  Is there some symbol that needs to be resolved from it and not another provider?
> Woops. It doesn't need to be first, but it needs to come before the builtin C libraries. In particular libpthread and libc.  This is because libc (and maybe pthread) can have their own definitions for some  of the _Unwind functions found in libgcc_s.  Because we want `ld` to always choose the version in libgcc_s we need to put it first.
> 
> Here is a quote from a bug report I found:
> 
> >If by some circumstance (use of -Bdirect, -z lazyload, maybe others)
> > libc.so.1 happens to be searched by ld.so.1 before libgcc_s.so.1 and
> > some library (e.g. libstdc++.so.6) uses functions both from GCC_3.0
> > (then resolved from libc.so.1) and others (resolved from libgcc_s.so.1),
> > crashes result due to mixing those different implementations with
> > different internal data structures.
> 
> http://patchwork.ozlabs.org/patch/312087/
> 
> I'm not sure if we will run into any of these cases when building libc++abi  but it seems best to mimic what the linux linker does with libgcc_s.
Because they may statically link against gcc_eh, I assume, which when not done with a rebuild of the system, you get this "beautiful" behavior.  Ill just throw in an "ugh" and look the other way.  However, I don't think that you will be able to inject the link request in the right location since the -lc is inserted by the driver, and the request that you put in will not be hoisted before that -lc.  So, you may still end up with cross-library references.  Im not sure what the best option here is.


http://reviews.llvm.org/D15440