This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Only add -GR- option to MSVC or clang-cl builds
ClosedPublic

Authored by DavidSpickett on Jul 6 2023, 1:54 AM.

Details

Summary

Previously we added both -GR- and -fno-rtti if RTTI was disabled.
When building with clang 16.x, that caused this error in part of the build:

clang-16: error: argument unused during compilation: '-G R-' [-Werror,-Wunused-command-line-argument]

I think the strange message is because clang is seeing R- as the argument
to -G, which is a valid clang option.

-GR- is an alternate syntax for the /GR- option for MSVC
(the dash means disable RTTI):
https://learn.microsoft.com/en-us/cpp/build/reference/gr-enable-run-time-type-information?view=msvc-170

This error is sort of fixed by cd18efb61d759405956dbd30e4b5f2720d8e1783
but not intentionally. Also, we'd have to wait for 17.x to benefit from that.

The proper fix here is to only add -GR- if we are building with MSVC
or the MSVC-like clang-cl, and add -fno-rtti if not.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jul 6 2023, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2023, 1:54 AM
DavidSpickett requested review of this revision.Jul 6 2023, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2023, 1:54 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I've confirmed this works with the build proposed in https://reviews.llvm.org/D154246, no need to disable the warning.

simon_tatham accepted this revision.Jul 6 2023, 2:02 AM

Looks good to me – but then I would say that, it was my suggestion :-)

Good catch on finding LIBCXX_TARGETING_CLANG_CL as well as LIBCXX_TARGETING_MSVC. I'd only spotted the latter.

DavidSpickett edited the summary of this revision. (Show Details)Jul 6 2023, 2:06 AM
michaelplatings accepted this revision.Jul 6 2023, 5:26 AM

Thanks for this, LGTM.

ldionne accepted this revision.Jul 6 2023, 5:45 AM
This revision is now accepted and ready to land.Jul 6 2023, 5:45 AM
mstorsjo added inline comments.
libcxx/CMakeLists.txt
572–576

I'm late to the party here, but AFAIK we don't need to use these very libcxx specific cmake defines here - if (MSVC) is just enough as well; the cmake MSVC define is set for MSVC proper or any commandline compatible environment (icl, clang-cl).

DavidSpickett marked an inline comment as done.Jul 7 2023, 2:57 AM
DavidSpickett added inline comments.
DavidSpickett marked an inline comment as done.Jul 7 2023, 2:57 AM