This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Replace llvm_check_linker_flag and llvm_check_compiler_linker_flag
Needs ReviewPublic

Authored by phosek on Feb 1 2023, 12:30 AM.

Details

Reviewers
mstorsjo
ldionne
DavidSpickett
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGefae3174f095: [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag
Summary

These are both polyfills for check_linker_flag which was introduced in
CMake 3.18 and now that LLVM requires CMake 3.20 as the minimum version
we can use it to simplify our build.

Diff Detail

Event Timeline

phosek created this revision.Feb 1 2023, 12:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 12:30 AM
phosek requested review of this revision.Feb 1 2023, 12:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 12:30 AM
phosek updated this revision to Diff 493860.Feb 1 2023, 12:56 AM
phosek retitled this revision from [CMake] Only set the linker flag for link commands to [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag.
phosek edited the summary of this revision. (Show Details)
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptFeb 1 2023, 12:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, Restricted Project, Enna1. · View Herald Transcript
mstorsjo accepted this revision.Feb 1 2023, 2:00 AM

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

I can put this through a full bootstrap build of mine too.

It built fine in a from-scratch build for me, so it looks good in that aspect!

And I guess we'll see if the libcxx CI says anything.

It seems like the CI is hitting a bunch of seemingly unrelated issues; can you rebase the patch on top of latest main to retrigger the CI, where I'd hope the unrelated issues are fixed?

phosek added a comment.Feb 1 2023, 9:38 AM

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

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.

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

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 "
phosek added a comment.Feb 1 2023, 8:47 PM

@ldionne would it be possible to take a look?

phosek updated this revision to Diff 498965.Feb 20 2023, 2:51 PM
phosek updated this revision to Diff 499289.Feb 21 2023, 1:55 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 21 2023, 8:25 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

I suspect this commit has broken the flang standalone build: https://lab.llvm.org/buildbot/#/builders/175/builds/25443

LLVMCheckCompilerLinkerFlag.cmake used to be present in the build dir, but is now not because it's in the top level cmake folder.

Building Flang as a standalone project.
CMake Error at /home/tcwg-buildbot/worker/flang-aarch64-out-of-tree/build_llvm/lib/cmake/llvm/HandleLLVMStdlib.cmake:16 (include):
  include could not find requested file:
    LLVMCheckLinkerFlag
Call Stack (most recent call first):
  /home/tcwg-buildbot/worker/flang-aarch64-out-of-tree/build_llvm/lib/cmake/llvm/HandleLLVMOptions.cmake:11 (include)
  CMakeLists.txt:107 (include)

Is the intention that we handle this like CMakePolicy.cmake and pull it from the source dir when we build flang, or is it a mistake to move it to the top level?

Due to the, unique, way the bot is configured, this is actually the build that first included the change: https://lab.llvm.org/buildbot/#/builders/175/builds/25426
(because it only triggers on certain subprojects it only shows the commits from those projects, which causes major confusion)

That passed, because we did not clean the build dir. The next time we did clear it was https://lab.llvm.org/buildbot/#/builders/175/builds/25433, which failed as shown before.

(because it only triggers on certain subprojects it only shows the commits from those projects, which causes major confusion)

Turns out Buildbot uses the Github API in a way that doesn't include the previous filenames. So a bot looking at llvm changes does not see a commit that only moves files out of llvm.

Is the intention that we handle this like CMakePolicy.cmake and pull it from the source dir when we build flang, or is it a mistake to move it to the top level?

Yes that's the intention. I'm also fine reverting this change; after D144509 lands which presumably will be soon, we should be able to just use check_linker_flag directly.

I suspect we'll want to use more common modules in the LLVM build in the future though so we'll need a better solution to avoid future such failures.

phosek updated this revision to Diff 499561.Feb 22 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 9:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
phosek reopened this revision.Feb 22 2023, 9:39 AM
phosek retitled this revision from [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag to [CMake] Replace llvm_check_linker_flag and llvm_check_compiler_linker_flag.
phosek edited the summary of this revision. (Show Details)

What is the status of this? We now require CMake 3.20, can this be rebased and landed in some form?

phosek added a comment.Jul 4 2023, 8:05 PM

What is the status of this? We now require CMake 3.20, can this be rebased and landed in some form?

The issue I ran into is that check_linker_flag annoyingly behaves differently from check_{c,cxx}_compiler_flag.

  • With check_{c,cxx}_compiler_flag, CMake appends CMAKE_REQUIRED_FLAGS to the flag before invoking try_compile.
  • With check_linker_flag, CMake replaces the content of CMAKE_REQUIRED_LINK_OPTIONS with the flag.

This somewhat reduces the utility of check_linker_flag and also means it behaves differently from llvm_check_compiler_linker_flag which is based on check_{c,cxx}_compiler_flag.

What I'm considering now is keeping llvm_check_linker_flag, but changing its implementation to be based on check_linker_flag while preserving CMAKE_REQUIRED_LINK_OPTIONS.