This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Support statically linking dependencies only to shared or static library
ClosedPublic

Authored by phosek on Jul 18 2018, 12:01 PM.

Details

Summary

Currently it's possible to select whether to statically link unwinder
or the C++ ABI library, but this option applies to both the shared
and static library. However, in some scenarios it may be desirable to
only statically link unwinder and C++ ABI library into static C++
library since for shared C++ library we can rely on dynamic linking
and linker scripts. This change enables selectively enabling or
disabling statically linking only to shared or static library.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jul 18 2018, 12:01 PM

This came up in Fuchsia. Clang driver, add -lc++ as a dependency. This is sufficient for shared libc++, but for static user has to manually pass -lc++abi -lunwind, so for static libc++ we would actually like to combine both libc++abi and libunwind into a single archive.

phosek added inline comments.Jul 18 2018, 12:09 PM
libcxx/CMakeLists.txt
158 ↗(On Diff #156127)

Maybe LIBCXX_ENABLE_STATIC_ABI_LIBRARY_IN_STATIC_LIBRARY would be more accurate?

ldionne added inline comments.Jul 18 2018, 12:20 PM
libcxx/CMakeLists.txt
158 ↗(On Diff #156127)

LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY and LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY?

And then LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_STATIC_LIBRARY/LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY.

libcxx/lib/CMakeLists.txt
269 ↗(On Diff #156127)

I don't understand why any of this needs to change -- can you please explain? Also, you probably didn't mean to leave the commented-out lines.

phosek updated this revision to Diff 156201.Jul 18 2018, 6:44 PM
phosek marked 2 inline comments as done.
phosek added inline comments.Jul 18 2018, 6:49 PM
libcxx/lib/CMakeLists.txt
269 ↗(On Diff #156127)

The reason this change is needed the case when we're linking shared libc++abi into shared libc++ in which case ${LIBCXX_CXX_ABI_LIBRARY} will be set to cxxabi_shared in HandleLibCXXABI.cmake but we cannot merge libc++abi.so into libc++.a, so instead we force the use of cxxabi_static here.

Alternatively, we could modify HandleLibCXXABI.cmake to set two dependencies, one for the static case and one for the shared case and use the former one here.

Removed the commented out lines.

ldionne added inline comments.Jul 19 2018, 6:54 AM
libcxx/lib/CMakeLists.txt
269 ↗(On Diff #156127)

Thanks. There's something I still don't understand. If you are linking the ABI library dynamically, why would you want to merge it (well, the static version of it) into libc++.a? It seems like this is somewhat defeating the purpose of dynamically linking against the ABI library, no?

libcxxabi/src/CMakeLists.txt
64 ↗(On Diff #156201)

Does this not need to change anymore? I think we'd have to set different flags for cxxabi_shared and cxxabi_static.

phosek added inline comments.Jul 19 2018, 2:28 PM
libcxx/lib/CMakeLists.txt
269 ↗(On Diff #156127)

In our case, we want to link shared library dynamically and merge all static libraries, so LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY=OFF and LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON. This means that LIBCXX_CXX_ABI_LIBRARY will be set to cxxabi_shared and it's what will be used other places throughout this file as the interface library. However, we cannot merge libc++abi.so into libc++.a so that's why we have to explicitly select cxxabi_static here rather than simply using LIBCXX_CXX_ABI_LIBRARY as before.

Like I mentioned in the previous comment, alternative would be to split this into two variables, e.g. LIBCXX_CXX_SHARED_ABI_LIBRARY and LIBCXX_CXX_STATIC_ABI_LIBRARY, would you prefer that approach?

libcxxabi/src/CMakeLists.txt
64 ↗(On Diff #156201)

This is relying on the fact that LIBCXXABI_LIBRARIES isn't used for library merging that's done on lines 158-162 and CMake should ignore shared library dependencies passed to target_link_libraries when building static library as done on line 163 (since linking dynamic library target against static library doesn't make sense). This seems to be working fine in my testing, but I'm also OK splitting this into two variables if you want it to be explicit.

ldionne added inline comments.Jul 19 2018, 3:00 PM
libcxx/lib/CMakeLists.txt
269 ↗(On Diff #156127)

Yes, I understand why the TARGET_LINKER_FILE must also be cxxabi_static. What I don't understand is why the conditions need to change, i.e. why you're going from

if (TARGET ${LIBCXX_CXX_ABI_LIBRARY} OR
    (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND HAVE_LIBCXXABI))

to

if (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?"
        AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))

Note that I do think your new condition make more sense (since if the ABI library is not a target, we should fall back on the else() branch). I just want to understand why you're changing it.

Like I mentioned in the previous comment, alternative would be to split this into two variables, e.g. LIBCXX_CXX_SHARED_ABI_LIBRARY and LIBCXX_CXX_STATIC_ABI_LIBRARY, would you prefer that approach?

I don't think that is necessary at this point.

libcxxabi/src/CMakeLists.txt
64 ↗(On Diff #156201)

I would rather see it handled explicitly.

phosek updated this revision to Diff 156416.Jul 19 2018, 6:34 PM
phosek marked 8 inline comments as done.
phosek added inline comments.Jul 19 2018, 6:44 PM
libcxx/lib/CMakeLists.txt
269 ↗(On Diff #156127)

You're right, that's a leftover from previous iterations.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 22 2018, 9:20 PM
This revision was automatically updated to reflect the committed changes.