This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Differentiate between static and shared libc++abi
ClosedPublic

Authored by phosek on Apr 2 2019, 12:58 AM.

Details

Summary

This addresses the issue introduced in r354212 which broke the case when
static libc++abi is merged into static libc++, but shared libc++ is
linked against shared libc++. There are 4 different possible
combinations which is difficult to capture using a single variable. This
change splits LIBCXX_CXX_ABI_LIBRARY into two:
LIBCXX_CXX_SHARED_ABI_LIBRARY and LIBCXX_CXX_STATIC_ABI_LIBRARY to
handle the shared and static cases. This in turn allows simplification
of some of the logic around merging of static archives.

Diff Detail

Repository
rCXX libc++

Event Timeline

phosek created this revision.Apr 2 2019, 12:58 AM

I guess that change makes sense overall, some comments from reading the code inline. I didn't apply the patch locally so I might be missing some context here...

libcxx/cmake/Modules/HandleLibCXXABI.cmake
111

This should be CXXABI_STATIC_LIBNAME, no?

libcxx/lib/CMakeLists.txt
62–65

LIBCXX_CXX_SHARED_ABI_LIBRARY?

This makes sense given the issue at hand, but we should really really move towards an approach based on find_package to find our dependencies.

phosek updated this revision to Diff 193333.Apr 2 2019, 11:22 AM
phosek marked 4 inline comments as done.

This makes sense given the issue at hand, but we should really really move towards an approach based on find_package to find our dependencies.

I think that's the best approach long-term, but I'd like to land this one first and make that change as a follow up, since right now our ASan builds that use libc++ are broken because of this issue so I'd like to unbreak them.

libcxx/cmake/Modules/HandleLibCXXABI.cmake
111

I've removed the condition altogether to simplify the logic.

libcxx/lib/CMakeLists.txt
62–65

I've changed the logic so now it's actually correct.

LGTM, though I agree with @ldionne, and I'll wait for him (or another libc++ maintainer) to accept.

libcxx/lib/CMakeLists.txt
64

Not your diff, but CMake reorders static libraries on the link line in dependency order, so I've seen libraries get reordered out of the whole-archive sandwich. I don't know of a good way to handle that generically though ... making the entire sandwich be one giant argument (e.g. "-Wl,--whole-archive,libfoo,--no-whole-archive") breaks when libfoo is a target, since CMake doesn't replace it with the path to the actual library, and using a generator expression breaks when libfoo isn't a target.

(On the same note, if you ever need to debug CMake's library reordering, CMAKE_LINK_DEPENDS_DEBUG_MODE is super useful.)

(to be clear I also think it's fine to unbreak this first and do the find_package change later, but the latter should hopefully end up being much nicer)

ldionne accepted this revision.Apr 2 2019, 12:23 PM
This revision is now accepted and ready to land.Apr 2 2019, 12:23 PM
Hahnfeld accepted this revision.Apr 2 2019, 1:52 PM
Hahnfeld added inline comments.
libcxx/cmake/Modules/HandleLibCXXABI.cmake
119–121

Does it make sense to move these lines into the above if (LIBCXX_CXX_ABI_INTREE)? If I understand the code correctly, LIBCXX_CXX_ABI_INTREE and LIBCXX_CXX_ABI_SYSTEM are mutually exclusive, no?

phosek marked an inline comment as done.Apr 2 2019, 2:40 PM
phosek added inline comments.
libcxx/cmake/Modules/HandleLibCXXABI.cmake
119–121

I'm not sure if that was the intention, but it's not how things are set up right now, e.g. for FreeBSD or default we set neither of these variables. This is probably something we should also consider cleaning up in the future.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 6:32 PM