Page MenuHomePhabricator

[CMake] Split linked libraries for shared and static libc++
ClosedPublic

Authored by phosek on Feb 6 2019, 7:11 PM.

Details

Summary

Some linker libraries are only needed for shared libc++, some only
for static libc++, combining these together in LIBCXX_LIBRARIES can
introduce unnecessary dependencies.

This changes splits those up into LIBCXX_SHARED_LIBRARIES and
LIBCXX_STATIC_LIBRARIES matching what libc++abi already does.

Diff Detail

Repository
rCXX libc++

Event Timeline

phosek created this revision.Feb 6 2019, 7:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 7:11 PM

combining these together in LIBCXX_LIBRARIES and LIBCXX_INTERFACE_LIBRARIES can introduce unnecessary dependencies.

Can you provide an example? And elaborate on the introducing unused libraries on the link line?

libcxx/lib/CMakeLists.txt
53–54

These are flags. Not shared libraries.

53–54

Does this make it an interface library? because that's incorrect.

95–99

If you're renaming this, can you double-check there are no buildbot configurations using it on zorg?

Maybe set up a bot or two? You can use the libcxx-cloud workers.

phosek updated this revision to Diff 188438.Feb 26 2019, 12:18 PM
phosek marked 4 inline comments as done.

combining these together in LIBCXX_LIBRARIES and LIBCXX_INTERFACE_LIBRARIES can introduce unnecessary dependencies.

Can you provide an example? And elaborate on the introducing unused libraries on the link line?

Example is change D49587. Currently, this one is failing

-- Configuring done
CMake Error: install(EXPORT "LLVMRuntimes" ...) includes target "cxx_static" which requires target "cxxabi_static" that is not in the export set.
CMake Error: install(EXPORT "LLVMRuntimes" ...) includes target "cxx_static" which requires target "unwind_static" that is not in the export set.
-- Generating done

The reason for this is because we're no installing static libunwind and libc++abi, instead we're merging them into libc++. This shouldn't be a problem, but add_interface_library(unwind_static) and add_interface_library("${LIBCXX_CXX_ABI_LIBRARY}") adds these as dependencies because it doesn't distinguish between static and shared library dependencies. However, there really isn't any such thing as static library dependency since you cannot link static library against another static library. This change is trying to address this issue by separating the notion of shared library dependency and static library "dependency" which is something that both libunwind and libc++abi build already does.

Does this make sense?

libcxx/lib/CMakeLists.txt
53–54

These are still passed to target_link_libraries just like before since add_library_flags simply adds the string to LIBCXX_LIBRARIES which was passed to target_link_libraries, so the original function name was a misnomer. I could change the name to something like LIBCXX_SHARED_LINK_FLAGS, but these should be still passed to target_link_libraries, not as LINK_FLAGS to set_target_properties.

95–99

I'm not renaming this variable, LIBCXXABI_ENABLE_STATIC_UNWINDER implies both LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY and LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_STATIC_LIBRARY ([using cmake_dependent_option](https://github.com/llvm/llvm-project/blob/master/libcxxabi/CMakeLists.txt#L97)), but each of them can be set separately. I'm making this condition more fine grained to account for that since in this branch we're only interested in the shared case.

smeenai added inline comments.Mar 6 2019, 10:28 PM
libcxx/lib/CMakeLists.txt
53–54

The commit message mentions eliminating LIBCXX_INTERFACE_LIBRARIES, but that doesn't appear to be the case. Did you mean getting rid of LIBCXX_LIBRARIES?

This looks equivalent to add_interface_library if you replaced LIBCXX_LIBRARIES with LIBCXX_SHARED_LIBRARIES in that macro, and it looks like you're also appending to LIBCXX_SHARED_LIBRARIES in all the places you append to LIBCXX_INTERFACE_LIBRARIES, so the macro could still be useful.

349–350

What does this change in practice?

phosek updated this revision to Diff 193415.Apr 2 2019, 7:12 PM
phosek marked 3 inline comments as done.
phosek updated this revision to Diff 193416.Apr 2 2019, 7:20 PM
phosek added inline comments.Apr 2 2019, 7:51 PM
libcxx/lib/CMakeLists.txt
349–350

Just a newer syntax, but it's not necessary for this change so removed.

Is this change still relevant given the recent CMake refactoring?

phosek updated this revision to Diff 195281.Apr 15 2019, 5:59 PM

Is this change still relevant given the recent CMake refactoring?

Yes although it's simpler now.

phosek edited the summary of this revision. (Show Details)Apr 15 2019, 6:01 PM

I think LIBCXX_STATIC_LIBRARIES and LIBCXX_SHARED_LIBRARIES would be better renamed LIBCXX_STATIC_LIBRARY_DEPENDENCIES and LIBCXX_SHARED_LIBRARY_DEPENDENCIES , or something similar. What do you think?

phosek updated this revision to Diff 195492.Apr 16 2019, 5:08 PM

I think LIBCXX_STATIC_LIBRARIES and LIBCXX_SHARED_LIBRARIES would be better renamed LIBCXX_STATIC_LIBRARY_DEPENDENCIES and LIBCXX_SHARED_LIBRARY_DEPENDENCIES , or something similar. What do you think?

I've eliminated that variable altogether by following the pattern we already use for libc++abi.

ldionne accepted this revision.Apr 17 2019, 11:37 AM

Love the cleanup!

This revision is now accepted and ready to land.Apr 17 2019, 11:37 AM
This revision was automatically updated to reflect the committed changes.