[libcxx-commits] [PATCH] D120719: [runtimes] Always configure libc++abi before libc++

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D120719#3358451 <https://reviews.llvm.org/D120719#3358451>, @beanz wrote:

> 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.

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 at: libcxx/cmake/caches/MinGW.cmake:9
 set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+set(LIBCXXABI_HERMETIC_STATIC_LIBRARY OFF CACHE BOOL "")
 
----------------
mstorsjo wrote:
> This would now start dllexporting cxxabi symbols in a full-static configuration, too, which isn’t desired. So if we’d want to set this flag, we should only do it in the mingw-dll configuration, not in mingw-static. (This won’t be noticed in CI, but it would produce static libraries that don’t do the right thing in practice.)
> 
> But overall; the setup of the linkage between libcxxabi and libcxx for windows has worked based solely on the usual `LIBCXX_ENABLE_SHARED` flags, for a couple releases now (which is good - the fewer options needed for getting it right, the better); I’d we’d now require setting the hermetic flag for controlling it, it’d be a breakage for such configurations out there. (I.e., if this code change requires changes to the CI script or the cmake caches, it will also break some users setups similarly.)
> 
> I’m not opposed to making the libcxxabi visibility mechanism for windows share internal logic with the hermetic library option though, but for the windows configuration, the user shouldn’t need to know about this option as it hasn’t been needed so far. (And if the user would start to need to use it, where things worked before, I’d like a proper deprecation cycle to let users migrate their build setups without breakage.)
> 
> Could we change the logic on libcxxabi cmakelists line 305 to `(LIBCXX_ENABLE_SHARED OR UNSET LIBCXX_ENABLE_SHARED) …`? I.e. the same logic we have right now, but treating unset as on (which is the default).
On all other platforms, we build the static library with symbols exported by default. I would really like to get rid of the special logic for Windows only, and TBH I think it's reasonable for vendors to be aware of it. One thing we could do is set `LIBCXXABI_HERMETIC_STATIC_LIBRARY` to `ON` by default if we're on Windows.

I'm not fond of the approach suggested in D120982 because it doesn't get rid of the (circular) dependency between libc++ and libc++abi, which is the root cause of several problems. Let's move this specific discussion to D120982, for now I'll remove this part of the diff so we can focus on that problem in D120982.


================
Comment at: runtimes/CMakeLists.txt:74
   if(NOT DEFINED LLVM_BUILD_COMPILER_RT OR LLVM_BUILD_COMPILER_RT)
     list(INSERT runtimes 0 ${compiler_rt_path})
   endif()
----------------
phosek wrote:
> 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.
I'll handle compiler-rt reordering similarly to how I'm handling libc++ and libc++abi.

IMO the issue of downstream users with custom runtimes isn't really a problem -- if we do end up having a 100% canonical ordering, we could always leave those at the end of the list.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120719/new/

https://reviews.llvm.org/D120719