Page MenuHomePhabricator

[cmake] Split linked libraries into private & public, for linker script
ClosedPublic

Authored by mgorny on Sep 28 2016, 1:51 AM.

Details

Summary

Introduce LIBCXX_LIBRARIES_PUBLIC in addition to LIBCXX_LIBRARIES that
holds 'public' interface libraries -- that is, libraries that both
libc++ links to and programs linked against it need to link to.

Currently this includes the ABI library and optionally -lunwind (when
LIBCXXABI_USE_LLVM_UNWINDER is on). The libraries are included in the
linker script, in order to make it possible to link C++ programs using
clang with compiler-rt runtime out-of-the-box.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 72777.Sep 28 2016, 1:51 AM
mgorny retitled this revision from to [libcxx] Include unwinder library in the linker script.
mgorny updated this object.
mgorny added a reviewer: EricWF.
mgorny added a subscriber: cfe-commits.

The patch generally makes sense to me.

I wonder if the test configuration can also benefit from this; test/libcxx/test/target_info.py manually updates the link line to use -lgcc_s or -lunwind depending on how the build is configured. Presumably, this workaround will no longer be needed if the linker script does that for us?

(Note:- -lgcc_s is linked twice there to satisfy some weird dependency, it's not a mistake)

The patch generally makes sense to me.

I wonder if the test configuration can also benefit from this; test/libcxx/test/target_info.py manually updates the link line to use -lgcc_s or -lunwind depending on how the build is configured. Presumably, this workaround will no longer be needed if the linker script does that for us?

I was originally thinking about that but we don't use the linker script on APPLE and I have no clue how that system is handled or whether it suffers from the same issue at all.

(Note:- -lgcc_s is linked twice there to satisfy some weird dependency, it's not a mistake)

Thanks for letting me know. I almost prepared a patch for that ;-D.

Side note: I see that gcc is solving similar problem via .spec files for its libraries, e.g.:

$ cat /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/libgomp.spec 
# This spec file is read by gcc when linking.  It is used to specify the
# standard libraries we need in order to link with libgomp.
*link_gomp: -lgomp %{static: -ldl }

If we wanted to be more portable, I guess we could establish something similar for clang, and install such a file along with libc++. However, I don't know if that's a problem.

mgorny planned changes to this revision.Oct 2 2016, 4:21 AM

I will be reworking this to use CMake's PRIVATE and PUBLIC linked libs consistently across the build, linker scripts and possibly tests.

@EricWF, would you prefer if I killed the add_library_flags* macros or extended them to support specifying PRIVATE and PUBLIC libs separately?

mgorny updated this revision to Diff 73840.Oct 6 2016, 12:56 PM
mgorny retitled this revision from [libcxx] Include unwinder library in the linker script to [cmake] Split linked libraries into private & public, for linker script.
mgorny updated this object.

Here's my patch v2. I have to admit it's not very different from the original one but the goal is to provide a more generic interface for future improvement.

EricWF added inline comments.Oct 7 2016, 4:35 PM
lib/CMakeLists.txt
41 ↗(On Diff #73840)

Please handle the special case for Apple here, where it manually re-exports the ABI lib.

86 ↗(On Diff #73840)

I don't think libgcc_s should be considered a public library like libunwind because both Clang and GCC link it as a default system library.

179 ↗(On Diff #73840)

Should the libraries be added using target_link_libraries(foo PRIVATE ...) and target_link_libraries(foo PUBLIC ...) now?

mgorny marked 2 inline comments as done.Oct 8 2016, 12:10 AM
mgorny added inline comments.
lib/CMakeLists.txt
86 ↗(On Diff #73840)

However, clang doesn't add it if you use compiler-rt. In which case you are left without an unwinder.

mgorny updated this revision to Diff 74019.Oct 8 2016, 12:11 AM

Apple re-export and PRIVATE/PUBLIC covered.

mgorny updated this revision to Diff 74020.Oct 8 2016, 2:28 AM
mgorny updated this object.
mgorny marked 2 inline comments as done.

As discussed on IRC, limited the unwinder deps to -lunwind, and I'll work on making clang include -lunwind by default.

EricWF accepted this revision.Oct 8 2016, 2:51 AM
EricWF edited edge metadata.

One concern I have is being able to specify libraries in the correct order when we have two different lists. However I'm not concerned enough not to try it.

LGTM.

lib/CMakeLists.txt
43 ↗(On Diff #74020)

Urg. I think this whole setup is borked on Apple. This was an existing issue though, so you don't have to fix it. this LGTM for now.

This revision is now accepted and ready to land.Oct 8 2016, 2:51 AM
This revision was automatically updated to reflect the committed changes.

Hi Michal,

this currently breaks for me: -lunwind will find the system default libunwind.so.8 instead of LLVMs libunwind.so.1 which breaks some tests.
This has worked magically before because libc++ and libc++abi where explicitely linked against LLVMs libunwind.so.1. Could we force that in the linker script as well?

Thanks,
Jonas

I think it'd be technically possible to force a specific SONAME in the linker script but I don't think that's a good idea. Clang should be able to use any libunwind implementation, and libc++ shouldn't really be encoding internal implementation details to the point of specific SONAME.

On the other hand, if we are doing a complete in-tree build of LLVM with libunwind, I guess it'd be reasonable to avoid combining the system library with the just-built library. Assuming we want to force 'our' library, and considering the fact that the tests are specifying all dependencies explicitly anyway, I think the most correct solution for that would be to skip the linker script for tests.

I see two possible solutions for that: either we pass full SONAME of the library when linking tests, or we provide an additional symlink to skip linker script. Gentoo already does the latter, and I wanted to add support for that into libcxx anyway, so that may be the solution. The idea is that besides libc++.so (the linker script), additional libc++_shared.so symlink is installed that references the library directly.

LIBCXXABI_USE_LLVM_UNWINDER implies to me: "Use LLVM's libunwind whenever you use libc++abi". This has worked until now and I would vote for this to be the right thing to do.

joerg added a subscriber: joerg.Oct 24 2016, 7:14 AM

So the historical reason for the libgcc_s dance for glibc was the use of unwinding for thread cancellation. With all the glibc versions I can find, going back to SLES11 SP1, this is no longer the case. To check for yourself, look for _Unwind* symbols in libc.so and libc.a. Only findfde seems to be exposed now.

As such, I think we can avoid all this complexity.

What are the possibilities to proceed here?

What are the possibilities to proceed here?

To be honest, I don't have any good idea besides renaming the library. Keeping two non-interchangeable libraries under the same name is bound to fail.

Please disregard the noise, I think the error lays in the testing of compiler-rt which clears LIBRARY_PATH and therefore doesn't find the right libunwind!