[CMake] Re-order runtimes in the order of dependencies

[phosek]

Summary

This allows runtimes to have checks like if(TARGET ${runtime}).

[phosek]

This is an alternative to D68791 which is trying to address the issue introduced in r374116.

[smeenai]

Makes sense to me, particularly since this is generalizing what we're already doing for compiler-rt.

Is compiler-rt's place in the dependency list accurate though? For example, libc++ needs a builtins library, which could be compiler-rt, and I think the compiler-rt to libc++ dependency is limited to things like libFuzzer. I don't think libc++ supports using an in-tree compiler-rt though, so that's probably a moot point.

llvm/runtimes/CMakeLists.txt

113

depend on -> depends on

[beanz]

Two thoughts.

(1) Short term I think this is wrong, in general we should avoid using if(TARGET...). Compiler-rt has the most ridiculous magic in it for which targets are enabled or not, which is why it was put first. That allows the other runtimes to check against compiler-rt which was harder to replicate logic for which sanitizers were included in the build. The other runtime libraries should be able to use the HAVE_${runtime} variables to determine the presence of the other runtimes. That should correctly allow libcxx, libcxxabi and libunwind to not be order dependent.
(2) Longer term, we really need to update the runtimes to make their dependencies based on generator expressions so that we can remove at least some of the order dependence.

[compnerd]

I feel like Im not understanding something. The point of CMake is that it tracks dependencies. We are we manually tracking dependencies?

The MSAN check was needed due to the inverted dependencies; with unified builds, this shouldnt be an issue any longer. I think that propagating the checks this way is the wrong approach.

Can you please explain why you need the if(TARGET runtime) in the first place? Understanding that might help with coming up with a better solution (or possibly an existing solution).

[phosek]

See https://reviews.llvm.org/D68791 for an example where this came up. I'm open to other solutions, but I think what's in that change (which is what we were using until r374116 when it was accidentally removed) isn't great either.

[ldionne]

CMake tracks dependencies between targets, but not between directories. If the CMakeLists.txt in some directory (e.g. <root>/libcxx/) needs a target defined in another directory (e.g. <root>/libcxxabi/), one has to make sure that libcxxabi's CMakeLists.txt is included before libcxx's CMakeLists.txt. This isn't new or vexing, IMO. If that is what this patch ensures (I don't know the runtimes build very well), I think this is good.

[...] in general we should avoid using if(TARGET...).

I'd like to question that affirmation. What is it based on?

The other runtime libraries should be able to use the HAVE_${runtime} variables to determine the presence of the other runtimes

IMO, the HAVE_${runtime} variables are the weird ones here. The normal LLVM monorepo build orders the directories correctly, and we don't run into that issue. I don't see how HAVE_${runtime} can get around things like being able to query properties of e.g. cxxabi_shared inside the libcxx build before cxxabi_shared has been defined. I do support a push towards using generator expressions more, but I don't think generator expressions are a complete solution to this problem.

I'd like to see this patch go in under some form so that we can remove the hacky workaround introduced in https://reviews.llvm.org/D68791.

[beanz]

In D68833#1706104, @ldionne wrote:

CMake tracks dependencies between targets, but not between directories. If the CMakeLists.txt in some directory (e.g. <root>/libcxx/) needs a target defined in another directory (e.g. <root>/libcxxabi/), one has to make sure that libcxxabi's CMakeLists.txt is included before libcxx's CMakeLists.txt. This isn't new or vexing, IMO. If that is what this patch ensures (I don't know the runtimes build very well), I think this is good.

The problem is that libcxx depends on compiler-rt, and compiler-rt depends on libcxx. Which means we have circular dependencies. The solution is to migrate to CMake usage patterns that don't require strict ordering. Specifically the use of generator expressions in this (and many other situations) is warranted.

I'd like to question that affirmation. What is it based on?

The fact that we have circular dependencies at the project level (not the target level). The if (TARGET ...) feature in CMake requires that targets are processed in specific orders and cannot deal with projects that have circular dependency relationships.

IMO, the HAVE_${runtime} variables are the weird ones here. The normal LLVM monorepo build orders the directories correctly, and we don't run into that issue.

I'm not sure what you mean by this. What I *think* you are referring to is specifying runtime libraries in LLVM_ENABLE_PROJECTS which uses the llvm/projects subdirectory for building them. This build flow is also part of the monorepo and uses LLVM_ENABLE_RUNTIMES. Fundamentally LLVM_ENABLE_PROJECTS is the incorrect way to build runtime libraries because they are not built with the in-tree compiler. While this works for most development cases it is 100% wrong for building and shipping toolchains, and it is questionable practice to have developers building and testing things that aren't representative of what we ship.

