[CMake][Compiler-rt] Compute LLVM_MAIN_SRC_DIR assuming the monorepo layout.

[delcypher]

Summary

When doing a standalone compiler-rt build we currently rely on
getting information from the llvm-config binary. Previously
we would rely on calling llvm-config --src-root to find the
LLVM sources. Unfortunately the returned path could easily be wrong
if the sources were built on another machine.

Now that compiler-rt is part of a monorepo we can easily fix this
problem by finding the LLVM source tree next to compiler-rt in
the monorepo. We do this regardless of whether or not the llvm-config
binary is available which moves us one step closer to not requiring
llvm-config to be available.

To try avoid anyone breaking anyone who relies on the current behavior,
if the path assuming the monorepo layout doesn't exist we invoke
llvm-config --src-root to get the path. A deprecation warning is
emitted if this path is taken because we should remove this path
in the future given that other runtimes already assume the monorepo
layout.

We also now emit a warning if LLVM_MAIN_SRC_DIR does not exist.
The intention is that this should be a hard error in future but
to avoid breaking existing users we'll keep this as a warning
for now.

rdar://76016632

[phosek]

Other runtimes like libc++, libc++abi and libunwind already assume the monorepo layout and don't rely on llvm-config. Removing the llvm-config dependency would allow further cleanups in the compiler-rt build so this is a highly desirable change. Given that, I'd actually prefer just making this the default behavior.

If you want to be extra cautious, then maybe we could invert this and try to use ${CMAKE_CURRENT_SOURCE_DIR}/../llvm by default, and only if it doesn't exist, fallback onto llvm-config but print a warning that we're going to remove this behavior soon.

[yln]

In D99620#2659855, @phosek wrote:

Other runtimes like libc++, libc++abi and libunwind already assume the monorepo layout and don't rely on llvm-config. Removing the llvm-config dependency would allow further cleanups in the compiler-rt build so this is a highly desirable change. Given that, I'd actually prefer just making this the default behavior.

+1

[delcypher]

In D99620#2659855, @phosek wrote:

Other runtimes like libc++, libc++abi and libunwind already assume the monorepo layout and don't rely on llvm-config. Removing the llvm-config dependency would allow further cleanups in the compiler-rt build so this is a highly desirable change. Given that, I'd actually prefer just making this the default behavior.

If you want to be extra cautious, then maybe we could invert this and try to use ${CMAKE_CURRENT_SOURCE_DIR}/../llvm by default, and only if it doesn't exist, fallback onto llvm-config but print a warning that we're going to remove this behavior soon.

Thanks. I wasn't aware of what the other runtimes did. Given that let's make not using llvm-config the default. I would prefer to be cautious here though so I'll make the old behavior a fallback with a deprecation warning.

[delcypher]

Make relying on the monorepo layout the default and using llvm-config a deprecated fallback.

[delcypher]

@phosek @yln I've made the change.

[tstellar]

We still build compiler-rt in non-monorepo configuration for Fedora. Which files from the llvm directory (or other directories in the monorepo) does compiler-rt depend on?

[delcypher]

In D99620#2660017, @tstellar wrote:

We still build compiler-rt in non-monorepo configuration for Fedora. Which files from the llvm directory (or other directories in the monorepo) does compiler-rt depend on?

