[runtimes] [CMake] Fix checks for -Werror when building with incomplete toolchains

[mstorsjo]

Summary

When we add --unwindlib=none during the CMake configure phase (to
make CMake linking tests succeed before the unwind library has been
built for the first time - when bootstrapping a cross toolchain from
scratch), we add it to CMAKE_REQUIRED_FLAGS to make later CMake tests
pass.

When the option is added to CMAKE_REQUIRED_FLAGS, it gets added to
both compilation and linking commands. When --unwindlib=none is added
to the compilation command, it causes warnings (about being unused
during compilation, as it only affects linking).

When all CMake test compilations produce warnings, later CMake tests
for -Werror fail.

Add --{start,end}-no-unused-arguments around --unwindlib=none, if
supported, to avoid unnecessary warnings due to this option.

If the CMake requirement is bumped to 3.14, we could use
CMAKE_REQUIRED_LINK_OPTIONS instead, removing the need for the
--{start,end}-no-unused-arguments options. (However, do note that
CMAKE_REQUIRED_LINK_OPTIONS is problematic in combination with
CMAKE_TRY_COMPILE_TARGET_TYPE set to STATIC_LIBRARY; see
https://gitlab.kitware.com/cmake/cmake/-/issues/23454.)

[ldionne]

runtimes/CMakeLists.txt

106–124

Shouldn't this be more like

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")
else()
  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --unwindlib=none")
endif()

Right now you seem to be executing

set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --unwindlib=none")

in all cases.

Edit Oh, I see it now. You override CMAKE_REQUIRED_FLAGS with ORIG_CMAKE_REQUIRED_FLAGS when C_SUPPORTS_START_NO_UNUSED_ARGUMENTS is supported. I think it would be clearer to use a if-else instead.

[mstorsjo]

runtimes/CMakeLists.txt

106–124

That's actually how I wrote it first, but it doesn't work quite like that.

The problem is that if we're using an incomplete toolchain where linking fails, the check_c_compiler_flag to check for whether --start-no-unused-arguments is supported also will fail (because cmake tests fail in linking) until we've set --unwindlib=none in CMAKE_REQUIRED_FLAGS. So we must set it first, then test for the other option, and then finally see if we can revise it later.

[ldionne]

runtimes/CMakeLists.txt

106–124

Tricky!

[phosek]

LGTM

runtimes/CMakeLists.txt

105

This is a minor nit, but it'd be nice to have a naming convention for this pattern. I did a quick search and found various different instances, like ORIG_<name>, <name>_ORIG, SAVED_<name>, __SAVED_<name>. The last one, __SAVED_<name> is the one I'd prefer. It's already used in CMake's own modules and I think it's best at signaling that this is an internal variable that has a specific purpose.

[mstorsjo]

Rebased, rerunning CI. (This codepath probably isn't exercised at all in the CI environment though.)

runtimes/CMakeLists.txt

 if (NOT LLVM_RUNTIMES_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")
+    # 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 the option 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)
+    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")
+    endif()
   endif()
 endif()
 
 # 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(C "-nostdlib++" CXX_SUPPORTS_NOSTDLIBXX_FLAG)