[PATCH] D99621: [CMake][Compiler-rt] Make it possible to configure standalone compiler-rt without `LLVMConfig.cmake`.

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In D99621#2659906 <https://reviews.llvm.org/D99621#2659906>, @phosek wrote:

> In D99621#2659869 <https://reviews.llvm.org/D99621#2659869>, @delcypher wrote:
>
>> In D99621#2659844 <https://reviews.llvm.org/D99621#2659844>, @phosek wrote:
>>
>>> Do you know what we need `LLVMConfig.cmake` for? Could we remove that dependency altogether? It's not used by any other runtime as far as I'm aware.
>>
>> I wouldn't want to remove the dependency at this stage as not enough of this new code has been tested (I only tested on macOS). However, I think this patch could used as a stepping stone towards completely removing the dependency on `LLVMConfig.cmake`. If we landed this and did the necessary work to make this new code path work on all the host platforms we care about then I think we could remove the dependency on `LLVMConfig.cmake`. I'd like to see that happen because the requirement makes testing harder and seems unnecessary.
>
> SGTM
>
>> The actions that the new `compiler_rt_mock_llvm_cmake_config` macro take do what I found to be necessary to get the CMake configure to work without having `LLVMConfig.cmake` present with on a macOS host. Basically this seems to be
>>
>> - Setting `TARGET_TRIPLE`. The build system fails in weird and wonderful ways when this is an empty string. **SHAKES FIST AT CMAKE**.
>
> I see only two dependencies on this variable in https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake#L317 and https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L131.

Those are the dependencies I ran into too but some of the test files depend on it too.

https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/test/fuzzer/lit.site.cfg.py.in#L14
https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/unittests/lit.common.unit.configured.in#L4

> I could see how the build starts misbehaving if that variable isn't set, but it should be also feasible to address that issue given that it's only two references.

Yep, that's what `compiler_rt_mock_llvm_cmake_config_set_target_triple` effectively does. I'm wondering if it should just use `${CMAKE_C_COMPILER_TARGET}` rather than calling llvm-config?

>> - Including `AddLLVM.cmake` from the LLVM source tree. It seems we have some CMake code that depends on code in this file.
>> - Add LLVM's CMake module directory into the CMake module search path.  This necessary for the include of `AddLLVM.cmake` to work.
>
> This should become a non-issue if we assume the monorepo layout.

Yep. That's why this patch depends on https://reviews.llvm.org/D99620.  The `compiler_rt_mock_llvm_cmake_config_include_cmake_files()` and `compiler_rt_mock_llvm_cmake_config_set_cmake_path()` macros do the necessary include via a path that assumes the monorepo.

> It may turn out we need to do more than this to support other platforms.

I think ideally we wouldn't have `CompilerRTMockLLVMCMakeConfig.cmake` at all and instead just fixed all the issues at the right places in the CMake code. Right now though it seems like a good self contained place to mimic the things from `LLVMConfig.cmake` that we depend on. If this file is updated to allow configure on all platforms to succeed then at all point in time I think we could remove `CompilerRTMockLLVMCMakeConfig.cmake` and make the necessary fixes. Unfortunately, I don't have the bandwidth right now to do a bunch of testing on other platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99621