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

[thakis]

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.

[thakis]

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

[thakis]

Note to self: Check https://github.com/ClangBuiltLinux/linux/issues/598 before landing this.

[nickdesaulniers]

clang/lib/Driver/ToolChains/Clang.cpp

2051

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

[thakis]

clang/lib/Driver/ToolChains/Clang.cpp

2051

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

[nickdesaulniers]

clang/lib/Driver/ToolChains/Clang.cpp

2051

oic

[rnk]

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.

[nathanchance]

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

[thakis]

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]

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]

We're going with D65233 instead.

clang/lib/Driver/ToolChains/Clang.cpp

   Buf = "--target=";
   Buf += Target;
   CDB << ", \"" << escape(Buf) << "\"]},\n";
 }
 
 static void CollectArgsForIntegratedAssembler(Compilation &C,
                                               const ArgList &Args,
                                               ArgStringList &CmdArgs,
-                                              const Driver &D) {
-  if (UseRelaxAll(C, Args))
-    CmdArgs.push_back("-mrelax-all");
-
-  // Only default to -mincremental-linker-compatible if we think we are
-  // targeting the MSVC linker.
-  bool DefaultIncrementalLinkerCompatible =
-      C.getDefaultToolChain().getTriple().isWindowsMSVCEnvironment();
-  if (Args.hasFlag(options::OPT_mincremental_linker_compatible,
-                   options::OPT_mno_incremental_linker_compatible,
-                   DefaultIncrementalLinkerCompatible))
-    CmdArgs.push_back("-mincremental-linker-compatible");
-
-  switch (C.getDefaultToolChain().getArch()) {
-  case llvm::Triple::arm:
-  case llvm::Triple::armeb:
-  case llvm::Triple::thumb:
-  case llvm::Triple::thumbeb:
-    if (Arg *A = Args.getLastArg(options::OPT_mimplicit_it_EQ)) {
-      StringRef Value = A->getValue();
-      if (Value == "always" || Value == "never" || Value == "arm" ||
-          Value == "thumb") {
-        CmdArgs.push_back("-mllvm");
-        CmdArgs.push_back(Args.MakeArgString("-arm-implicit-it=" + Value));
-      } else {
-        D.Diag(diag::err_drv_unsupported_option_argument)
-            << A->getOption().getName() << Value;
+                                              const Driver &D,
+                                              bool IsIntegratedAs) {
+  // Claim flags for the integrated assembler only if it's being used.
+  if (IsIntegratedAs) {
+    if (UseRelaxAll(C, Args))
+      CmdArgs.push_back("-mrelax-all");
+
+    // Only default to -mincremental-linker-compatible if we think we are
+    // targeting the MSVC linker.
+    bool DefaultIncrementalLinkerCompatible =
+        C.getDefaultToolChain().getTriple().isWindowsMSVCEnvironment();
+    if (Args.hasFlag(options::OPT_mincremental_linker_compatible,
+                     options::OPT_mno_incremental_linker_compatible,
+                     DefaultIncrementalLinkerCompatible))
+      CmdArgs.push_back("-mincremental-linker-compatible");
+
+    switch (C.getDefaultToolChain().getArch()) {
+    case llvm::Triple::arm:
+    case llvm::Triple::armeb:
+    case llvm::Triple::thumb:
+    case llvm::Triple::thumbeb:
+      if (Arg *A = Args.getLastArg(options::OPT_mimplicit_it_EQ)) {
+        StringRef Value = A->getValue();
+        if (Value == "always" || Value == "never" || Value == "arm" ||
+            Value == "thumb") {
+          CmdArgs.push_back("-mllvm");
+          CmdArgs.push_back(Args.MakeArgString("-arm-implicit-it=" + Value));
+        } else {
+          D.Diag(diag::err_drv_unsupported_option_argument)
+              << A->getOption().getName() << Value;
+        }
       }
+      break;
+    default:
+      break;
     }
-    break;
-  default:
-    break;
   }
 
   // When passing -I arguments to the assembler we sometimes need to
   // unconditionally take the next argument.  For example, when parsing
   // '-Wa,-I -Wa,foo' we need to accept the -Wa,foo arg after seeing the
   // -Wa,-I arg and when parsing '-Wa,-I,foo' we need to accept the 'foo'
   // arg after parsing the '-I' arg.
   bool TakeNextArg = false;
 
   bool UseRelaxRelocations = C.getDefaultToolChain().useRelaxRelocations();
   bool UseNoExecStack = C.getDefaultToolChain().isNoExecStackDefault();
   const char *MipsTargetFeature = nullptr;
   for (const Arg *A :
        Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
     A->claim();
 
     for (StringRef Value : A->getValues()) {
       if (TakeNextArg) {
         CmdArgs.push_back(Value.data());
         TakeNextArg = false;
         continue;
       }
 
+      // External assemblers have arbitrary flags, don't try to parse them --
+      // claiming the -Wa flags is sufficient.
+      if (!IsIntegratedAs)
+        continue;
+
       if (C.getDefaultToolChain().getTriple().isOSBinFormatCOFF() &&
           Value == "-mbig-obj")
         continue; // LLVM handles bigobj automatically
 
       switch (C.getDefaultToolChain().getArch()) {
       default:
         break;
       case llvm::Triple::thumb:

   // Push all default warning arguments that are specific to
   // the given target.  These come before user provided warning options
   // are provided.
   TC.addClangWarningOptions(CmdArgs);
 
   // Select the appropriate action.
   RewriteKind rewriteKind = RK_None;
 
+  // If CollectArgsForIntegratedAssembler() isn't called below, call it here
+  // with a dummy args list to mark assembler flags as used even when not
+  // running an assembler. Otherwise, clang would emit "argument unused"
+  // warnings for assembler flags when e.g. adding "-E" to flags while debugging
+  // something. That'd be somewhat inconvenient, and it's also inconsistent with
+  // most other flags -- we don't warn on -ffunction-sections not being used
+  // in -E mode either for example, even though it's not really used either.
+  if (!isa<AssembleJobAction>(JA)) {
+    ArgStringList DummyArgs;
+    CollectArgsForIntegratedAssembler(C, Args, DummyArgs, D,
+                                      TC.useIntegratedAs());
+  }
+
   if (isa<AnalyzeJobAction>(JA)) {
     assert(JA.getType() == types::TY_Plist && "Invalid output type.");
     CmdArgs.push_back("-analyze");
   } else if (isa<MigrateJobAction>(JA)) {
     CmdArgs.push_back("-migrate");
   } else if (isa<PreprocessJobAction>(JA)) {
     if (Output.getType() == types::TY_Dependencies)
       CmdArgs.push_back("-Eonly");
     else {
       CmdArgs.push_back("-E");
       if (Args.hasArg(options::OPT_rewrite_objc) &&
           !Args.hasArg(options::OPT_g_Group))
         CmdArgs.push_back("-P");
     }
   } else if (isa<AssembleJobAction>(JA)) {
     CmdArgs.push_back("-emit-obj");
 
-    CollectArgsForIntegratedAssembler(C, Args, CmdArgs, D);
+    assert(TC.useIntegratedAs());
+    CollectArgsForIntegratedAssembler(C, Args, CmdArgs, D,
+                                      /*IsIntegratedAs=*/true);
 
     // Also ignore explicit -force_cpusubtype_ALL option.
     (void)Args.hasArg(options::OPT_force__cpusubtype__ALL);
   } else if (isa<PrecompileJobAction>(JA)) {
     if (JA.getType() == types::TY_Nothing)
       CmdArgs.push_back("-fsyntax-only");
     else if (JA.getType() == types::TY_ModuleFile)
       CmdArgs.push_back(IsHeaderModulePrecompile

 
   // Consume all the warning flags. Usually this would be handled more
   // gracefully by -cc1 (warning about unknown warning flags, etc) but -cc1as
   // doesn't handle that so rather than warning about unused flags that are
   // actually used, we'll lie by omission instead.
   // FIXME: Stop lying and consume only the appropriate driver flags
   Args.ClaimAllArgs(options::OPT_W_Group);
 
-  CollectArgsForIntegratedAssembler(C, Args, CmdArgs,
-                                    getToolChain().getDriver());
+  CollectArgsForIntegratedAssembler(
+      C, Args, CmdArgs, getToolChain().getDriver(), /*IsIntegratedAs=*/true);
 
   Args.AddAllArgs(CmdArgs, options::OPT_mllvm);
 
   assert(Output.isFilename() && "Unexpected lipo output.");
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
   const llvm::Triple &T = getToolChain().getTriple();

clang/test/Driver/as-options.s

 // RUN:   -Ifoo_dir -### 2>&1 \
 // RUN:   | FileCheck %s
 
 // RUN: %clang -target armv7-linux-gnueabihf -c -integrated-as %s \
 // RUN:   -Ifoo_dir -### 2>&1 \
 // RUN:   | FileCheck %s
 
 // CHECK: "-I" "foo_dir"
+
+// Test that assembler options don't cause warnings when there's no assembler
+// stage.
+
+// RUN: %clang -mincremental-linker-compatible -E \
+// RUN:   -o /dev/null -x c++ %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// RUN: %clang -mincremental-linker-compatible -E \
+// RUN:   -o /dev/null -x assembler-with-cpp %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// RUN: %clang -mimplicit-it=always -target armv7-linux-gnueabi -E \
+// RUN:   -o /dev/null -x c++ %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// RUN: %clang -mimplicit-it=always -target armv7-linux-gnueabi -E \
+// RUN:   -o /dev/null -x assembler-with-cpp %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// RUN: %clang -Wa,-mbig-obj -target i386-pc-windows -E \
+// RUN:   -o /dev/null -x c++ %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// RUN: %clang -Wa,-mbig-obj -target i386-pc-windows -E \
+// RUN:   -o /dev/null -x assembler-with-cpp %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// NOWARN-NOT: unused
+
+// Test that unsupported arguments do not cause errors when -fno-integrated-as
+// is set.
+// RUN: %clang -Wa,-mno-warn-deprecated -fno-integrated-as %s -S 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOERROR --allow-empty %s
+// NOERROR-NOT: error: unsupported argument '-mno-warn-deprecated' to option 'Wa,'
+
+// -Wa flags shouldn't cause warnings without an assembler stage with
+// -fno-integrated-as either.
+// RUN: %clang -Wa,-mno-warn-deprecated -fno-integrated-as %s -S 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+
+// But -m flags for the integrated assembler _should_ warn if the integrated
+// assembler is not in use.
+// RUN: %clang -mrelax-all -fintegrated-as %s -S 2>&1 \
+// RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
+// RUN: %clang -mrelax-all -fno-integrated-as %s -S 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
+// WARN: unused