[CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag

[phosek]

Summary

These will be replaced by CMake's check_linker_flag once we update
the minimum CMake version 3.20.

[phosek]

Ideally we would land D143052 but the 3.20 bump is getting pushed back due to buildbot issues so I'd like to land this in the meantime to unblock other cleanup.

[mstorsjo]

LGTM

[smeenai]

cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake

3–8

Is it intentional that you're checking for the check_compiler_linker_flag command here but using the check_linker_flag command below?

[mstorsjo]

cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake

3–8

Oh, that does indeed look like a bug - good catch, sorry for not noticing!

[smeenai]

LGTM

[fsb4000]

Hi!

I'm going to revert this commit.

git bisect shows that after that commit libc++ tests started to fail.

b00aaab730ae8bd7f8a44e1808e668e20c2c9282 is the first bad commit
commit b00aaab730ae8bd7f8a44e1808e668e20c2c9282
Author: Petr Hosek <[e-mail]>
Date:   Thu Mar 9 18:23:04 2023 +0000

    [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag

    These will be replaced by CMake's check_linker_flag once we update
    the minimum CMake version 3.20.

    Differential Revision: https://reviews.llvm.org/D145716

 cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake | 41 ++++++++++---------------
 1 file changed, 17 insertions(+), 24 deletions(-)

Look at the recent "Scheduled build": https://buildkite.com/llvm-project/libcxx-ci/builds/20490

I created https://reviews.llvm.org/D145858 and it looks the tests pass after revertion: https://buildkite.com/llvm-project/libcxx-ci/builds/20495

Update: FreeBSD tests failed, but that probably different. Windows tests passed at least...

[mstorsjo]

I took a look at what's going wrong here; I diffed the cmake output from a cmake run before and after this change:

@@ -17,12 +17,12 @@
 -- Performing Test LLVM_RUNTIMES_LINKING_WORKS
 -- Performing Test LLVM_RUNTIMES_LINKING_WORKS - Success
 -- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG
--- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Failed
+-- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Success
 -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG
 -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG - Failed
 -- Using Release VC++ CRT: MD
 -- Performing Test SUPPORTS_BREPRO
--- Performing Test SUPPORTS_BREPRO - Success
+-- Performing Test SUPPORTS_BREPRO - Failed
 -- Looking for os_signpost_interval_begin
 -- Looking for os_signpost_interval_begin - not found
 -- Found Python3: /usr/bin/python3.10 (found version "3.10.6") found components: Interpreter
@@ -33,8 +33,6 @@
 -- Using libc++ testing configuration: /home/martin/code/llvm-project/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in
 -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG
 -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG - Failed
--- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG
--- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG - Failed
 -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA
 -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA - Failed
 -- Performing Test CXX_SUPPORTS_FALIGNED_ALLOCATION_FLAG
@@ -44,71 +42,65 @@
 -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG
 -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG - Failed
 -- Performing Test CXX_SUPPORTS_W4_FLAG
--- Performing Test CXX_SUPPORTS_W4_FLAG - Success
+-- Performing Test CXX_SUPPORTS_W4_FLAG - Failed
 -- Performing Test CXX_SUPPORTS_WEXTRA_FLAG

I.e. this change makes CXX_SUPPORTS_NOSTDLIBXX_FLAG suddenly succeed where it was failing before. This causes all further checks to fail.

The reason for this is ... complex. When running a compile+link test with a non-MSVC compiler, you first compile the source file with $CC (where CMAKE_C_FLAGS and CMAKE_REQUIRED_FLAGS are included), then you link it with $CC, where CMAKE_REQUIRED_FLAGS and CMAKE_EXE_LINKER_FLAGS are included.

When doing a compile+link test with a MSVC style compiler, you compile it with $CC with the same flags as above. But then cmake does the linking by invoking link.exe/lld-link directly, where it only includes CMAKE_EXE_LINKER_FLAGS.

In the case of -nostdlibc++, clang-cl warns about the warning being unknown, but at least lld-link seems to accept the flag silently. Before, when we were adding the flag to CMAKE_REQUIRED_FLAGS, the test compile+link itself succeeded, but it was still deemed a failure due to it producing clang-16: warning: unknown argument ignored in clang-cl: '-nostdlib++' [-Wunknown-argument]. Now after this refactoring, the flag is only added to CMAKE_EXE_LINKER_FLAGS, where it doesn't produce any warning. But if this test succeeded, in runtimes/CMakeLists.txt we go on to add the flag to CMAKE_REQUIRED_FLAGS, where it later breaks every future test.

I don't have the whole picture in my mind right now, but I get the feeling that if we're planning to add -nostdlib++ to CMAKE_REQUIRED_FLAGS, we also should be using a test that actually tests setting the flag in CMAKE_REQUIRED_FLAGS.

[phosek]

In D145716#4188311, @mstorsjo wrote:

I took a look at what's going wrong here; I diffed the cmake output from a cmake run before and after this change:

@@ -17,12 +17,12 @@
 -- Performing Test LLVM_RUNTIMES_LINKING_WORKS
 -- Performing Test LLVM_RUNTIMES_LINKING_WORKS - Success
 -- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG
--- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Failed
+-- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Success
 -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG
 -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG - Failed
 -- Using Release VC++ CRT: MD
 -- Performing Test SUPPORTS_BREPRO
--- Performing Test SUPPORTS_BREPRO - Success
+-- Performing Test SUPPORTS_BREPRO - Failed
 -- Looking for os_signpost_interval_begin
 -- Looking for os_signpost_interval_begin - not found
 -- Found Python3: /usr/bin/python3.10 (found version "3.10.6") found components: Interpreter
@@ -33,8 +33,6 @@
 -- Using libc++ testing configuration: /home/martin/code/llvm-project/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in
 -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG
 -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG - Failed
--- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG
--- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG - Failed
 -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA
 -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA - Failed
 -- Performing Test CXX_SUPPORTS_FALIGNED_ALLOCATION_FLAG
@@ -44,71 +42,65 @@
 -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG
 -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG - Failed
 -- Performing Test CXX_SUPPORTS_W4_FLAG
--- Performing Test CXX_SUPPORTS_W4_FLAG - Success
+-- Performing Test CXX_SUPPORTS_W4_FLAG - Failed
 -- Performing Test CXX_SUPPORTS_WEXTRA_FLAG

I.e. this change makes CXX_SUPPORTS_NOSTDLIBXX_FLAG suddenly succeed where it was failing before. This causes all further checks to fail.

The reason for this is ... complex. When running a compile+link test with a non-MSVC compiler, you first compile the source file with $CC (where CMAKE_C_FLAGS and CMAKE_REQUIRED_FLAGS are included), then you link it with $CC, where CMAKE_REQUIRED_FLAGS and CMAKE_EXE_LINKER_FLAGS are included.

When doing a compile+link test with a MSVC style compiler, you compile it with $CC with the same flags as above. But then cmake does the linking by invoking link.exe/lld-link directly, where it only includes CMAKE_EXE_LINKER_FLAGS.

In the case of -nostdlibc++, clang-cl warns about the warning being unknown, but at least lld-link seems to accept the flag silently. Before, when we were adding the flag to CMAKE_REQUIRED_FLAGS, the test compile+link itself succeeded, but it was still deemed a failure due to it producing clang-16: warning: unknown argument ignored in clang-cl: '-nostdlib++' [-Wunknown-argument]. Now after this refactoring, the flag is only added to CMAKE_EXE_LINKER_FLAGS, where it doesn't produce any warning. But if this test succeeded, in runtimes/CMakeLists.txt we go on to add the flag to CMAKE_REQUIRED_FLAGS, where it later breaks every future test.

I don't have the whole picture in my mind right now, but I get the feeling that if we're planning to add -nostdlib++ to CMAKE_REQUIRED_FLAGS, we also should be using a test that actually tests setting the flag in CMAKE_REQUIRED_FLAGS.

Thank you for looking into this, that analysis was really helpful. I think the correct solution is to use CMAKE_REQUIRED_LINK_OPTIONS instead of CMAKE_REQUIRED_FLAGS, but that can only be done after we bump the minimum CMake version past 3.14 (in fact we should review every use llvm_check_compiler_linker_flag and switch to use CMAKE_REQUIRED_LINK_OPTIONS in other places). I think we should also teach clang-cl to silently accept -nostdlib++ the same way clang does.

For now as a temporary workaround I just excluded MSVC for the -nostdlib++ flag and added a TODO comment.

[mstorsjo]

Thanks for the revert - I was also just looking into a build issue that seems to be caused by this commit.

runtimes/CMakeLists.txt

144 ↗

(On Diff #508896)

I believe that this is what caused the failures for me; check_cxx_compiler_flag only passes the flag to compilation, not to linking. If the C++ library isn't available yet, this will fail, since it's essential that -nostdlib++ gets passed to the linking command (too).

[mstorsjo]

Btw, for issues like these, it would be super useful if there was a CI configuration that builds libcxx/libunwind/libcxxabi from scratch in an environment where there's no preexisting unwinder or C++ library.

I considered adding such a config for the mingw builds, by having the CI run script remove lib{c++,unwind}.* and the C++ headers from the toolchain before doing the regular CI build - but that doesn't work, since the CI environment doesn't start up from a clean container image every time, but runs persistently within a container, so any modifications to the toolchain within a CI job does carry over to later jobs too.

cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake

-include(CMakePushCheckState)
+include(CheckLinkerFlag OPTIONAL)
 
-include(CheckCompilerFlag OPTIONAL)
+if (COMMAND check_linker_flag)
+  macro(llvm_check_compiler_linker_flag)
+    check_linker_flag(${ARGN})
+  endmacro()
+else()
+  # Until the minimum CMAKE version is 3.18
 
-if(NOT COMMAND check_compiler_flag)
-  include(CheckCCompilerFlag)
   include(CheckCXXCompilerFlag)
-endif()
-
-function(llvm_check_compiler_linker_flag lang flag out_var)
-  # If testing a flag with check_c_compiler_flag, it gets added to the compile
-  # command only, but not to the linker command in that test. If the flag
-  # is vital for linking to succeed, the test would fail even if it would
-  # have succeeded if it was included on both commands.
-  #
-  # Therefore, try adding the flag to CMAKE_REQUIRED_FLAGS, which gets
-  # added to both compiling and linking commands in the tests.
 
-  cmake_push_check_state()
-  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} ${flag}")
-  if(COMMAND check_compiler_flag)
-    check_compiler_flag("${lang}" "" ${out_var})
-  else()
-    # Until the minimum CMAKE version is 3.19
-    # cmake builtin compatible, except we assume lang is C or CXX
+  function(llvm_check_compiler_linker_flag lang flag out_var)
+    cmake_policy(PUSH)
+    cmake_policy(SET CMP0056 NEW)
+    set(_CMAKE_EXE_LINKER_FLAGS_SAVE ${CMAKE_EXE_LINKER_FLAGS})
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${flag}")
     if("${lang}" STREQUAL "C")
       check_c_compiler_flag("" ${out_var})
     elseif("${lang}" STREQUAL "CXX")
       check_cxx_compiler_flag("" ${out_var})
     else()
       message(FATAL_ERROR "\"${lang}\" is not C or CXX")
     endif()
-  endif()
-  cmake_pop_check_state()
+    set(CMAKE_EXE_LINKER_FLAGS ${_CMAKE_EXE_LINKER_FLAGS_SAVE})
+    cmake_policy(POP)
   endfunction()
+endif()