[CMake][runtimes] Redo the --unwindlib=none and -nostdlib++ checks

[phosek]

Summary

Recent CMake change made the unused argument warning more strict:
https://gitlab.kitware.com/cmake/cmake/-/commit/950effe434623b50a1c85e2c53ed592210e79ceb

After that change, runtimes build started failing. Upon investigation,
it turned out that our --unwindlib=none and -nostdlib++ checks weren't
correct since the flags wouldn't be correct wrapped in
--{start,end}-no-unused-arguments. Furthermore, the
llvm_check_compiler_linker_flag macro wouldn't correctly propagate the
flag.

[phosek]

runtimes/CMakeLists.txt

112–113

@mstorsjo do you know what's the reasoning behind this? I was wondering if we could add --unwindlib=none and -nostdlib++ unconditionally to simplify this logic and make the behavior more deterministic.

[mstorsjo]

runtimes/CMakeLists.txt

112–113

Sorry, I don't remember all the details exactly - but I think the issue was exposed in the libc++ CI builds, for the ASAN/TSAN builds there. I guess the easiest would be to put up a test patch that adds it unconditionally, and see what issues we hit in CI.

[mstorsjo]

cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake

16

Is there any functional change to the changes to the helper macro/function here, or is this just a pure refactoring? I'd kinda prefer this change (which does seem reasonable) in a separate commit if possible, with separate explanation/motivation for it.

runtimes/CMakeLists.txt

116

Does the change from manually backed up flags to cmake_push_check_state have any functional effect here, or is it a pure refactoring?

143

This doesn't seem right to me, and/or changes the behaviour from before? Even if C++ linking does work out of the box (i.e. we have a complete environment with a preexisting standard C++ library), we'd still want to use -nostdlib++ (we did before at least)?

151

