[libcxx-commits] [PATCH] D142957: [CMake][runtimes] Redo the --unwindlib=none and -nostdlib++ checks

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake:16
+    cmake_policy(PUSH)
+    cmake_policy(SET CMP0056 NEW)
+    cmake_push_check_state()
----------------
Is there any functional change to the changes to the helper macro/function here, or is this just a pure refactoring? I'd kinda prefer this change (which does seem reasonable) in a separate commit if possible, with separate explanation/motivation for it.


================
Comment at: runtimes/CMakeLists.txt:116
   if (CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG)
-    set(ORIG_CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")
-    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --unwindlib=none")
+    cmake_push_check_state()
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments --unwindlib=none --end-no-unused-arguments")
----------------
Does the change from manually backed up flags to `cmake_push_check_state` have any functional effect here, or is it a pure refactoring?


================
Comment at: runtimes/CMakeLists.txt:143
+check_cxx_compiler_flag("" CXX_LINKING_WORKS)
+if (NOT CXX_LINKING_WORKS)
+  # Disable use of the installed C++ standard library when building runtimes.
----------------
This doesn't seem right to me, and/or changes the behaviour from before? Even if C++ linking does work out of the box (i.e. we have a complete environment with a preexisting standard C++ library), we'd still want to use `-nostdlib++` (we did before at least)?


================
Comment at: runtimes/CMakeLists.txt:151
+    cmake_push_check_state()
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --start-no-unused-arguments -nostdlib++ --end-no-unused-arguments")
+    check_cxx_compiler_flag("" CXX_SUPPORTS_START_NO_UNUSED_ARGUMENTS)
----------------
Does this break things for GCC cases? We're conflating the checks for `--start-no-unused-arguments` and `--unwindlib=none`, `-nostdlib++` and `-nostdinc++`. For `--unwindlib=none`, that's probably fine, since AFAIK Clang is the only one supporting both (and if we don't support Clang older than a couple versions, both options should be supported equally). For GCC, I think some of `-nostdlib++` or `-nostdinc++` might be supported (I remember one of them being discussed of being added quite recently).

So for that case, I think it might be better if we'd keep it so that we check for support for the `--start-no-unused-arguments --end-no-unused-arguments` option first, and then use that to either wrap or not wrap the other options when they are tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142957