[libcxx-commits] [PATCH] D124377: [runtimes] [CMake] Fix checks for -Werror when building with incomplete toolchains

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: runtimes/CMakeLists.txt:106-124
     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
----------------
ldionne wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124377