Does this break things for GCC cases? We're conflating the checks for --start-no-unused-arguments and --unwindlib=none, -nostdlib++ and -nostdinc++. For --unwindlib=none, that's probably fine, since AFAIK Clang is the only one supporting both (and if we don't support Clang older than a couple versions, both options should be supported equally). For GCC, I think some of -nostdlib++ or -nostdinc++ might be supported (I remember one of them being discussed of being added quite recently).

So for that case, I think it might be better if we'd keep it so that we check for support for the --start-no-unused-arguments --end-no-unused-arguments option first, and then use that to either wrap or not wrap the other options when they are tested.

[mstorsjo]

runtimes/CMakeLists.txt

112–113

I tried running a patch through CI, removing the if (NOT LLVM_RUNTIMES_LINKING_WORKS) condition from around adding --unwindlib=none, and it no longer broke anything. So that's a bit unsettling...

I dug up the original case where I added this, https://reviews.llvm.org/D113253#3112444, and the CI build that failed before I added that check:
https://buildkite.com/llvm-project/libcxx-ci/builds/6433

There, essentially all cmake tests failed after adding the option, and this then broke the cmake configure in libunwind (otherwise it would probably have been a severely broken build anyway).

I'm not quite sure what has changed since, that makes it no longer break though. I guess it would be possible to bisect somehow...

[ldionne]

Let's try to rebase this onto main and push this forward, since this is blocking @Mordante 's work on modules. Thanks for looking into this, though!

cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake

8

I think you mean Until the minimum CMake version is 3.18, since check_linker_flag was introduced in CMake 3.18.

runtimes/CMakeLists.txt

112–113

Let's remove it, then! Perhaps in a separate patch?

143

Yeah I think I agree, we always want to use -nostdlib++.

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.19
 
-if(NOT COMMAND check_compiler_flag)
   include(CheckCCompilerFlag)
   include(CheckCXXCompilerFlag)
-endif()
+  include(CMakePushCheckState)
 
   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_policy(PUSH)
+    cmake_policy(SET CMP0056 NEW)
     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
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${flag}")
     # cmake builtin compatible, except we assume lang is C or CXX
     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()
   endfunction()
+endif()

runtimes/CMakeLists.txt

 
 # This variable is used by individual runtimes to locate LLVM files.
 set(LLVM_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../llvm)
 
 include(CheckLibraryExists)
 include(LLVMCheckCompilerLinkerFlag)
 include(CheckCCompilerFlag)
 include(CheckCXXCompilerFlag)
+include(CMakePushCheckState)
 
 # CMake omits default compiler include paths, but in runtimes build, we use
 # -nostdinc and -nostdinc++ and control include paths manually so this behavior
 # is undesirable. Filtering CMAKE_{LANG}_IMPLICIT_INCLUDE_DIRECTORIES to remove
 # paths that are inside the build directory disables this behavior.
 #
 # See https://gitlab.kitware.com/cmake/cmake/-/issues/19227 for further details.
 
 function(filter_prefixed list prefix outvar)
   foreach(str ${list})
     string(FIND "${str}" "${prefix}" out)
     if(NOT "${out}" EQUAL 0)
       list(APPEND result ${str})
     endif()
   endforeach()
   set(${outvar} ${result} PARENT_SCOPE)
 endfunction()
 
 filter_prefixed("${CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES}" ${LLVM_BINARY_DIR} CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES)
 filter_prefixed("${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES}" ${LLVM_BINARY_DIR} CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES)
 filter_prefixed("${CMAKE_ASM_IMPLICIT_INCLUDE_DIRECTORIES}" ${LLVM_BINARY_DIR} CMAKE_ASM_IMPLICIT_INCLUDE_DIRECTORIES)
 
-check_c_compiler_flag("" LLVM_RUNTIMES_LINKING_WORKS)
-if (NOT LLVM_RUNTIMES_LINKING_WORKS)
+check_c_compiler_flag("" C_LINKING_WORKS)
+if (NOT C_LINKING_WORKS)
   # The compiler driver may be implicitly trying to link against libunwind,
   # which might not work if libunwind doesn't exist yet. Try to check if
   # --unwindlib=none is supported, and use that if possible.
   # Don't add this if not necessary to fix linking, as it can break using
   # e.g. ASAN/TSAN.
   llvm_check_compiler_linker_flag(C "--unwindlib=none" CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG)
   if (CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG)
-    set(ORIG_CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")
-    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --unwindlib=none")
+    cmake_push_check_state()
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments --unwindlib=none --end-no-unused-arguments")
+
     # TODO: When we can require CMake 3.14, we should use
     # CMAKE_REQUIRED_LINK_OPTIONS here. Until then, we need a workaround:
     # When using CMAKE_REQUIRED_FLAGS, this option gets added both to
     # compilation and linking commands. That causes warnings in the
     # compilation commands during cmake tests. This is normally benign, but
     # when testing whether -Werror works, that test fails (due to the
     # preexisting warning).
     #
     # Therefore, before we can use CMAKE_REQUIRED_LINK_OPTIONS, check if we
     # can use --start-no-unused-arguments to silence the warnings about
     # --unwindlib=none during compilation.
     #
     # We must first add --unwindlib=none to CMAKE_REQUIRED_FLAGS above, to
     # allow this subsequent test to succeed, then rewrite CMAKE_REQUIRED_FLAGS
     # below.
-    check_c_compiler_flag("--start-no-unused-arguments" C_SUPPORTS_START_NO_UNUSED_ARGUMENTS)
+    check_c_compiler_flag("" C_SUPPORTS_START_NO_UNUSED_ARGUMENTS)
+    cmake_pop_check_state()
     if (C_SUPPORTS_START_NO_UNUSED_ARGUMENTS)
-      set(CMAKE_REQUIRED_FLAGS "${ORIG_CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments --unwindlib=none --end-no-unused-arguments")
+      set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments --unwindlib=none --end-no-unused-arguments")
     endif()
   endif()
 endif()
 
+check_cxx_compiler_flag("" CXX_LINKING_WORKS)
+if (NOT CXX_LINKING_WORKS)
   # Disable use of the installed C++ standard library when building runtimes.
   # Check for -nostdlib++ first; if there's no C++ standard library yet,
   # all check_cxx_compiler_flag commands will fail until we add -nostdlib++
   # (or -nodefaultlibs).
   llvm_check_compiler_linker_flag(CXX "-nostdlib++" CXX_SUPPORTS_NOSTDLIBXX_FLAG)
   if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
-  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nostdlib++")
+    cmake_push_check_state()
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments -nostdlib++ --end-no-unused-arguments")
+    check_cxx_compiler_flag("" CXX_SUPPORTS_START_NO_UNUSED_ARGUMENTS)
+    cmake_pop_check_state()
+    if (CXX_SUPPORTS_START_NO_UNUSED_ARGUMENTS)
+      set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments -nostdlib++ --end-no-unused-arguments")
+    endif()
   endif()
-check_cxx_compiler_flag(-nostdinc++ CXX_SUPPORTS_NOSTDINCXX_FLAG)
-if (CXX_SUPPORTS_NOSTDINCXX_FLAG)
-  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nostdinc++")
 endif()
 
 # Avoid checking whether the compiler is working.
 set(LLVM_COMPILER_CHECKED ON)
 
 # Handle common options used by all runtimes.
 include(AddLLVM)
 include(HandleLLVMOptions)