I don't see how HAVE_${runtime} can get around things like being able to query properties of e.g. cxxabi_shared inside the libcxx build before cxxabi_shared has been defined. I do support a push towards using generator expressions more, but I don't think generator expressions are a complete solution to this problem.

What do you think that generator expressions can't do that is relevant to this problem?

I'd like to see this patch go in under some form so that we can remove the hacky workaround introduced in https://reviews.llvm.org/D68791.

You can also remove that hacky workaround using generator expressions. Replacing one hack with another isn't exactly a meaningful transformation of the codebase.

[ldionne]

In D68833#1706296, @beanz wrote:

In D68833#1706104, @ldionne wrote:

CMake tracks dependencies between targets, but not between directories. If the CMakeLists.txt in some directory (e.g. <root>/libcxx/) needs a target defined in another directory (e.g. <root>/libcxxabi/), one has to make sure that libcxxabi's CMakeLists.txt is included before libcxx's CMakeLists.txt. This isn't new or vexing, IMO. If that is what this patch ensures (I don't know the runtimes build very well), I think this is good.

[...]
I'm not sure what you mean by this. What I *think* you are referring to is specifying runtime libraries in LLVM_ENABLE_PROJECTS which uses the llvm/projects subdirectory for building them.

Yes, precisely. That's also the currently preferred way of building libc++: https://libcxx.llvm.org/docs/BuildingLibcxx.html

This build flow is also part of the monorepo and uses LLVM_ENABLE_RUNTIMES. Fundamentally LLVM_ENABLE_PROJECTS is the incorrect way to build runtime libraries because they are not built with the in-tree compiler. While this works for most development cases it is 100% wrong for building and shipping toolchains, and it is questionable practice to have developers building and testing things that aren't representative of what we ship.

Not everybody ships libc++/libc++abi as part of the toolchain, and for those, building with whatever CMAKE_CXX_COMPILER they specify is really the right thing to do. Don't get me wrong, I'm 100% on board that there's value in having this runtime build, however let's not pretend that it's the only correct way to build libc++.

I don't see how HAVE_${runtime} can get around things like being able to query properties of e.g. cxxabi_shared inside the libcxx build before cxxabi_shared has been defined. I do support a push towards using generator expressions more, but I don't think generator expressions are a complete solution to this problem.

What do you think that generator expressions can't do that is relevant to this problem?

Say I need to generate a file based on the properties of a target. I'll need to call get_target_property on a target that hasn't been defined yet, and there's no way around that because file(GENERATE) does not expand generator expressions.

I'd like to see this patch go in under some form so that we can remove the hacky workaround introduced in https://reviews.llvm.org/D68791.

You can also remove that hacky workaround using generator expressions.

Are you thinking about this?

set(libname "$<IF:$<TARGET_EXISTS:${lib}>,$<TARGET_PROPERTY:${lib},OUTPUT_NAME>,${lib}>")
 list(APPEND link_libraries "${CMAKE_LINK_LIBRARY_FLAG}${libname}")

That is clever, I had not thought about it.

If the above workaround works, I don't care about this patch that much. I still think we need to clarify the status of the Runtimes build and document it, unless that's already done and I've missed it (in which case please point it to me).

[beanz]

In D68833#1706315, @ldionne wrote:

Yes, precisely. That's also the currently preferred way of building libc++: https://libcxx.llvm.org/docs/BuildingLibcxx.html

That documentation is more than a bit lacking, as is much of the LLVM documentation.

Not everybody ships libc++/libc++abi as part of the toolchain, and for those, building with whatever CMAKE_CXX_COMPILER they specify is really the right thing to do.

While this is true, in many instances libc++ even when libc++ isn't shipped with a toolchain it is locked to one. Darwin is a prime example of this. On Darwin libc++ is shipped as part of the OS, but that cycle is closely coordinated with the toolchain updates and the two are usually kept in sync.

Don't get me wrong, I'm 100% on board that there's value in having this runtime build, however let's not pretend that it's the only correct way to build libc++.

I would argue if you don't ship libc++ as part of the toolchain you shouldn't build it as part of the toolchain either. In which case the standalone build configuration is the correct way to build it. My intention isn't to say building libcxx as a runtime is the only correct way to build libc++, my intention is to state that *if* you are building libc++ with the toolchain, building it as a runtime is the only correct way to build it.

