This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Switch from llvm-config to find_package(LLVM)
ClosedPublic

Authored by mgorny on Oct 29 2022, 10:54 AM.

Diff Detail

Event Timeline

mgorny created this revision.Oct 29 2022, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2022, 10:54 AM
mgorny requested review of this revision.Oct 29 2022, 10:54 AM
phosek accepted this revision.Oct 29 2022, 11:23 AM

LGTM

compiler-rt/lib/xray/tests/CMakeLists.txt
68–81

Rather than manually linking against libraries and their dependencies (which may change over time and break this code), we should use import targets provided by the LLVM CMake config: LLVMXRay and LLVMTestingSupport. That can be done in a follow up change though.

This revision is now accepted and ready to land.Oct 29 2022, 11:23 AM
mgorny added inline comments.Oct 29 2022, 11:34 AM
compiler-rt/lib/xray/tests/CMakeLists.txt
68–81

I actually wanted to do that but couldn't figure out how. Perhaps my approach was wrong, as I was trying to get linked libraries out of target properties.

phosek added inline comments.Oct 29 2022, 6:43 PM
compiler-rt/lib/xray/tests/CMakeLists.txt
68–81

I can take a look once this change lands, it might require changes to compiler-rt helpers.

Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 8:19 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
lkail added a subscriber: lkail.Oct 31 2022, 9:48 PM
mgorny reopened this revision.Oct 31 2022, 10:33 PM

Gotta figure the buildbot regression out.

This revision is now accepted and ready to land.Oct 31 2022, 10:33 PM
mgorny added a comment.Nov 1 2022, 6:42 AM

Ok, I think I know what the problem is — these buildbots rely on being able to override LLVM_CONFIG_PATH. I'm going to see if I can provide backwards compatibility with it.

mgorny updated this revision to Diff 472285.Nov 1 2022, 6:52 AM

Add a backwards compatibility path that uses LLVM_CONFIG_PATH to locate LLVM CMake files. I think it's sufficient to use path relative to the specified location, and we don't want to actually call it.

Restore the explicit warning when LLVM is not found and the fallback path is used.

mgorny added a comment.Nov 1 2022, 6:53 AM

@phosek, could you take another look? The only changes are on top of load_llvm_config.

phosek added inline comments.Nov 1 2022, 11:16 AM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
283–284

Is the assumption that LLVM_CMAKE_DIR is two directories up from LLVM_CONFIG_PATH? If so, I'd consider leaving a comment here to make that clear.

Is that also correct? In my build for example, I have bin/llvm-config and lib/cmake/llvm/LLVMConfig.cmake.

mgorny added inline comments.Nov 1 2022, 12:26 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
283–284

One directory up, the first call just strips the progname, i.e. it's the equivalent of:

$ dirname $(dirname /usr/lib/llvm/16/bin/llvm-config)
/usr/lib/llvm/16

I'll add a comment. According to my testing, LLVM_CMAKE_DIR works with either the exact CMake file directory or the prefix containing lib*/cmake. I went for the latter because it lets me avoid having to figure out the exact subdir. I suppose it's similar to how CMake locates these files relative to PATH.

mgorny updated this revision to Diff 472376.Nov 1 2022, 12:30 PM

Add an explanatory comment.

phosek accepted this revision.Nov 1 2022, 10:34 PM

Still LGTM, thanks for the explanation.

Thanks! Let's hope it works this time.

Thanks! Let's hope it works this time.

