Page MenuHomePhabricator

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

Authored by delcypher on Tue, Mar 30, 2:31 PM.

Details

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

Diff Detail

Event Timeline

delcypher created this revision.Tue, Mar 30, 2:31 PM
delcypher requested review of this revision.Tue, Mar 30, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 30, 2:31 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

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 added a comment.Tue, Mar 30, 3:03 PM

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

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 updated this revision to Diff 334286.Tue, Mar 30, 3:28 PM

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

delcypher edited the summary of this revision. (Show Details)Tue, Mar 30, 3:29 PM

@phosek @yln I've made the change.

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?

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 retitled this revision from [CMake][Compiler-rt] Fix `LLVM_MAIN_SRC_DIR` to be correct when `llvm-config` reports a path that doesn't exist. to [CMake][Compiler-rt] Compute LLVM_MAIN_SRC_DIR assuming the monorepo layout..Tue, Mar 30, 4:32 PM

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.

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

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.

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

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

delcypher edited the summary of this revision. (Show Details)Thu, Apr 1, 9:52 AM
mstorsjo added inline comments.Thu, Apr 1, 2:11 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
307

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?

330

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 added inline comments.Thu, Apr 1, 4:00 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
307

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.

330

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 added inline comments.Thu, Apr 1, 9:45 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
330

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 updated this revision to Diff 335005.Fri, Apr 2, 12:03 PM
  • Hoist code out of LLVM_CONFIG_PATH branch.
delcypher marked an inline comment as done.Fri, Apr 2, 12:38 PM

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

delcypher edited the summary of this revision. (Show Details)Fri, Apr 2, 12:56 PM
vitalybuka resigned from this revision.Fri, Apr 2, 12:59 PM
delcypher added inline comments.Fri, Apr 2, 1:01 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
330

@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 updated this revision to Diff 335018.Fri, Apr 2, 1:11 PM
delcypher edited the summary of this revision. (Show Details)
  • 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.

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 added inline comments.Fri, Apr 2, 2:00 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
277

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 added inline comments.Fri, Apr 2, 2:36 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
277

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

delcypher updated this revision to Diff 335040.Fri, Apr 2, 4:05 PM
  • 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 updated this revision to Diff 335043.Fri, Apr 2, 4:11 PM

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

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

compiler-rt/cmake/Modules/CompilerRTUtils.cmake
277

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 I think I've fixed the patch to support the standalone builtins build. Can you test again?

Thanks, now it seems to work!

@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 accepted this revision.Sat, Apr 3, 2:58 PM

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

This revision is now accepted and ready to land.Sat, Apr 3, 2:58 PM
phosek accepted this revision.Tue, Apr 6, 12:31 AM

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
277

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 updated this revision to Diff 335520.Tue, Apr 6, 8:26 AM
  • Renamed CRT_SRC_ROOT_PATH to COMPILER_RT_ROOT_SRC_PATH
This revision was landed with ongoing or failed builds.Tue, Apr 6, 8:33 AM
This revision was automatically updated to reflect the committed changes.

@phosek Thanks for the review.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake
277

Thanks. I've renamed the variable.