[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 inline comments.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake:36
+    execute_process(
+      COMMAND "${CMAKE_C_COMPILER}" -print-target-triple
+      RESULT_VARIABLE HAD_ERROR
----------------
mstorsjo wrote:
> Clang supports `-dumpmachine` too - is there any functional difference between the two?
Not that I know of. I wrote support for clang first and then I thought I should probably add gcc support just incase that's what you were using to build compiler-rt. I can merge the branches to use the same flag if that's what you prefer.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake:47
+  else()
+    message(FATAL_ERROR
+      "Fetching target triple from compiler \"${CMAKE_C_COMPILER_ID}\" "
----------------
mstorsjo wrote:
> If building with e.g. `-DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE`, which sets `TARGET_TRIPLE` somehow (I don't know offhand where/how), I guess that we shouldn't try to do this detection here, but just go with whatever was provided that way?
> 
> It also seems like it's possible to build by just setting `TARGET_TRIPLE` only (without setting `COMPILER_RT_DEFAULT_TARGET_ONLY`) - that kinda works but has odd/unexpected effects for me (by building both i386 and x86_64 at the same time, when I've been used to only building one arch at a time).
> If building with e.g. -DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE, which sets TARGET_TRIPLE somehow (I don't know offhand where/how),

Are you sure about this? I can't reproduce when running like this (without this patch applied)

```
cmake  -GNinja -DLLVM_CONFIG_PATH="" -DCMAKE_C_COMPILER_TARGET=x86_64-apple-darwin20.3.0  -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE -DCOMPILER_RT_INCLUDE_TESTS=ON /path/compiler-rt
```

Where are you looking for the value of `TARGET_TRIPLE` to show up? One place you can look for it in your build directory is `unittests/lit.common.unit.configured`. I see

```
config.target_triple = ""
```

Note don't look at `test/lit.common.configured`. That has `COMPILER_RT_DEFAULT_TARGET_TRIPLE` expanded and not `TARGET_TRIPLE` into the `"target_triple"` lit config, which is extremely confusing.

> I guess that we shouldn't try to do this detection here, but just go with whatever was provided that way?

We could... I guess we're interacting with this existing code...

```lang=cmake
macro(construct_compiler_rt_default_triple)
  if(COMPILER_RT_DEFAULT_TARGET_ONLY)
    if(DEFINED COMPILER_RT_DEFAULT_TARGET_TRIPLE)
      message(FATAL_ERROR "COMPILER_RT_DEFAULT_TARGET_TRIPLE isn't supported when building for default target only")
    endif()
    set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${CMAKE_C_COMPILER_TARGET})
  else()
    set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${TARGET_TRIPLE} CACHE STRING
          "Default triple for which compiler-rt runtimes will be built.")
  endif()
  ...
```

So I guess `compiler_rt_mock_llvm_cmake_config_set_target_triple` could have an early exit... to use `CMAKE_C_COMPILER_TARGET` if `COMPILER_RT_DEFAULT_TARGET_ONLY` is set. However, there's no point doing that if `TARGET_TRIPLE` is actually getting set elsewhere because the results will just overwritten.

Can you confirm that `TARGET_TRIPLE` is actually being set using the method I outlined above?

> It also seems like it's possible to build by just setting TARGET_TRIPLE only (without setting COMPILER_RT_DEFAULT_TARGET_ONLY) - that kinda works but has odd/unexpected effects for me (by building both i386 and x86_64 at the same time, when I've been used to only building one arch at a time).

I'd said this is both expected and unexpected. I'd expect that because compiler-rt normally builds for multiple architectures (at least it does in the configurations I normally use) and so the name of `COMPILER_RT_DEFAULT_TARGET_ONLY` does what it suggests, it builds only one target. So if you don't use the option you get builds for multiple architectures.

It's also a little unexpected because having `TARGET_TRIPLE` be a single architecture might suggest that's the only thing compiler-rt should compile for. But as you've seen that's not what happens.

The fact that this variable exists at all and is used is feels like a build system design mistake but fixing that isn't really in the scope of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99621