driver: Don't warn about assembler flags being unused when not assembling; different approach

[thakis]

Summary

This has mostly the same effect as https://reviews.llvm.org/D65108 but has a more localized implementation that duplicates more code.

This approach also doesn't validate flags for the integrated assembler if the assembler step doesn't run.

[rnk]

lgtm

clang/lib/Driver/ToolChains/Clang.cpp

3564 ↗

(On Diff #211564)

This makes me think perhaps we should diagnose -mimplicit-it= on non-arm targets as an error and then simplify this, but that's a separable change.

[phosek]

We see test failures in as-options.s:

******************** TEST 'Clang :: Driver/as-options.s' FAILED ********************
Script:
--
: 'RUN: at line 3';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target x86_64-linux-gnu -c -no-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -Ifoo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 7';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target x86_64-linux-gnu -c -no-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -I foo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 11';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target x86_64-linux-gnu -c -integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -Ifoo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 15';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target x86_64-linux-gnu -c -integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -I foo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 21';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target aarch64-linux-gnu -c -no-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -Ifoo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 25';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target aarch64-linux-gnu -c -integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -Ifoo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 29';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target armv7-linux-gnueabihf -c -no-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -Ifoo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 33';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -target armv7-linux-gnueabihf -c -integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s    -Ifoo_dir -### 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 42';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -mincremental-linker-compatible -E    -o /dev/null -x c++ /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 45';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -mincremental-linker-compatible -E    -o /dev/null -x assembler-with-cpp /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 48';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -mimplicit-it=always -target armv7-linux-gnueabi -E    -o /dev/null -x c++ /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 51';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -mimplicit-it=always -target armv7-linux-gnueabi -E    -o /dev/null -x assembler-with-cpp /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 54';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -Wa,-mbig-obj -target i386-pc-windows -E    -o /dev/null -x c++ /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 57';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -Wa,-mbig-obj -target i386-pc-windows -E    -o /dev/null -x assembler-with-cpp /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 60';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -Xassembler -mbig-obj -target i386-pc-windows -E    -o /dev/null -x c++ /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 63';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -Xassembler -mbig-obj -target i386-pc-windows -E    -o /dev/null -x assembler-with-cpp /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 70';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -Wa,-mno-warn-deprecated -fno-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s -S 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOERROR --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 76';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -Wa,-mno-warn-deprecated -fno-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s -S 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 81';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -mrelax-all -fintegrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s -S 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=NOWARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
: 'RUN: at line 83';   /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/clang -mrelax-all -fno-integrated-as /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s -S 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangrtoFQg/llvm_build_dir/bin/FileCheck --check-prefix=WARN --allow-empty /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s:66:16: error: NOWARN-NOT: excluded string found in input
// NOWARN-NOT: unused
               ^
<stdin>:1:95: note: found here
clang-10: warning: /b/s/w/ir/k/llvm-project/clang/test/Driver/as-options.s: 'assembler' input unused [-Wunused-command-line-argument]
                                                                                              ^~~~~~

---

[thakis]

phosek: In what config / on what bot?

[thakis]

phosek: Does r367176 help?

[phosek]

In D65233#1603653, @thakis wrote:

phosek: In what config / on what bot?

It's our toolchain linux-x86 builder, see https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-x64-linux/b8906751725919481408, mac-x86 has been working fine.

In D65233#1603654, @thakis wrote:

phosek: Does r367176 help?

No, it's still failing with the same error, see https://luci-milo.appspot.com/p/fuchsia/g/clang/console.

[bjope]

I made another fixup (similar to https://reviews.llvm.org/rL367176) here https://reviews.llvm.org/rL367182.

Can someone please take a look (a post-commit review) of https://reviews.llvm.org/rL367182 to verify that I did not misunderstand the intention with the test somehow?

[phosek]

In D65233#1603741, @bjope wrote:

I made another fixup (similar to https://reviews.llvm.org/rL367176) here https://reviews.llvm.org/rL367182.

Can someone please take a look (a post-commit review) of https://reviews.llvm.org/rL367182 to verify that I did not misunderstand the intention with the test somehow?

That seemed to have helped, thanks!

[thakis]

In D65233#1603741, @bjope wrote:

I made another fixup (similar to https://reviews.llvm.org/rL367176) here https://reviews.llvm.org/rL367182.

Can someone please take a look (a post-commit review) of https://reviews.llvm.org/rL367182 to verify that I did not misunderstand the intention with the test somehow?

Looks good. Thanks much for the fix!

[bjope]

In D65233#1603783, @thakis wrote:

In D65233#1603741, @bjope wrote:

I made another fixup (similar to https://reviews.llvm.org/rL367176) here https://reviews.llvm.org/rL367182.

Can someone please take a look (a post-commit review) of https://reviews.llvm.org/rL367182 to verify that I did not misunderstand the intention with the test somehow?

Looks good. Thanks much for the fix!

Nice to know that I did not mess up the test in my attempt to silence the buildbots. So thanks for taking a look.

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

if (Arg *A = Args.getLastArg(options::OPT_mimplicit_it_EQ)) {
             << A->getOption().getName() << Value;
       }
     }
     break;
   default:
     break;
   }
 
+  // If you add more args here, also add them to the block below that
+  // starts with "// If CollectArgsForIntegratedAssembler() isn't called below".
+
   // 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();
void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   // 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, claim the args
+  // it claims 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)) {
+    // The args claimed here should match the args used in
+    // CollectArgsForIntegratedAssembler().
+    if (TC.useIntegratedAs()) {
+      Args.ClaimAllArgs(options::OPT_mrelax_all);
+      Args.ClaimAllArgs(options::OPT_mno_relax_all);
+      Args.ClaimAllArgs(options::OPT_mincremental_linker_compatible);
+      Args.ClaimAllArgs(options::OPT_mno_incremental_linker_compatible);
+      switch (C.getDefaultToolChain().getArch()) {
+      case llvm::Triple::arm:
+      case llvm::Triple::armeb:
+      case llvm::Triple::thumb:
+      case llvm::Triple::thumbeb:
+        Args.ClaimAllArgs(options::OPT_mimplicit_it_EQ);
+      default:
+        break;
+      }
+    }
+    Args.ClaimAllArgs(options::OPT_Wa_COMMA);
+    Args.ClaimAllArgs(options::OPT_Xassembler);
+  }
+
   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");

cfe/trunk/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
+// RUN: %clang -Xassembler -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 -Xassembler -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