[PATCH] D143052: [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

In D143052#4095952 <https://reviews.llvm.org/D143052#4095952>, @mstorsjo wrote:

> LGTM, this looks great!
>
> I wonder why we didn't think of that before (in particular, using `CMAKE_EXE_LINKER_FLAGS` instead of `CMAKE_REQUIRED_FLAGS` before) - I wonder if there's some pitfalls we don't come to think of right now? I can put this through a full bootstrap build of mine too. And I guess we'll see if the libcxx CI says anything.
>
> What are the practical differences between `CMAKE_EXE_LINKER_FLAGS` and `CMAKE_REQUIRED_FLAGS` here - both get used in all linking tests, while `CMAKE_REQUIRED_FLAGS` also gets used in pure compilation tests (which is suboptimal)? In a large comment in `runtimes/CMakeLists.txt`, I've noted that we should move to using `CMAKE_REQUIRED_LINK_OPTIONS` when we can require cmake 3.14 - what would be the differences between `CMAKE_REQUIRED_LINK_OPTIONS` and `CMAKE_EXE_LINKER_FLAGS` here?
>
> (I've noted one such difference when testing `CMAKE_REQUIRED_LINK_OPTIONS`; when testing with `CMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY`, the flags from `CMAKE_REQUIRED_LINK_OPTIONS` end up passed to the static library tool, which fails - and that wouldn't be the issue with `CMAKE_EXE_LINKER_FLAGS`.)

`CMAKE_EXE_LINKER_FLAGS` instead of `CMAKE_REQUIRED_FLAGS` serve a different purpose. `CMAKE_REQUIRED_FLAGS` are applied only to CMake checks whereas `CMAKE_EXE_LINKER_FLAGS` are also applied to CMake targets so we don't want to be appending flags that are only needed during configuration to it. That's why `llvm_check_linker_flag` saves and restores `CMAKE_EXE_LINKER_FLAGS` (at least that was the intention, I found a bug in the implementation that's addressed in D143088 <https://reviews.llvm.org/D143088>).

Furthermoe, the modern CMake approach is to avoid modifying global variables such as `CMAKE_EXE_LINKER_FLAGS` and instead set flags directly on targets so I think we should avoid relying on that approach in general. Once we update the minimum CMake version, we should switch to `CMAKE_REQUIRED_LINK_OPTIONS` and `check_linker_flag` which I think is the best solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143052