Differential D120719
[runtimes] Always configure libc++abi before libc++ Authored by ldionne on Mar 1 2022, 6:19 AM.
Details
That makes it possible to reuse libc++abi targets from the libc++ This patch requires tweaking how libc++abi knows whether to enable Also, as a fly-by fix, make sure that the libc++ CI pipeline is triggered
Diff Detail
Event TimelineComment Actions Play around with the MinGW configuration based on discussion with @mstorsjo. I suspect this is going to pass in the CI, but it will also cause anyone building for WIN32
Comment Actions I’m generally opposed to making anything in our CMake order-dependent. There are very few situations where order dependence is really required. ‘If(TARGET …)’ requires ordering, but usually you don’t actually need to check if a target exists, you can just check the configuration variables that would result in the target existing. Comment Actions I posted a different patch that fixes the issue without breaking users existing workflows without warning in D120982. Comment Actions Remove Windows workaround and handle compiler-rt like we handle libc++ and libc++abi. Comment Actions I agree that we should strive for order-independence. However, all CMake projects are inherently order-dependent to some extent. For example, we use add_subdirectory() and expect that it's going to set up some targets, which we then use after the call to add_subdirectory(). It's just how CMake works -- it is not sufficiently declarative to be fully order independent. I am not a huge fan of this either, but I just don't see how to work around that. In a sense, this patch actually removes some order dependency by pinning a specific order. With this patch, we are closer to having libc++abi be a subdirectory of libc++ that we'd add with add_subdirectory to then be able to access its targets. When the user specifies LLVM_ENABLE_RUNTIMES=foo1;foo2;foo3, they don't actually care about the order in which they specify it, and I think we should do whatever is needed to make it work regardless of the order the user chooses. By pinning down the order, we are a bit closer to that.
Comment Actions I'm curious what you mean by this. There are lots of ways that CMake allows you to reference targets before they are defined. The only things you really can't do are check if a target exists and query target properties. Many places where you do those two things can be converted to generator expressions and resolved at generation time instead. Comment Actions For instance, in D120727, I do: if (TARGET cxxabi_shared) add_library(libcxx-abi-shared ALIAS cxxabi_shared) endif() Assuming we don't enforce that libc++abi is configured before libc++, it's possible that cxxabi_shared doesn't exist when we get to that line. If CMake were a bit more lazy, I could just drop the if (TARGET and write this instead: add_library(libcxx-abi-shared ALIAS cxxabi_shared) But in the current state of things, CMake fails to configure because it doesn't know about cxxabi_shared yet: CMake Error at /Users/ldionne/code/llvm/libcxx/cmake/Modules/HandleLibCXXABI.cmake:115 (add_library): add_library cannot create ALIAS target "libcxx-abi-shared" because target "cxxabi_shared" does not already exist. Call Stack (most recent call first): /Users/ldionne/code/llvm/libcxx/CMakeLists.txt:509 (include) I don't really know how to work around that. Also note that I don't want to do add_library(libcxx-abi-shared INTERFACE) target_link_libraries(libcxx-abi-shared INTERFACE cxxabi_shared) or something similar, because I need the original properties of cxxabi_shared to be forwarded through (for example I need to be able to get the TARGET_LINKER_FILE of libcxx-abi-shared). Comment Actions if(TARGET...) and ALIAS targets do require order, but I'm not sure why you need to use those constructs. There is a TARGET_EXISTS generator expression that can be used to eliminate many ordering cases, and generator expressions are all late-evaluated. You can query most target properties through generator expressions too.
I don't fully get the context here of what you're trying to do, but this is very much possible with generator expressions. Comment Actions Do you know how I can create a target libcxx-abi-shared such that the TARGET_LINKER_FILE of libcxx-abi-shared is $<TARGET_LINKER_FILE:cxxabi_shared> even if cxxabi_shared doesn't exist yet? If I could do that, that would indeed solve my problem, I think. Looking at the CMake documentation, I can't find a target-level property like LINKER_FILE that I could set for that to work. Comment Actions I don’t understand why you need a target? What is the context you’re relying on that where a generator expression on cxxabi_shared isn’t sufficient? Comment Actions I need to set up an alias target because I want to be able to use the name libcxx-abi-shared to refer to "whichever ABI library was selected by the user". In some circumstances, that ABI library is cxxabi_shared (i.e. the just-build libc++abi library). In other contexts, it's an entirely different thing, for example an imported library for some system-installed ABI library (e.g. libcxxrt). In all those cases, I need to be able to use TARGET_LINKER_FILE, and for that I need an alias library, not an interface library that links against cxxabi_shared. Comment Actions Rebase onto main. I've attempted to solve this problem in various other ways, and they are all greatly inferior to simply enforcing a canonical order here. If CMake could handle ALIAS targets where the aliased target isn't defined yet, that would be a nice way to solve this problem. However, I don't think it's possible to support LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY and friends for arbitrary ABI libraries without this fix, so I'd like to move forward with it. Comment Actions Gentle ping on this @phosek . I spoke to @beanz offline who said he was "neutral" on this change (I guess he doesn't quite like it but also won't push back). This isn't nice, but I think it's a necessary evil and it will allow greatly cleaning up how we pick up the ABI library from within libc++. Does this look OK to you? Comment Actions I'd like to do one additional experiment to see if there was a way to achieve this with generator expressions, I'll report back hopefully within a day if that's fine with you? Comment Actions Yes, that's fine with me. My goal is to be able to land https://reviews.llvm.org/D120727. The way I test D120727 right now is: cmake -S runtimes -B <build> -GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" \
-DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON \
-DLIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY=ON \
-DLIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ONWithout this patch, it fails due to cxxabi_shared not being a known target when we configure libcxx. If we apply this patch, it works. Also note that using LIBCXX_ENABLE_STATIC_ABI_LIBRARY & friends is relevant -- if you remove them, it's easy to implement D120727 without needing this patch right here, you just have to make libcxx-abi-static be an INTERFACE library that links against cxxabi_static (even if it isn't defined yet). But that approach breaks when you use LIBCXX_ENABLE_STATIC_ABI_LIBRARY & friends, since we need to get $<TARGET_LINKER_FILE:libcxx-abi-static>. Comment Actions @ldionne this is what I had in mind: cmake_minimum_required(VERSION 3.13.4) project(Test C) add_library(A STATIC a.c) add_library(C SHARED c.c) target_link_libraries(C PRIVATE "$<GENEX_EVAL:$<TARGET_LINKER_FILE:B>>") add_library(B IMPORTED STATIC) set_target_properties(B PROPERTIES IMPORTED_LOCATION "$<TARGET_LINKER_FILE:A>") It seems to be working as expected in this minimal example. Comment Actions This is really clever, but the problem is that when you do target_link_libraries(C PRIVATE "$<GENEX_EVAL:$<TARGET_LINKER_FILE:B>>"), you lose the property of linking against the B target itself -- you're only linking against a specific .dylib or .a file, but you don't get all the target-level stuff like transitive target_compile_definitions, target_include_directories and so on, right? So for example, below C won't get the includes added by B: cmake_minimum_required(VERSION 3.13.4) project(Test C) add_library(A STATIC a.c) add_library(C SHARED c.c) target_link_libraries(C PRIVATE "$<GENEX_EVAL:$<TARGET_LINKER_FILE:B>>") # This doesn't get `-I some/path/include` add_library(B IMPORTED STATIC) target_include_directories(B INTERFACE "some/path/include") set_target_properties(B PROPERTIES IMPORTED_LOCATION "$<TARGET_LINKER_FILE:A>") Comment Actions Rebase onto main. I'll land this tomorrow if there is no additional discussion. I agree it would be nice to find a solution where we don't need to hardcode the order of libc++ and libc++abi, however the reality is that (1) it doesn't cause all that much harm, and (2) we haven't found a way to achieve D120727 without this ordering. Since D120727 is an important cleanup, I think this is worth doing. Comment Actions You can use the same technique for include directories: cmake_minimum_required(VERSION 3.13.4) project(Test C) add_library(A STATIC a.c) target_include_directories(A PUBLIC "include/a") add_library(C SHARED c.c) target_link_libraries(C PRIVATE "$<GENEX_EVAL:$<TARGET_LINKER_FILE:B>>") target_include_directories(C PRIVATE "$<GENEX_EVAL:$<TARGET_PROPERTY:B,INCLUDE_DIRECTORIES>>") add_library(B IMPORTED STATIC) set_target_properties(B PROPERTIES IMPORTED_LOCATION "$<TARGET_LINKER_FILE:A>") set_target_properties(B PROPERTIES INCLUDE_DIRECTORIES "$<TARGET_PROPERTY:A,INCLUDE_DIRECTORIES>") Is that what you had in mind? Comment Actions Right, this technically works, however it's kind of crazy to have to do this instead of just target_link_libraries(C PRIVATE B). I don't think that's a viable way to move forward. Furthermore, concretely there isn't that much of a downside in hardcoding an order between libc++ and libc++abi. There's also precedent for doing this with compiler-rt. So.. yes, technically we can make it work without enforcing an order, however targets that use libcxx-abi-shared & friends in D120727 will have to do it in a really awkward way. Comment Actions LGTM after discussing this with @ldionne on Discord. We'll try to come up with a more general solution next. | ||||||||||||||||||||||||||||||
We similarly reorder compiler-rt here so these blocks should probably be moved next to each other.
If we decide to go take this direction, I wonder if we should have a canonical order for runtimes rather than reodering them ad-hoc? I remember having that discussion with @beanz in the past and he argued against it but I don't remember what the reason was, @beanz do you remember?
A potential issue I can think of is if downstream users have their own runtimes (that is doing -DLLVM_ENABLE_RUNTIMES=libcxx;foo), there's currently no way for them to specify the order and if we start relying on a canonical ordering, we may inadvertently break them, but I'm not sure if that's a practical concern.