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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
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
----------------
mstorsjo wrote:
> 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.
Tricky!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124377