[libcxx-commits] [PATCH] D145716: [CMake] Unify llvm_check_linker_flag and llvm_check_compiler_linker_flag

[Petr Hosek via Phabricator via libcxx-commits]

phosek added a comment.

In D145716#4188311 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145716/new/

https://reviews.llvm.org/D145716