Say I need to generate a file based on the properties of a target. I'll need to call get_target_property on a target that hasn't been defined yet, and there's no way around that because file(GENERATE) does not expand generator expressions.

Is this something you need to do? If so I'd question higher-level decisions about how the build is structured.

Are you thinking about this?

set(libname "$<IF:$<TARGET_EXISTS:${lib}>,$<TARGET_PROPERTY:${lib},OUTPUT_NAME>,${lib}>")
 list(APPEND link_libraries "${CMAKE_LINK_LIBRARY_FLAG}${libname}")

That is clever, I had not thought about it.

If the above workaround works, I don't care about this patch that much. I still think we need to clarify the status of the Runtimes build and document it, unless that's already done and I've missed it (in which case please point it to me).

That is a much better approach. CMake 3.11 is when the TARGET_EXISTS generator expression was added, although the documentation wasn't updated until CMake 3.15, so this change would require a CMake version update, which I don't think is unreasonable.

[phosek]

In D68833#1706544, @beanz wrote:

In D68833#1706315, @ldionne wrote:

Yes, precisely. That's also the currently preferred way of building libc++: https://libcxx.llvm.org/docs/BuildingLibcxx.html

That documentation is more than a bit lacking, as is much of the LLVM documentation.

Not everybody ships libc++/libc++abi as part of the toolchain, and for those, building with whatever CMAKE_CXX_COMPILER they specify is really the right thing to do.

While this is true, in many instances libc++ even when libc++ isn't shipped with a toolchain it is locked to one. Darwin is a prime example of this. On Darwin libc++ is shipped as part of the OS, but that cycle is closely coordinated with the toolchain updates and the two are usually kept in sync.

Don't get me wrong, I'm 100% on board that there's value in having this runtime build, however let's not pretend that it's the only correct way to build libc++.

I would argue if you don't ship libc++ as part of the toolchain you shouldn't build it as part of the toolchain either. In which case the standalone build configuration is the correct way to build it. My intention isn't to say building libcxx as a runtime is the only correct way to build libc++, my intention is to state that *if* you are building libc++ with the toolchain, building it as a runtime is the only correct way to build it.

Say I need to generate a file based on the properties of a target. I'll need to call get_target_property on a target that hasn't been defined yet, and there's no way around that because file(GENERATE) does not expand generator expressions.

Is this something you need to do? If so I'd question higher-level decisions about how the build is structured.

Are you thinking about this?

set(libname "$<IF:$<TARGET_EXISTS:${lib}>,$<TARGET_PROPERTY:${lib},OUTPUT_NAME>,${lib}>")
 list(APPEND link_libraries "${CMAKE_LINK_LIBRARY_FLAG}${libname}")

That is clever, I had not thought about it.

If the above workaround works, I don't care about this patch that much. I still think we need to clarify the status of the Runtimes build and document it, unless that's already done and I've missed it (in which case please point it to me).

That is a much better approach. CMake 3.11 is when the TARGET_EXISTS generator expression was added, although the documentation wasn't updated until CMake 3.15, so this change would require a CMake version update, which I don't think is unreasonable.

I've tested D68880 which requires newer CMake but it does seem to be working. Shall we start the discussion about bumping the minimum CMake version requirement on llvm-dev?

[ldionne]

In D68833#1706696, @phosek wrote:

In D68833#1706544, @beanz wrote:

That is a much better approach. CMake 3.11 is when the TARGET_EXISTS generator expression was added, although the documentation wasn't updated until CMake 3.15, so this change would require a CMake version update, which I don't think is unreasonable.

I've tested D68880 which requires newer CMake but it does seem to be working. Shall we start the discussion about bumping the minimum CMake version requirement on llvm-dev?

Yes, I think that would be great. CMake is one of those programs that's terribly easy to install, and I think it's reasonable to request a recent CMake in order to build LLVM (or at least libc++).

If the rest of LLVM wants/needs to stay stuck on CMake 3.4 (!!!), it might be possible to move the requirements for libc++/libc++abi only.

llvm/runtimes/CMakeLists.txt

