This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag
ClosedPublic

Authored by phosek on Mar 9 2023, 11:40 AM.

Details

Summary

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

Diff Detail

Event Timeline

phosek created this revision.Mar 9 2023, 11:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 11:40 AM
phosek requested review of this revision.Mar 9 2023, 11:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 11:40 AM

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 accepted this revision.Mar 9 2023, 1:29 PM

LGTM

This revision is now accepted and ready to land.Mar 9 2023, 1:29 PM
smeenai added inline comments.Mar 9 2023, 1:37 PM
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 added inline comments.Mar 9 2023, 1:45 PM
cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake
3–8

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

phosek updated this revision to Diff 504230.Mar 10 2023, 11:35 AM
phosek marked 2 inline comments as done.
This revision was landed with ongoing or failed builds.Mar 10 2023, 11:36 AM
This revision was automatically updated to reflect the committed changes.
fsb4000 added a subscriber: fsb4000.EditedMar 11 2023, 10:36 PM

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 <phosek@google.com>
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...

phosek reopened this revision.Mar 12 2023, 3:59 PM
This revision is now accepted and ready to land.Mar 12 2023, 3:59 PM
phosek updated this revision to Diff 504483.Mar 12 2023, 3:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 3:59 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Mar 12 2023, 3:59 PM

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 updated this revision to Diff 508371.Mar 25 2023, 8:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2023, 8:17 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

phosek updated this revision to Diff 508440.Mar 26 2023, 1:38 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2023, 11:07 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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

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.