This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Do not error out on spurious C(XX) flags
ClosedPublic

Authored by cryptoad on Feb 5 2019, 8:19 AM.

Details

Summary

The standalone Scudo version is being built with -Werror which can be
tripped by extraneous command line arguments. We have little control over
those as they can be passed down to us by CMAKE_C(XX)_FLAGS, the reported
scenario involving -stdlib=libc++ (see https://reviews.llvm.org/D57412#1384504).

To work around this, disable -Wunused-command-line-argument.

Diff Detail

Event Timeline

cryptoad created this revision.Feb 5 2019, 8:19 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 5 2019, 8:19 AM
Herald added subscribers: Restricted Project, delcypher, mgorny. · View Herald Transcript
eugenis added inline comments.Feb 5 2019, 10:56 AM
lib/scudo/standalone/CMakeLists.txt
8–9

This behavior could (or even should) depend on COMPILER_RT_ENABLE_WERROR.

May be removing -stdlib is better solution since it contradict with -nostdinc++?

cryptoad updated this revision to Diff 185354.Feb 5 2019, 11:05 AM

Remove the default -Werror and use COMPILER_RT_ENABLE_WERROR instead.

@eugenis I changed the Werror logic to gate it behind COMPILER_RT_ENABLE_WERROR
@Eugene.Zelenko : to be fair, flags others than -stdlib could cause a similar issue, so disabling the warning sounds like a more catch-all approach.

eugenis added inline comments.Feb 5 2019, 12:10 PM
lib/scudo/standalone/CMakeLists.txt
14

Did you mean to add -Werror here instead of -Wno-unused-command-line-argument?
COMPILER_RT_HAS_WERROR_FLAG only means that -Werror is supported, not the other one.

cryptoad added inline comments.Feb 5 2019, 12:14 PM
lib/scudo/standalone/CMakeLists.txt
14

It seems to be already added in compiler-rt/CMakeLists.txt:

if(COMPILER_RT_ENABLE_WERROR)
  append_string_if(COMPILER_RT_HAS_WERROR_FLAG -Werror CMAKE_C_FLAGS CMAKE_CXX_FLAGS)

AFAIU the cmake flags will be in the final command line.

Does the problem appear when -stdlib is passed through custom CFLAGS? I think it is common to disable WERROR as well in that case, or add -Wno-unused-command-line-argument to custom CFLAGS.

Does the problem appear when -stdlib is passed through custom CFLAGS? I think it is common to disable WERROR as well in that case, or add -Wno-unused-command-line-argument to custom CFLAGS.

So to summarize, remove Werror and let it be handled by COMPILER_RT_ENABLE_WERROR.
And if someone wants to enable COMPILER_RT_ENABLE_WERROR, then it's up to them to deal with spurious command line flag?

I've re-read the original problem. Since this is not custom cflags, but an LLVM cmake flag, I think we are obligated to handle it.
Removing -stdlib.* from the flags looks like an OK solution.
We could also fix clang to understand that in "-stdlib=libc++ -nodefaultlibs" there are no unused flags, like with the other flag groups that cancel each other.

cryptoad updated this revision to Diff 185603.Feb 6 2019, 11:44 AM

Settle on removing -stdlib=... from the CMake CXX flags.
This prevents a -Werror,-Wunused-command-line-argument error when compiling with
-DLLVM_ENABLE_LIBCXX=ON -DCLANG_DEFAULT_CXX_STDLIB=libc++ -DCOMPILER_RT_ENABLE_WERROR=ON
or the like.

cryptoad marked 3 inline comments as done.Feb 6 2019, 11:45 AM
eugenis accepted this revision.Feb 6 2019, 12:39 PM

Looks good enough to me.

This revision is now accepted and ready to land.Feb 6 2019, 12:39 PM
This revision was automatically updated to reflect the committed changes.