[libcxx-commits] [PATCH] D143052: [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D143052#4097128 <https://reviews.llvm.org/D143052#4097128>, @phosek wrote:

> `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.

Ah, I see - thanks!

Regarding `CMAKE_REQUIRED_LINK_OPTIONS`, as mentioned before, that one is annoying in practice, but I guess it's still the right way forward. I have a local patch to take that into use, where I've had to add this workaround:

  diff --git a/libunwind/CMakeLists.txt b/libunwind/CMakeLists.txt
  index 5c547883f992..81b4d42658a5 100644
  --- a/libunwind/CMakeLists.txt
  +++ b/libunwind/CMakeLists.txt
  @@ -231,9 +231,12 @@ add_cxx_compile_flags_if_supported(-EHsc)
   # This leads to libunwind not being built with this flag, which makes
   # libunwind quite useless in this setup.
   set(_previous_CMAKE_TRY_COMPILE_TARGET_TYPE ${CMAKE_TRY_COMPILE_TARGET_TYPE})
  +set(_previous_CMAKE_REQUIRED_LINK_OPTIONS ${CMAKE_REQUIRED_LINK_OPTIONS})
   set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
  +set(CMAKE_REQUIRED_LINK_OPTIONS)
   add_compile_flags_if_supported(-funwind-tables)
   set(CMAKE_TRY_COMPILE_TARGET_TYPE ${_previous_CMAKE_TRY_COMPILE_TARGET_TYPE})
  +set(CMAKE_REQUIRED_LINK_OPTIONS ${_previous_CMAKE_REQUIRED_LINK_OPTIONS})
   
   if (LIBUNWIND_USES_ARM_EHABI AND NOT CXX_SUPPORTS_FUNWIND_TABLES_FLAG)
     message(SEND_ERROR "The -funwind-tables flag must be supported "


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143052