FYI, this broke builds of mine (https://github.com/mstorsjo/llvm-mingw/actions/runs/3382275510/jobs/5617040099), but I just need to change/add a workaround so I don't think we need to revert it due to that.

My setup is that I'm building LLVM with -DLLVM_INSTALL_TOOLCHAIN_ONLY=TRUE, so it only installs toolchain binaries, but not files like LLVMConfig.cmake or the individual LLVM libraries. I then use this new toolchain to (cross-)build compiler-rt (and all other runtimes), to bootstrap new cross-sysroots.

Previously, the compiler-rt cmake files would pick up a distro-installed llvm-config and due to that, add systemwide install directories into the mix when building compiler-rt (for a potential cross target!). I worked around that by passing -DLLVM_CONFIG_PATH="" to make sure that it doesn't pick up an unrelated llvm-config binary. (https://github.com/mstorsjo/llvm-mingw/commit/3b96433cdf89e8ef444c6669254b386229dbddd9) Now after this change, it can again find unrelated LLVMConfig.cmake from the surrounding distribution, which ends up adding unexpected include directories. This time I'm working around it with https://github.com/mstorsjo/llvm-mingw/commit/486dcf32274ae6c3e0ea7916fd1e88a58742b7ca, by adding -DCMAKE_FIND_ROOT_PATH=$PREFIX/$arch-w64-mingw32 -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ONLY, so that cmake won't find unrelated cmake files.

These workarounds are fine for me, but I wonder if it would be useful with a more direct cmake option to tell it not look for these files at all. I'm building out of the git monorepo checkout, so whatever source files and cmake scripts can be found there, but other LLVM build products (individual libraries etc) aren't available, and cmake shouldn't try to venture out on its own to find them in unexpected places.

@phosek - How does this work for general cross compilation of compiler-rt btw - when building compiler-rt for a cross target, but it finds LLVMConfig.cmake for the current host, and ends up including headers (and possibly more) from there? (If the include directory added from LLVMConfig.cmake is something like /usr/include/llvm-15 it's probably benign, but in the msys2 cases, the llvm headers are installed in the toplevel include directory, so it ended up adding the equivalent of -I/usr/include in something which is supposed to be a cross build.) I can manually mitigate this with the -DCMAKE_FIND_ROOT_PATH* options in my case, but I don't see e.g. llvm-project/llvm/runtimes doing this anywhere?

mgorny added a comment.Nov 3 2022, 6:53 AM

These workarounds are fine for me, but I wonder if it would be useful with a more direct cmake option to tell it not look for these files at all.

CMake has something like that built-in. I think it's -DCMAKE_DISABLE_FIND_PACKAGE_LLVM=ON.

These workarounds are fine for me, but I wonder if it would be useful with a more direct cmake option to tell it not look for these files at all.

CMake has something like that built-in. I think it's -DCMAKE_DISABLE_FIND_PACKAGE_LLVM=ON.

Oh, interesting, I wasn't aware of that one.

I thought more of the lines of telling compiler-rt to entirely skip the codepaths that try to look for these bits, but maybe it's not necessary in the end. Anyway, I'll follow this effort if there are more refactorings coming up in this area.

thetruestblue added a subscriber: thetruestblue.EditedNov 16 2022, 9:13 AM

This breaks our use case. We run sanitizer tests built in a standalone build using a toolchain from an artifact built upstream-- the toolchain contains binaries only.
Compiler-rt tests rely on the llvm tools binary being in the PATH.

This patch removes the logic that sets the binary tools dir using llvm-config. We don't have llvmConfig.cmake in our toolchain or our build tree and relied on llvm-config to
set(LLVM_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}"directory.

We can work around this by setting the LLVM_TOOLS_BINARY_DIR directly (and potentially other paths... tbd).

But I think we need to reactor this cmake file so that this is considered a valid code path.

This patch removes the logic that sets the binary tools dir using llvm-config. We don't have llvmConfig.cmake in our toolchain or our build tree and relied on llvm-config to
set(LLVM_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}"directory.

While I sympathize with you, I don't think this is valid reason to maintain full compatibility with llvm-config. Unless i'm mistaken, its use for building other LLVM projects standalone was deprecated for a few years already, and compiler-rt was the last project to carry the compatibility code. Furthermore, skipping cmake files from LLVM seems wrong.

thetruestblue added a comment.EditedNov 16 2022, 10:19 AM

While I sympathize with you, I don't think this is valid reason to maintain full compatibility with llvm-config.

I was suggesting a standalone build with LLVM_TOOLS_BINARY_DIR set directly could be a valid code path. We already have code to mock LLVMConfig.cmake. I believe that the mock and setting llvm_tools_dir directly allows compiler-rt is enough to build compiler-rt and run tests without llvm.
Or perhaps a flag to indicate we're mocking llvmconfig?

The goal is to generate compiler-rt lit test suites for an arbitrary LLVM toolchain and then run the tests against it.

Furthermore, skipping cmake files from LLVM seems wrong.

Not sure what you mean. Isn't this the default behavior for toolchain builds and standalone builds? If only compiler-rt is built, llvmconfig.cmake isn't built/added to build tree. Same for toolchains, which contain only binary files. I may be misunderstanding what you mean, though.

This patch removes the logic that sets the binary tools dir using llvm-config. We don't have llvmConfig.cmake in our toolchain or our build tree and relied on llvm-config to
set(LLVM_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}"directory.

While I sympathize with you, I don't think this is valid reason to maintain full compatibility with llvm-config. Unless i'm mistaken, its use for building other LLVM projects standalone was deprecated for a few years already, and compiler-rt was the last project to carry the compatibility code. Furthermore, skipping cmake files from LLVM seems wrong.

@mgorny To add a bit of context to this discussion. We are not asking for compatibility with the llvm-config binary. I'm fine if that goes way, which it already has.

What we want to do is make sure is that our use case still works. I know that "skipping cmake files from LLVM seems wrong" but there is a very good reason for this. We have a testing configuration where we consume pre-built toolchains and run the compiler-rt tests against these. We use the standalone CMake configure of compiler-rt to generate the test suites, nothing actually gets built.
In this scenario the toolchain does not include LLVM CMake files because we do not ship them to customers. Making it a hard requirement to have those CMake files would prevent us from doing that kind of testing.

I suggest that @thetruestblue and I draft a patch so our intentions our clearer.

Well, I'm certainly not opposed to making all the paths configurable. However, I'm not sure if having CMake file accessible one way or another wouldn't eventually be a necessity. For one thing, I would like to move more common code from standalone build codepaths of individual projects into a dedicated CMake file in LLVM (and while I don't want to speak of others, it seems that there are at least few other people who would like to see something similar done). I suppose this wouldn't be an outright blocker if llvm/cmake/Modules directory were present but I'm not 100% sure.

@thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need?

@thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need?

TBD.
So far this seems like the only necessary path. But I am working to confirm this.

@thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need?

TBD.
So far this seems like the only necessary path. But I am working to confirm this.

Ok, I just looked at the code and I think all the variables that were set by llvm-config can also be set on the command line, so you should be able to specify everything manually to get it to work. I'm curious where do you get your sources from? Do you start with the full monorepo sources or do you use the compiler-rt standalone tarball?

Well, I'm certainly not opposed to making all the paths configurable. However, I'm not sure if having CMake file accessible one way or another wouldn't eventually be a necessity. For one thing, I would like to move more common code from standalone build codepaths of individual projects into a dedicated CMake file in LLVM (and while I don't want to speak of others, it seems that there are at least few other people who would like to see something similar done). I suppose this wouldn't be an outright blocker if llvm/cmake/Modules directory were present but I'm not 100% sure.

Isn't this two kinda separate concerns? If I build llvm but only install the executables (no libLLVMSupport.a, no cmake files etc), there's no LLVMConfig.cmake. But if I have the full llvm-project monorepo around, all the cmake helper scripts are available to be found in the monorepo - there's just no LLVMConfig.cmake (which is specific for one build of the llvm tools).

Well, I'm certainly not opposed to making all the paths configurable. However, I'm not sure if having CMake file accessible one way or another wouldn't eventually be a necessity. For one thing, I would like to move more common code from standalone build codepaths of individual projects into a dedicated CMake file in LLVM (and while I don't want to speak of others, it seems that there are at least few other people who would like to see something similar done). I suppose this wouldn't be an outright blocker if llvm/cmake/Modules directory were present but I'm not 100% sure.

@mgorny In our use case the full llvm-project source tree is available. In fact CompilerRTMockLLVMCMakeConfig which we use currently relies on this. So I am 100% fine with CMake code being refactored out of compiler-rt to elsewhere in the source tree (if it makes sense). What we don't have available is the "build tree/install tree" LLVMConfig.cmake file (and friends) because we don't ship it in our toolchain.