Page MenuHomePhabricator

Reland "driver: Don't warn about assembler flags being unused when not assembling"
AbandonedPublic

Authored by thakis on Jul 22 2019, 12:36 PM.

Details

Reviewers
rnk
Summary

This relands r365703 (and r365714), originally reviewed at
https://reviews.llvm.org/D64527.

The problem with the old approach was that clang would now warn about
-Wa flags that the integrated assembler didn't understand even when
-fno-integrated-as was used.

Just checking for that before calling CollectArgsForIntegratedAssembler
still makes clang warn about unused flags with -fno-integrated-as, which
isn't desired either.

Instead, omit just the "unknown flag" diag if the integrated assembler
isn't in use.

One of the new test cases is by nickdesaulniers from
https://reviews.llvm.org/D64655.

Diff Detail

Event Timeline

thakis created this revision.Jul 22 2019, 12:36 PM
thakis marked an inline comment as done.
thakis added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2051

(This just wraps all the red lines on the lhs in an if (IsIntegratedAs) and leaves them all otherwise unchanged.)

clang/lib/Driver/ToolChains/Clang.cpp
2051

Return early on negated condition instead of adding additional indentation and messing with git blame.

thakis marked an inline comment as done.Jul 22 2019, 12:51 PM
thakis added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2051

It doesn't skip the whole function, just the first few flags.

clang/lib/Driver/ToolChains/Clang.cpp
2051

oic

rnk added inline comments.Jul 22 2019, 1:04 PM
clang/lib/Driver/ToolChains/Clang.cpp
3564

I think it would be better to use ClaimAllArgs here. If you look around in this code, there's a fair amount of stuff like this:

if (KernelOrKext) {
  // -mkernel and -fapple-kext imply no exceptions, so claim exception related
  // arguments now to avoid warnings about unused arguments.
  Args.ClaimAllArgs(options::OPT_fexceptions);
  Args.ClaimAllArgs(options::OPT_fno_exceptions);
  Args.ClaimAllArgs(options::OPT_fobjc_exceptions);
  Args.ClaimAllArgs(options::OPT_fno_objc_exceptions);
  Args.ClaimAllArgs(options::OPT_fcxx_exceptions);
  Args.ClaimAllArgs(options::OPT_fno_cxx_exceptions);
  return;
}

It repeats the knowledge of which flags are passed to the assembler (looks like 5), but it is consistent with what's already done.

I tested this patch on top of r366728 and saw no regressions in my set of kernel builds.

thakis marked an inline comment as done.Jul 22 2019, 4:57 PM
thakis added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3564

I think that's worse for this use case here: In addition of having to duplicate the flags, the flag parsing error output (for integrated as) is then again dependent on if we actually run the assembler, which is the same kind of inconsistency that this change is fixing.

thakis marked an inline comment as done.Jul 24 2019, 11:14 AM
thakis added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3564

I uploaded a patch for the approach you suggested to https://reviews.llvm.org/D65233 . It's ~20 (non-whitespace) lines of code, while this patch is ~10 lines (it looks like more because phab doesn't highlight whitespace-only changes well.)

I like this patch more, but let me know which one you'd like me to land.

thakis abandoned this revision.Jul 26 2019, 5:56 PM

We're going with D65233 instead.