This is an archive of the discontinued LLVM Phabricator instance.

[CMake][runtimes] Redo the --unwindlib=none and -nostdlib++ checks
Needs RevisionPublic

Authored by phosek on Jan 30 2023, 11:18 PM.


Group Reviewers
Restricted Project
Restricted Project

Recent CMake change made the unused argument warning more strict:

After that change, runtimes build started failing. Upon investigation,
it turned out that our --unwindlib=none and -nostdlib++ checks weren't
correct since the flags wouldn't be correct wrapped in
--{start,end}-no-unused-arguments. Furthermore, the
llvm_check_compiler_linker_flag macro wouldn't correctly propagate the

Diff Detail

Event Timeline

phosek created this revision.Jan 30 2023, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 11:18 PM
phosek requested review of this revision.Jan 30 2023, 11:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 30 2023, 11:18 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek edited the summary of this revision. (Show Details)Jan 30 2023, 11:26 PM
phosek added a reviewer: ldionne.
phosek added inline comments.Jan 30 2023, 11:50 PM

@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.

mstorsjo added inline comments.Jan 31 2023, 12:33 AM

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.

mstorsjo added inline comments.Jan 31 2023, 12:49 AM

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.


Does the change from manually backed up flags to cmake_push_check_state have any functional effect here, or is it a pure refactoring?


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)?


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.

mstorsjo added inline comments.Feb 1 2023, 2:41 PM

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,, and the CI build that failed before I added that check:

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...

ldionne requested changes to this revision.Apr 11 2023, 9:19 AM
ldionne added a subscriber: Mordante.

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!


I think you mean Until the minimum CMake version is 3.18, since check_linker_flag was introduced in CMake 3.18.


Let's remove it, then! Perhaps in a separate patch?


Yeah I think I agree, we always want to use -nostdlib++.

This revision now requires changes to proceed.Apr 11 2023, 9:19 AM