foreach(proj ${LLVM_ENABLE_RUNTIMES})
   else()
     message(FATAL_ERROR "LLVM_ENABLE_RUNTIMES requests ${proj} but directory not found: ${proj_dir}")
   endif()
   string(TOUPPER "${proj}" canon_name)
   STRING(REGEX REPLACE "-" "_" canon_name ${canon_name})
   set(LLVM_EXTERNAL_${canon_name}_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../${proj}")
 endforeach()
 
-function(get_compiler_rt_path path)
+function(get_runtime_path runtime path)
   foreach(entry ${runtimes})
     get_filename_component(projName ${entry} NAME)
-    if("${projName}" MATCHES "compiler-rt")
+    if("${projName}" MATCHES "${runtime}")
       set(${path} ${entry} PARENT_SCOPE)
       return()
     endif()
   endforeach()
 endfunction()
 
 # If this file is acting as a top-level CMake invocation, this code path is
 # triggered by the external project call for the runtimes target below.
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
 
   # Add the root project's CMake modules, and the LLVM build's modules to the
   # CMake module path.
   list(INSERT CMAKE_MODULE_PATH 0
     "${CMAKE_CURRENT_SOURCE_DIR}/../cmake"
     "${CMAKE_CURRENT_SOURCE_DIR}/../cmake/modules"
   )
 
-  # Some of the runtimes will conditionally use the compiler-rt sanitizers
-  # to make this work smoothly we ensure that compiler-rt is added first in
-  # the list of sub-projects. This allows other sub-projects to have checks
-  # like `if(TARGET asan)` to enable building with asan.
-  get_compiler_rt_path(compiler_rt_path)
-  if(compiler_rt_path)
-    list(REMOVE_ITEM runtimes ${compiler_rt_path})
-    if(NOT DEFINED LLVM_BUILD_COMPILER_RT OR LLVM_BUILD_COMPILER_RT)
-      list(INSERT runtimes 0 ${compiler_rt_path})
-    endif()
-  endif()
-
   # Setting these variables will allow the sub-build to put their outputs into
   # the library and bin directories of the top-level build.
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${LLVM_LIBRARY_DIR})
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${LLVM_TOOLS_BINARY_DIR})
 
   # This variable makes sure that e.g. llvm-lit is found.
   set(LLVM_MAIN_SRC_DIR ${LLVM_BUILD_MAIN_SRC_DIR})
   set(LLVM_CMAKE_PATH ${LLVM_MAIN_SRC_DIR}/cmake/modules)
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
   # Remove the -nostdlib++ option we've added earlier.
   string(REPLACE "-nostdlib++" "" CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")
 
   # Use libtool instead of ar if you are both on an Apple host, and targeting Apple.
   if(CMAKE_HOST_APPLE AND APPLE)
     include(UseLibtool)
   endif()
 
+  # Re-order runtimes in the order of dependencies: libcxxabi depend on libunwind,
+  # libcxx depends on libcxxabi, some compiler-rt runtimes depend on libcxx. This
+  # allows these runtimes to have checks like `if(TARGET ${runtime})`.
+  foreach(runtime compiler-rt libcxx libcxxabi libunwind)
+    get_runtime_path(${runtime} runtime_path)
+    if(runtime_path)
+      list(REMOVE_ITEM runtimes ${runtime_path})
+      list(INSERT runtimes 0 ${runtime_path})
+    endif()
+  endforeach()
+
   # This can be used to detect whether we're in the runtimes build.
   set(RUNTIMES_BUILD ON)
 
   foreach(entry ${runtimes})
     get_filename_component(projName ${entry} NAME)
 
     # TODO: Clean this up as part of an interface standardization
     string(REPLACE "-" "_" canon_name ${projName})
llvm_ExternalProject_Add(builtins-${target}
                              TOOLCHAIN_TOOLS clang lld llvm-ar llvm-ranlib llvm-nm llvm-objcopy llvm-objdump llvm-strip
                              USE_TOOLCHAIN
                              ${EXTRA_ARGS})
   endfunction()
 
   # If compiler-rt is present we need to build the builtin libraries first. This
   # is required because the other runtimes need the builtin libraries present
   # before the just-built compiler can pass the configuration tests.
-  get_compiler_rt_path(compiler_rt_path)
+  get_runtime_path(compiler-rt compiler_rt_path)
   if(compiler_rt_path)
     if(NOT LLVM_BUILTIN_TARGETS)
       builtin_default_target(${compiler_rt_path}
         DEPENDS clang-resource-headers)
     else()
       if("default" IN_LIST LLVM_BUILTIN_TARGETS)
         builtin_default_target(${compiler_rt_path}
           DEPENDS clang-resource-headers)