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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: Mordante.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: ekilmer.

Let's try to rebase this onto `main` and push this forward, since this is blocking @Mordante 's work on modules. Thanks for looking into this, though!



================
Comment at: cmake/Modules/LLVMCheckCompilerLinkerFlag.cmake:8
+else()
+  # Until the minimum CMAKE version is 3.19
 
----------------
I think you mean `Until the minimum CMake version is 3.18`, since `check_linker_flag` was introduced in CMake 3.18.


================
Comment at: runtimes/CMakeLists.txt:112-113
   # --unwindlib=none is supported, and use that if possible.
   # Don't add this if not necessary to fix linking, as it can break using
   # e.g. ASAN/TSAN.
   llvm_check_compiler_linker_flag(C "--unwindlib=none" CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG)
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > phosek wrote:
> > > @mstorsjo do you know what's the reasoning behind this? I was wondering if we could add `--unwindlib=none` and `-nostdlib++` unconditionally to simplify this logic and make the behavior more deterministic.
> > Sorry, I don't remember all the details exactly - but I think the issue was exposed in the libc++ CI builds, for the ASAN/TSAN builds there. I guess the easiest would be to put up a test patch that adds it unconditionally, and see what issues we hit in CI.
> I tried running a patch through CI, removing the `if (NOT LLVM_RUNTIMES_LINKING_WORKS)` condition from around adding `--unwindlib=none`, and it no longer broke anything. So that's a bit unsettling...
> 
> I dug up the original case where I added this, https://reviews.llvm.org/D113253#3112444, and the CI build that failed before I added that check:
> https://buildkite.com/llvm-project/libcxx-ci/builds/6433
> 
> There, essentially all cmake tests failed after adding the option, and this then broke the cmake configure in libunwind (otherwise it would probably have been a severely broken build anyway).
> 
> I'm not quite sure what has changed since, that makes it no longer break though. I guess it would be possible to bisect somehow...
Let's remove it, then! Perhaps in a separate patch?


================
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.
----------------
mstorsjo wrote:
> 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)?
Yeah I think I agree, we always want to use `-nostdlib++`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142957