@tstellar Several. This subsequent patch (https://reviews.llvm.org/D99621) might illuminate some of this because it starts the work of trying to remove the dependency on the LLVMConfig.cmake file which is generated by an LLVM build but isn't necessarily shipped with toolchains.

In your case if the output of llvm-config --src-root is usable then the build should still work. You also have the option of invoking CMake with -DLLVM_MAIN_SRC_DIR=/path/to/llvm to set the path to be whatever you need it to be.

The intention though is to eventually deprecate calling llvm-config --src-root to get the LLVM source path but I am in no huge rush to do that.

[delcypher]

In D99620#2660134, @delcypher wrote:

In D99620#2660017, @tstellar wrote:

We still build compiler-rt in non-monorepo configuration for Fedora. Which files from the llvm directory (or other directories in the monorepo) does compiler-rt depend on?

@tstellar Several. This subsequent patch (https://reviews.llvm.org/D99621) might illuminate some of this because it starts the work of trying to remove the dependency on the LLVMConfig.cmake file which is generated by an LLVM build but isn't necessarily shipped with toolchains.

@tstellar

To expand on this.

Compiler-rt currently depends on at least the following

• The LLVMConfig.cmake file that's generated by the build of LLVM. It's located at lib/cmake/llvm/LLVMConfig.cmake in the root of an LLVM build and is optionally installed by toolchain maintainers.
• Several CMake files that lives in the llvm/cmake/modules in the llvm source tree. LLVMConfig.cmake does include(${LLVM_CMAKE_DIR}/LLVM-Config.cmake) where LLVM_CMAKE_DIR points at the directory containing this file. For a build directory LLVM_CMAKE_DIR points into the llvm source tree, for an installed toolchain LLVM_CMAKE_DIR will point to where the files where optionally installed by toolchain maintainers.

[tstellar]

@delcypher Ok, thanks. So my question for for @phosek is what if we replaced the usage of llvm-config with find_package(LLVM) would that change also enable the clean ups you are talking about? Or does relying on any kind of out-of-tree files make the build more complicated?

[delcypher]

In D99620#2660215, @tstellar wrote:

@delcypher Ok, thanks. So my question for for @phosek is what if we replaced the usage of llvm-config with find_package(LLVM) would that change also enable the clean ups you are talking about? Or does relying on any kind of out-of-tree files make the build more complicated?

@tstellar

@phosek likely has his own opinions on this but I wouldn't consider using find_package(LLVM) a clean up. find_package(LLVM) requires the the LLVM CMake files to actually be available and not all toolchain vendors ship them. This would make building a standalone build of compiler-rt even more painful than it already is.

I think ideally we should move to a world where neither the llvm-config binary nor doing find_package(LLVM) is required to build compiler-rt. I think it's fine for compiler-rt to depend on CMake source files in the LLVM source tree (it already does) but depending on any LLVM specific build artefacts (like llvm-config or a LLVM CMake package) that a toolchain might not ship is a pain which I don't think is technically necessary.

[phosek]

The biggest issue with the current setup is that compiler-rt build relies on various LLVM build artifacts, like using the just built Clang but not in a very disciplined way which requires us to use various workaround like this: https://github.com/llvm/llvm-project/blob/99fd0662278470f5405b8abd79b681b96cac7856/runtimes/CMakeLists.txt#L116 (if you search for compiler-rt in that file, you'll see other issues).

I completely agree with @delcypher. Using find_package(LLVM) wouldn't really be an improvement. Ideally compiler-rt build would directly use other CMake files from the LLVM source tree, but wouldn't require LLVM build output. If it needs to use any build artifacts from LLVM, those would have be passed explicitly via CMake options (that is, no automatic discovery). In the LLVM runtimes build that would be done automatically.

[mstorsjo]

In D99620#2660251, @delcypher wrote:

In D99620#2660215, @tstellar wrote:

@delcypher Ok, thanks. So my question for for @phosek is what if we replaced the usage of llvm-config with find_package(LLVM) would that change also enable the clean ups you are talking about? Or does relying on any kind of out-of-tree files make the build more complicated?

@tstellar

@phosek likely has his own opinions on this but I wouldn't consider using find_package(LLVM) a clean up. find_package(LLVM) requires the the LLVM CMake files to actually be available and not all toolchain vendors ship them. This would make building a standalone build of compiler-rt even more painful than it already is.

I think ideally we should move to a world where neither the llvm-config binary nor doing find_package(LLVM) is required to build compiler-rt. I think it's fine for compiler-rt to depend on CMake source files in the LLVM source tree (it already does) but depending on any LLVM specific build artefacts (like llvm-config or a LLVM CMake package) that a toolchain might not ship is a pain which I don't think is technically necessary.

+1 for this. Preferring finding things from the surrounding monorepo is much nicer than relying on either llvm-config or LLVMConfig.cmake to be installed (as the toolchains I build don't ship with those).

Compiler-rt currently depends on at least the following

• The LLVMConfig.cmake file that's generated by the build of LLVM. It's located at lib/cmake/llvm/LLVMConfig.cmake in the root of an LLVM build and is optionally installed by toolchain maintainers.
• Several CMake files that lives in the llvm/cmake/modules in the llvm source tree. LLVMConfig.cmake does include(${LLVM_CMAKE_DIR}/LLVM-Config.cmake) where LLVM_CMAKE_DIR points at the directory containing this file. For a build directory LLVM_CMAKE_DIR points into the llvm source tree, for an installed toolchain LLVM_CMAKE_DIR will point to where the files where optionally installed by toolchain maintainers.

It doesn't strictly require those, at least not for all build configurations: I do build compiler-rt (since a couple years) without an installed llvm-config and without LLVMConfig.cmake available (without any custom patches). This hits the case on lines 216-220 above, printing a build time warning about "UNSUPPORTED COMPILER-RT CONFIGURATION DETECTED: llvm-config not found. Reconfigure with -DLLVM_CONFIG_PATH=path/to/llvm-config", but it builds just fine despite that. I guess that I can't build and run tests in that configuration though, but other than that it does work. I do build with -DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE though, if that makes any difference.

Anyway, reducing the reliance on trying to locate llvm-config and LLVMConfig.cmake externally makes things much more predictable for me. As I don't install either of them from the main LLVM build, I expect hitting that "llvm config not found" case. However, if I do build this on a distro with LLVM installed system wide via the package manager, it can find a llvm-config from that installation (if those particular packages that contain it happen to be installed), which can be from a woefully different version. That makes it try to include odd old versions of the cmake files, with really funky (and hard to debug) results - when I just wanted it not to do that at all.

[delcypher]

In D99620#2660641, @mstorsjo wrote:

In D99620#2660251, @delcypher wrote:

In D99620#2660215, @tstellar wrote:

@delcypher Ok, thanks. So my question for for @phosek is what if we replaced the usage of llvm-config with find_package(LLVM) would that change also enable the clean ups you are talking about? Or does relying on any kind of out-of-tree files make the build more complicated?

@tstellar

@phosek likely has his own opinions on this but I wouldn't consider using find_package(LLVM) a clean up. find_package(LLVM) requires the the LLVM CMake files to actually be available and not all toolchain vendors ship them. This would make building a standalone build of compiler-rt even more painful than it already is.

I think ideally we should move to a world where neither the llvm-config binary nor doing find_package(LLVM) is required to build compiler-rt. I think it's fine for compiler-rt to depend on CMake source files in the LLVM source tree (it already does) but depending on any LLVM specific build artefacts (like llvm-config or a LLVM CMake package) that a toolchain might not ship is a pain which I don't think is technically necessary.

+1 for this. Preferring finding things from the surrounding monorepo is much nicer than relying on either llvm-config or LLVMConfig.cmake to be installed (as the toolchains I build don't ship with those).

Compiler-rt currently depends on at least the following

• The LLVMConfig.cmake file that's generated by the build of LLVM. It's located at lib/cmake/llvm/LLVMConfig.cmake in the root of an LLVM build and is optionally installed by toolchain maintainers.
• Several CMake files that lives in the llvm/cmake/modules in the llvm source tree. LLVMConfig.cmake does include(${LLVM_CMAKE_DIR}/LLVM-Config.cmake) where LLVM_CMAKE_DIR points at the directory containing this file. For a build directory LLVM_CMAKE_DIR points into the llvm source tree, for an installed toolchain LLVM_CMAKE_DIR will point to where the files where optionally installed by toolchain maintainers.

It doesn't strictly require those, at least not for all build configurations: I do build compiler-rt (since a couple years) without an installed llvm-config and without LLVMConfig.cmake available (without any custom patches). This hits the case on lines 216-220 above, printing a build time warning about "UNSUPPORTED COMPILER-RT CONFIGURATION DETECTED: llvm-config not found. Reconfigure with -DLLVM_CONFIG_PATH=path/to/llvm-config", but it builds just fine despite that. I guess that I can't build and run tests in that configuration though, but other than that it does work. I do build with -DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE though, if that makes any difference.

Yeah that does make a difference. I discovered in https://reviews.llvm.org/D99621 that we rely on the TARGET_TRIPLE variable from LLVMConfig.cmake to work. IIRC specifying CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY=TRUE works around that problem. Without those variables, the first sign of trouble is

CMake Error at cmake/Modules/CompilerRTUtils.cmake:361 (string):                                                   
  string sub-command REPLACE requires at least four arguments.                                                     
Call Stack (most recent call first):                                                                               
  CMakeLists.txt:111 (construct_compiler_rt_default_triple)

then you'll get a bunch of other configure errors later on.

[delcypher]

@mstorsjo @phosek @tstellar Can this patch be approved and landed, or is there more to discuss?

[mstorsjo]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

244

This is done within an if (LLVM_CONFIG_PATH) block. As we should work towards reducing the reliance on that, shouldn't we be doing this outside of that if statement?

267

Previously, not having this directory available wasn't a fatal error, now it is. Then again, as we should be able to pick it up from the monorepo so it's probably tolerable. (I'm not sure what effect this would start having on my builds though, that currently are done without including anything from llvm/cmake, but which now will start to find such things.)

[delcypher]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

244

This is a good point. The original reason it was written this way because the subsequent patch (https://reviews.llvm.org/D99621) needs LLVM_MAIN_SRC_PATH to be available so I made it a hard error.

I'll try hosting it outside of if (LLVM_CONFIG_PATH) and see how far I get.

267

It's currently only a hard error in the case that we have llvm-config available because it should be able to give us a valid path. If we hoist handling LLVM_MAIN_SRC_DIR outside of the path where we know llvm-config is available. Should I make the path not being available a warning first so that you can evaluate the impact and then in a subsequent patch we can make it an error?

[mstorsjo]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

267

I can do a test build if you update the patch, with the code hoisted outside of the if statement. It shouldn't matter if it's an error for my test builds, as it should succeed at least in my setup. But for the subsequent patches, if we can keep it optional as it is today (as it works for some build configs at least), it'd be nice. Not sure if it's worth bending over backwards for supporting it though...

[delcypher]

• Hoist code out of LLVM_CONFIG_PATH branch.

[delcypher]

@mstorsjo I've updated the patch to hoist the code out of LLVM_CONFIG_PATH.

[delcypher]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

267

@mstorsjo I've made the if (NOT EXISTS "${LLVM_MAIN_SRC_DIR}") check a warning. I'd like to avoid breaking user workflows at this stage. But I do think it should be a hard error in the future.

[delcypher]

• Move part of warning outside of if (LLVM_CONFIG_PATH) so that it always shows up.
• s/MAIN_SOURCE_DIR/MAIN_SRC_DIR/ - I accidently renamed the variable so I put it back.

[mstorsjo]

This looks good to me, thanks! I'd still like for e.g. @phosek to have another look at it, but if not I can formally approve it later.

[mstorsjo]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

226

Actually, this path doesn't work out correctly if building by targeting compiler-rt/lib/builtins directly instead of building the whole compiler-rt. I do both of those configurations - building only builtins at an early stage, and then building the whole later.

[delcypher]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

226

Hmm. I didn't test that workflow. I might need to use CMAKE_CURRENT_LIST_DIR instead. I'll take a look.

[delcypher]

• Add get_compiler_rt_root_source_dir function for computing where the compiler-rt source tree root is located.
• Handle Compiler-RT builtins standalone build.

[delcypher]

Add an extra sanity check to make sure we're finding the right path.

[delcypher]

@mstorsjo I think I've fixed the patch to support the standalone builtins build. Can you test again?

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

226

Damn so CMAKE_CURRENT_FUNCTION_LIST_DIR could be used to help us out here but that isn't available until CMake 3.17 and 3.13.4 is the minimum supported version. So I'll have to implement this in a different way.

[mstorsjo]

In D99620#2667127, @delcypher wrote:

@mstorsjo I think I've fixed the patch to support the standalone builtins build. Can you test again?

Thanks, now it seems to work!

[delcypher]

In D99620#2667296, @mstorsjo wrote:

In D99620#2667127, @delcypher wrote:

@mstorsjo I think I've fixed the patch to support the standalone builtins build. Can you test again?

Thanks, now it seems to work!

Great. If you're happy could you approve the patch?

@phosek Could you take another look?

[mstorsjo]

LGTM, but as said earlier, ideally someone else would have another look too.

[phosek]

LGTM (I've tested this together with D99621 locally for the Fuchsia build and everything seems to be working).

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

226

I'd avoid CRT because that can be confused with CRT subproject of compiler-rt: https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/CMakeLists.txt#L20

[delcypher]

• Renamed CRT_SRC_ROOT_PATH to COMPILER_RT_ROOT_SRC_PATH

[delcypher]

@phosek Thanks for the review.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

226

Thanks. I've renamed the variable.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

if (NOT LLVM_CONFIG_PATH)
                       "llvm-config not found.\n"
                       "Reconfigure with -DLLVM_CONFIG_PATH=path/to/llvm-config.")
     endif()
   endif()
   if (LLVM_CONFIG_PATH)
     execute_process(
       COMMAND ${LLVM_CONFIG_PATH} "--obj-root" "--bindir" "--libdir" "--src-root" "--includedir"
       RESULT_VARIABLE HAD_ERROR
       OUTPUT_VARIABLE CONFIG_OUTPUT)
     if (HAD_ERROR)
       message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
     endif()
     string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" CONFIG_OUTPUT ${CONFIG_OUTPUT})
     list(GET CONFIG_OUTPUT 0 BINARY_DIR)
     list(GET CONFIG_OUTPUT 1 TOOLS_BINARY_DIR)
     list(GET CONFIG_OUTPUT 2 LIBRARY_DIR)
     list(GET CONFIG_OUTPUT 3 MAIN_SRC_DIR)
     list(GET CONFIG_OUTPUT 4 INCLUDE_DIR)
 
     set(LLVM_BINARY_DIR ${BINARY_DIR} CACHE PATH "Path to LLVM build tree")
     set(LLVM_LIBRARY_DIR ${LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
     set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
     set(LLVM_TOOLS_BINARY_DIR ${TOOLS_BINARY_DIR} CACHE PATH "Path to llvm/bin")
     set(LLVM_INCLUDE_DIR ${INCLUDE_DIR} CACHE PATH "Paths to LLVM headers")
 
+    # `--src-root` might report a path does not exist (e.g. built on another machine).
+    # In this case we fallback on the LLVM sources living next to `compiler-rt` which
+    # is true in the compiler-rt is checked out as part of the monorepo.
+    if (NOT EXISTS "${LLVM_MAIN_SRC_DIR}")
+      # TODO(dliew): We should make this behavior the only behavior rather than
+      # a fallback once we know nobody relies on the old behavior.
+      message(WARNING "LLVM source directory (${LLVM_MAIN_SRC_DIR}) reported by llvm-config does not exist")
+      get_filename_component(LLVM_MAIN_SRC_DIR_REL "${CMAKE_CURRENT_SOURCE_DIR}/../llvm" ABSOLUTE)
+      if (NOT EXISTS "${LLVM_MAIN_SRC_DIR_REL}")
+        message(FATAL_ERROR "LLVM source directory (${LLVM_MAIN_SRC_DIR_REL}) does not exist")
+      endif()
+      set(LLVM_MAIN_SRC_DIR "${LLVM_MAIN_SRC_DIR_REL}")
+    endif()
+    message(STATUS "LLVM source directory: \"${LLVM_MAIN_SRC_DIR}\"")
+
     # Detect if we have the LLVMXRay and TestingSupport library installed and
     # available from llvm-config.
     execute_process(
       COMMAND ${LLVM_CONFIG_PATH} "--ldflags" "--libs" "xray"
       RESULT_VARIABLE HAD_ERROR
       OUTPUT_VARIABLE CONFIG_OUTPUT
       ERROR_QUIET)
     if (HAD_ERROR)
       message(WARNING "llvm-config finding xray failed with status ${HAD_ERROR}")
       set(COMPILER_RT_HAS_LLVMXRAY FALSE)
     else()
       string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" CONFIG_OUTPUT ${CONFIG_OUTPUT})
       list(GET CONFIG_OUTPUT 0 LDFLAGS)
       list(GET CONFIG_OUTPUT 1 LIBLIST)
       file(TO_CMAKE_PATH "${LDFLAGS}" LDFLAGS)
       file(TO_CMAKE_PATH "${LIBLIST}" LIBLIST)
       set(LLVM_XRAY_LDFLAGS ${LDFLAGS} CACHE STRING "Linker flags for LLVMXRay library")
       set(LLVM_XRAY_LIBLIST ${LIBLIST} CACHE STRING "Library list for LLVMXRay")