There's a lot of CMake logic that's only relevant to the shared library,
yet it was using a code path and setting variables that impact both the
shared and the static libraries. This patch moves this logic so that it
clearly only impacts the shared library.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCXX libc++
Event Timeline
Comment Actions
I saw https://reviews.llvm.org/D57872 and I thought to myself: let's just eradicate all these variables that hold lists of libraries. The better approach is to set those flags/libraries-to-link-against directly on the targets. This change refactors some of this mess for shared libraries.
Comment Actions
I really like the direction this is going.
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 177 ↗ | (On Diff #193742) | target_link_options was added in CMake 3.13, so you're gonna have to use target_link_libraries to specify linker options (which is unfortunate, but our minimum CMake version is 3.4.3). How come you're using a generator expression here whereas the previous code was just using LIBCXX_CXX_STATIC_ABI_LIBRARY directly? The need for that might go away with the switch to target_link_libraries. |
| 185 ↗ | (On Diff #193742) | We were previously using add_interface_library for this, which would populate the list of libraries used to form the libc++ linker script. I don't see a replacement for that logic here? |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 177 ↗ | (On Diff #193742) |
Will do. Out of curiosity, what/who decides what's the minimal version of CMake we have to support? CMake being one of the easiest tools to update on any system, I'd argue we should have a more aggressive stance towards requesting a recent version.
It didn't work anymore with target_link_options, probably because it doesn't expand target names into their TARGET_LINKER_FILE. Changing to target_link_libraries removes the need for it, but I'd like to keep it because it's more precise (it doesn't rely on CMake magic under the hood). Changing to target_link_libraries also removes the need for the add_dependencies call below, so I'll remove that one. |
| 185 ↗ | (On Diff #193742) | When I wrote my patch I originally added it explicitly to LIBCXX_INTERFACE_LIBRARY_NAMES as: list(APPEND LIBCXX_INTERFACE_LIBRARIES "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")but eventually I decided it wasn't necessary. I can't remember why now -- maybe I was wrong. I'll put it back. |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 179 ↗ | (On Diff #193758) | Same comment here: target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic,$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>,-Bdynamic,--no-whole-archive")(or separated out) target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive -Wl,-Bstatic $<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}> -Wl,-Bdynamic -Wl,--no-whole-archive") |
| 184 ↗ | (On Diff #193758) | I believe this should be part of the else() above. |
| 200 ↗ | (On Diff #193758) | I'd combine these two into a single argument |
| 201 ↗ | (On Diff #193758) | Same here |
| 177 ↗ | (On Diff #193742) | I'd agree on being more aggressive with CMake upgrades. CC @beanz One problem with the generator expression is that it assumes ${LIBCXX_CXX_STATIC_ABI_LIBRARY} is a target, whereas it could in theory be the libc++abi system library. I don't know if anyone actually cares about the use case of statically linking the system libc++abi (assuming your system even has a static libc++abi) into libc++ though. Since you're using the generator expression, could you switch to target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load,$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>")That'll prevent CMake's library reordering from misplacing the argument to the force_load. (Alternatively, if you find combining -Wl arguments too ugly, the following should be fine too: target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load $<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>") |
Comment Actions
Address smeenai's comments
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 179 ↗ | (On Diff #193758) | Note that this requires re-adding the add_dependencies call, because CMake doesn't see the implicit dependency anymore. |
| 184 ↗ | (On Diff #193758) | You're right. My intent was that the linker script would work too when statically linking libc++abi into libc++, but actually it's the other way around. When you statically link libc++abi.a into libc++.so, you don't want the linker script to try to link against libc++abi. |
| 201 ↗ | (On Diff #193758) | Note that both used to be, but I had to change it when I was using target_link_options because the quoting was different. |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 183 ↗ | (On Diff #193767) | Sorry, I just realized a potential issue here. At least with the Ninja generator, this adds an order-only dependency from cxx_shared to LIBCXX_CXX_STATIC_ABI_LIBRARY, not a full dependency. In other words, if you change a source file for LIBCXX_CXX_STATIC_ABI_LIBRARY and then rebuild cxx_shared, LIBCXX_CXX_STATIC_ABI_LIBRARY will get rebuilt, but cxx_shared won't be relinked. I'm not sure if there's any way to get the full dependency other than just using target_link_libraries directly :( |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 183 ↗ | (On Diff #193767) | Well, there's LINK_DEPENDS, but it's only available for Makefile and Ninja generators. It looks like at least the Xcode generator doesn't have the order-only dependency issue though. |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 321 ↗ | (On Diff #193767) | Have you considered moving this block into the if(LIBCXX_ENABLE_SHARED) above as well? AFAIK this is the only use of LIBCXX_INTERFACE_LIBRARIES so we might be able to eliminate that variable altogether. |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 183 ↗ | (On Diff #193767) | Okay, I'm using the original target_link_libraries line where everything is split. We shouldn't have issues with de-duplication of linker flags for libc++abi.dylib. |
| 321 ↗ | (On Diff #193767) | I moved the block, but I'll see if it's possible to eliminate LIBCXX_INTERFACE_LIBRARIES separately. |
Comment Actions
LGTM. Thanks for doing this!
You may wanna wait for @phosek as well, since he's been doing a bunch of build work here too.
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 177 ↗ | (On Diff #193907) | Ah, so CMake adds the implicit dependency even with the generator expression form, as long as the generator expression is its own argument? That's useful. I kinda prefer not having the generator expression here, just cos it's standard CMake convention to just use a target name directly, and cos then it seems more intuitive that the dependency gets added. I'm not gonna lose any sleep if this goes in as-is though. |
| lib/CMakeLists.txt | ||
|---|---|---|
| 183 | This should be LIBCXX_CXX_SHARED_ABI_LIBRARY instead. This is fixed by r357818 (which addresses a related build bot failure). | |
| lib/CMakeLists.txt | ||
|---|---|---|
| 179 | This broke my compilation of libcxx for mingw (where I statically link in libc++abi into it), with the following error: CMake Error at lib/CMakeLists.txt:179 (target_link_libraries):
Error evaluating generator expression:
$<TARGET_LINKER_FILE:c++abi>
No target "c++abi"I'm not really familiar enough with this aspect of CMake to know what is wrong here and what's expected. Prior to this patch, the corresponding code elsewhere in the same CMakeLists.txt did this: add_library_flags("-Wl,--whole-archive" "-Wl,-Bstatic")
add_library_flags("${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
add_library_flags("-Wl,-Bdynamic" "-Wl,--no-whole-archive") | |
| lib/CMakeLists.txt | ||
|---|---|---|
| 179 | How do you point the build to the static libc++abi? Is it not built in the tree? | |
| lib/CMakeLists.txt | ||
|---|---|---|
| 179 | No, it's not built in the same tree, I build libunwind/libcxxabi/libcxx standalone separately. I configure cmake with -DLIBCXX_CXX_ABI=libcxxabi \ -DLIBCXX_CXX_ABI_INCLUDE_PATHS=../../libcxxabi/include \ -DLIBCXX_CXX_ABI_LIBRARY_PATH=../../libcxxabi/build-$arch-$type/lib \ which up until now has worked fine for finding and using it. | |
This broke my compilation of libcxx for mingw (where I statically link in libc++abi into it), with the following error:
CMake Error at lib/CMakeLists.txt:179 (target_link_libraries): Error evaluating generator expression: $<TARGET_LINKER_FILE:c++abi> No target "c++abi"I'm not really familiar enough with this aspect of CMake to know what is wrong here and what's expected. Prior to this patch, the corresponding code elsewhere in the same CMakeLists.txt did this:
add_library_flags("-Wl,--whole-archive" "-Wl,-Bstatic") add_library_flags("${LIBCXX_CXX_STATIC_ABI_LIBRARY}") add_library_flags("-Wl,-Bdynamic" "-Wl,--no-whole-archive")