This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by EricWF on Dec 10 2015, 8:26 PM.

Details

Diff Detail

Event Timeline

EricWF updated this revision to Diff 42495.Dec 10 2015, 8:26 PM
EricWF retitled this revision from to [libc++abi] Use libgcc and libgcc_s to provide _Unwind symbols instead of libgcc_eh.a.
EricWF updated this object.
EricWF added a subscriber: cfe-commits.
compnerd added inline comments.Dec 10 2015, 10:54 PM
cmake/config-ix.cmake
45

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.

src/CMakeLists.txt
37

Do we need to worry about an alternative spelling of -z defs?

EricWF added inline comments.Dec 10 2015, 11:57 PM
cmake/config-ix.cmake
45

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).

src/CMakeLists.txt
37

Not sure. I only considered the spellings used within the LLVM source tree llvm/cmake/modules/HandleLLVMOptions.cmake.

compnerd added inline comments.Dec 11 2015, 8:26 AM
cmake/config-ix.cmake
45

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).

src/CMakeLists.txt
37

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.

62

Why does gcc_s need to be first? Is there some symbol that needs to be resolved from it and not another provider?

EricWF added inline comments.Dec 12 2015, 3:04 AM
cmake/config-ix.cmake
45

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.

src/CMakeLists.txt
37

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?

62

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.

compnerd added inline comments.Dec 12 2015, 8:35 AM
cmake/config-ix.cmake
45

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.

src/CMakeLists.txt
37

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.

62

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.

EricWF added inline comments.Dec 12 2015, 12:55 PM
src/CMakeLists.txt
37

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.

62

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

EricWF added inline comments.Dec 12 2015, 1:32 PM
src/CMakeLists.txt
37

After looking at documentation it would appear that --allow-shlib-undefined doesn't do what we though.

http://stackoverflow.com/questions/2356168/force-gcc-to-notify-about-undefined-references-in-shared-libraries

EricWF updated this revision to Diff 42772.Dec 14 2015, 2:16 PM
EricWF updated this object.

This patch is updated in the following ways:

  1. No longer try and remove -Wl,-z,defs from the link flags. I think I was mistaken that this caused an issue.
  2. No longer try and link libgcc_s before and after libc. It is only linked after libc now. I don't think this will cause issues when building a shared library.
  3. Reorder pthread before libc.
compnerd accepted this revision.Dec 14 2015, 2:18 PM
compnerd edited edge metadata.
This revision is now accepted and ready to land.Dec 14 2015, 2:18 PM
EricWF closed this revision.Dec 14 2015, 2:23 PM
This revision was automatically updated to reflect the committed changes.