Page MenuHomePhabricator

[libcxx] Add support for linking libc++ against a static ABI library.
ClosedPublic

Authored by EricWF on Mar 2 2015, 2:16 PM.

Details

Diff Detail

Event Timeline

EricWF updated this revision to Diff 21039.Mar 2 2015, 2:16 PM
EricWF retitled this revision from to [libcxx] Add support for linking libc++ against a static ABI library..
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added reviewers: mclow.lists, danalbert, jroelofs.
EricWF added a subscriber: Unknown Object (MLST).
danalbert accepted this revision.Mar 2 2015, 2:19 PM
danalbert edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Mar 2 2015, 2:19 PM
compnerd added inline comments.
cmake/Modules/HandleLibCXXABI.cmake
85

Why do you change the name of the library, the name should stay the same, its the extension that changes (and I believe should be implicit based on add_library).

lib/CMakeLists.txt
41

Can you add this in side of a check for the linker type? The options are very GNU centric.

test/CMakeLists.txt
47

Can you explain why this is correct? The tests are meant to test libc++abi. They should link to it either statically or dynamically as built. The libc++ tests however, should not.

EricWF added a comment.Mar 2 2015, 2:49 PM

Address inline comments.

cmake/Modules/HandleLibCXXABI.cmake
85

The actual library name ('libc++abi.so' or 'libc++abi.a') doesn't change. These are CMake target names for when libc++abi and libc++ are being built intree together.

lib/CMakeLists.txt
41

Sounds like a good change. I'll look into it.

test/CMakeLists.txt
47

Because when libc++ statically links to libc++abi then all of the symbols from libc++abi should be available via libc++. If we don't do this and always link the tests to libc++abi then it could hide errors where libc++ doesn't properly expose the required symbols.

EricWF added inline comments.Mar 2 2015, 3:09 PM
lib/CMakeLists.txt
41

To comment on this further. The correct thing to do would be to use find_library with the static library name. However there are some consumers of libc++ that depend on libc++'s CMake not requiring that the library be found at configuration time since it will not yet exist.

This is also the reason why use use LIBCXX_CXX_ABI_LIBRARY_PATH to supply the library's directory and not the full path of the library file.
I'm hoping to change this in future.

compnerd added inline comments.Mar 2 2015, 3:16 PM
CMakeLists.txt
64

Maybe this should be something like LIBCXX_STATIC_LIBCXXABI. Otherwise, we need checks for the case that we are trying to use libsupc++/libcxxrt and don't have a static version available.

74

Any reason to not support this on Darwin really? I think that we can build a DSO for libc++ and just grab the $<TARGET_OBJECTS:c++abi> (or a static archive/library).

cmake/Modules/HandleLibCXXABI.cmake
85

Yeah, thats what happens when you look at a patch set out of order :-p.

test/CMakeLists.txt
47

Agreed, but the libc++abi tests shouldn't try to pull the symbols indirectly from libc++. They should have direct access to libc++abi symbols.

EricWF added inline comments.Mar 2 2015, 3:26 PM
CMakeLists.txt
64

Perhaps. Since the option is OFF by default I don't think we *need* checks. I have no problem with these cases failing to link because the library is missing. If a user enables this option they should know that they have a static version of the library available.

74

Because linking libc++ on Darwin is already a mess and it would need to be cleaned up first. I also don't see the motivation to support this on Darwin because libc++ reexports libc++abi.dynlib.

test/CMakeLists.txt
47

This shouldn't affect the libc++abi tests because they don't use the lit.site.cfg generated by libc++.

EricWF updated this revision to Diff 21056.Mar 2 2015, 3:53 PM
EricWF edited edge metadata.

More cleanup and comments.

EricWF closed this revision.Mar 3 2015, 8:02 AM