[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:47
+  else()
+    message(FATAL_ERROR
+      "Fetching target triple from compiler \"${CMAKE_C_COMPILER_ID}\" "
----------------
mstorsjo wrote:
> delcypher wrote:
> > 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.
> Sorry - I wasn't sure it was setting `TARGET_TRIPLE` - I just kinda assumed that was the internal variable that ends up set.
> 
> If I configure that way, I actually get the default `TARGET_TRIPLE` of the llvm build (provided that llvm-config can be found), not the one I set in `CMAKE_C_COMPILER_TARGET`.
> 
> So, sorry for the confusion.
> 
> (But I see that when building via llvm/runtimes, it sets both `COMPILER_RT_DEFAULT_TARGET_ONLY`, and I think, `TARGET_TRIPLE`, but not `CMAKE_C_COMPILER_TARGET`, but when I try doing that myself, it errors out due to the list of architectures ending up empty, " -- Compiler-RT supported architectures:")
> 
> Doing anything about the case of building multiple architectures at once is out of scope indeed. (Do you happen to know how to request what set of architectures to build for, can I request building for eg i386, x86_64, armv7 and aarch64 at once, of is it just all x86 variants at once?)
> Do you happen to know how to request what set of architectures to build for, can I request building for eg i386, x86_64, armv7 and aarch64 at once, of is it just all x86 variants at once?)

I don't know of a way. Looking at `test_targets()` in `cmake/config-ix.cmake` that ends up populating the `COMPILER_RT_SUPPORTED_ARCH` list with of architectures that `test_targets()` managed to detect support for (provided the host isn't an Apple platform). Later on in `cmake/config-ix.cmake` there are calls to `filter_available_targets()` (not on Apple platforms) that set the supported targets for various different sanitizers. The variables here look like normal variables rather than CMake cache variables so I don't think you'll be able to override the behaviour here using `-DCOMPILER_RT_SUPPORTED_ARCH` unless some changes are made to the existing code. I could be wrong about this though...



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99621