[PATCH] D26653: [compiler-rt] Support building builtins for a single target

[Chris Bieneman via llvm-commits]

beanz added a comment.

I have one long comment below, but otherwise this is great. Thanks for working on this!



================
Comment at: cmake/Modules/CompilerRTUtils.cmake:232
+  endif()
+  set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${COMPILER_RT_COMPILER_TARGET} CACHE STRING
       "Default triple for which compiler-rt runtimes will be built.")
----------------
This is actually bad because of the way CMake's caching behavior works. `CMAKE_C_COMPILER_TARGET` if set would be a cached variable, and defining another cached variable off of it means that if you update one `CMAKE_C_COMPILER_TARGET` `COMPILER_RT_DEFAULT_TARGET_TRIPLE` won't be updated as well.

I'm not sure if there is a good way to change this without breaking existing users. If you can't come up with something can you add a check to ensure that if `CMAKE_C_COMPILER_TARGET` is defined, it is the same as `COMPILER_RT_DEFAULT_TARGET_TRIPLE`? That will at least tell users when something goes wrong.

An alternative approach that appeals to me would be to have the code paths based on `COMPILER_RT_DEFAULT_TARGET_ONLY` use `CMAKE_C_COMPILER_TARGET` instead of `COMPILER_RT_DEFAULT_TARGET_TRIPLE`, and make an error if `COMPILER_RT_DEFAULT_TARGET_TRIPLE` is specified without `CMAKE_C_COMPILER_TARGET`.

The reason I like that approach is that as users migrate onto `COMPILER_RT_DEFAULT_TARGET_ONLY` we will be encouraging them to also migrate to using CMake's builtin cross-targeting mechanisms instead of the hand rolled ones.


Repository:
  rL LLVM

https://reviews.llvm.org/D26653