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

[Eric Fiselier via cfe-commits]

EricWF added inline comments.

================
Comment at: src/CMakeLists.txt:37
@@ +36,3 @@
+
+remove_flags(-Wl,-z,defs)
+
----------------
compnerd wrote:
> 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.
That seems fairly reasonable. I didn't think of using `--allow-shlib-undefined` since that was already the default behavior. However so long as it actually works it seems like a good solution.

================
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})
----------------
compnerd wrote:
> 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.
libgcc_eh is only provided as a static library for static linking, That's the intent of the library [1][2].

When we are building libc++abi we link with `-nodefaultlib` so we don't need to worry about the driver messing with the link line.

More importantly, the link line I create here is meant to mimic what clang already does. On linux both GCC and clang will make sure "-lgcc_s" appears after "-lc++" but before "-lc" and "-lpthread" (as well at at the end of the link line). 

I think this is needed because libc and libpthread contain their own definitions of the _Unwinding symbols in order to support  C++ destructors in `pthread_exit` and `pthread_cancel`.  
It appears that libc's definition of _Unwind_resume attempts to dispatch to libgcc_s via dlopen. [3]

I suspect this is the reason for the deliberate placement and double linking "-lgcc_s". 

I think putting "-lgcc_s" before "-lc" is needed to ensure that libraries like libc++abi resolve their _Unwind symbols in libgcc and not libc.


[1] https://gcc.gnu.org/ml/gcc-patches/2005-02/msg00541.html
[2] https://gcc.gnu.org/ml/gcc/2012-03/msg00104.html
[3] http://osxr.org/glibc/source/sysdeps/gnu/unwind-resume.c?v=glibc-2.18


http://reviews.llvm.org/D15440