[clang][Arm] Fix handling of -Wa,-implicit-it=

[nickdesaulniers]

Summary

Similiar to D95872, this flag can be set for the assembler directly.
Move validation code into a reusable helper function.

Link: https://bugs.llvm.org/show_bug.cgi?id=49023
Link: https://github.com/ClangBuiltLinux/linux/issues/1270
Reported-by: Arnd Bergmann <[e-mail]>
Signed-off-by: Nick Desaulniers <[e-mail]>

[DavidSpickett]

clang/lib/Driver/ToolChains/Clang.cpp

2307

I would rename this AddARMImplicitITArgs to match AddARMTargetArgs.

2386

You could merge the two ifs with &&

clang/test/Driver/arm-target-as-mimplicit-it.s

14

For these multiple option tests, we're checking that the last -mimplicit-it wins, so I think the first argument should be another different value. E.g.
-Wa,-mimplicit-it=thumb -Wa,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS

Then if we got our logic wrong, we'll see "=thumb" in the result and fail.
(afaik -mthumb doesn't set implicit-it to anything)

27

Does it make sense to check which wins of -mimplicit-it and -Wa,-mimplicit-it for assembler and c inputs?

I guess right now we would have the first for C files, and both for assembler files but the last -mllvm implicit-it wins, and it'd be the assembler's option. Either way some tests to confirm that would be good.

You can use any old C file, my change that you reviewed does the same thing to check this. (but in that case, since it was earlier it could tell whether we had both kinds of option which I don't think can be done here)

[nickdesaulniers]

• address code review comments, add moar tests

[DavidSpickett]

clang/test/Driver/arm-target-as-mimplicit-it.s

27

I'm confused why this generates two -arm-implicit-it.

I'd expect that:
A c file only uses the compiler's argument
An assembler file uses the compiler and assembler argument, with the assembler argument last

Are we calling CollectArgsForIntegratedAssembler even for a C file?

44

I'd put these last two before the unsupported argument lines, to match the test order.

[nickdesaulniers]

• reorder test checks

clang/test/Driver/arm-target-as-mimplicit-it.s

27

I don't think this is an issue.

I'm confused why this generates two -arm-implicit-it.

If you check the implementation of CollectArgsForIntegratedAssembler, it will call my added helper (AddARMImplicitITArgs) for -mimplicit-it=, then check for -Wa,-implicit-it=. If both are present, then both will be generated for the assembler, with the latter taking preference.

Are we calling CollectArgsForIntegratedAssembler even for a C file?

Yes. Unless -no-integrated-as or -S was used, we need to be able to modify how assembler is handled both in generating it from C code, but also for inline assembly that may be in a C source.

[DavidSpickett]

LGTM

clang/test/Driver/arm-target-as-mimplicit-it.s

27

Right, I forgot about the inline assembly scenario.

[nickdesaulniers]

Thanks for the review (here and D96304)!

clang/lib/Driver/ToolChains/Clang.cpp

if (Err) {
     Driver.Diag(diag::err_drv_compilationdatabase) << Path << Err.message();
     return;
   }
   CompilationDatabase =
       std::make_unique<llvm::raw_fd_ostream>(FD, /*shouldClose=*/true);
   DumpCompilationDatabase(C, "", Target, Output, Input, Args);
 }
 
+static bool AddARMImplicitITArgs(const ArgList &Args, ArgStringList &CmdArgs,
+                                 StringRef Value) {
+  if (Value == "always" || Value == "never" || Value == "arm" ||
+      Value == "thumb") {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-arm-implicit-it=" + Value));
+    return true;
+  }
+  return false;
+}
+
 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 {
+      if (!AddARMImplicitITArgs(Args, CmdArgs, Value))
         D.Diag(diag::err_drv_unsupported_option_argument)
             << 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".
 
for (StringRef Value : A->getValues()) {
 
       switch (C.getDefaultToolChain().getArch()) {
       default:
         break;
       case llvm::Triple::thumb:
       case llvm::Triple::thumbeb:
       case llvm::Triple::arm:
       case llvm::Triple::armeb:
+        if (Value.startswith("-mimplicit-it=") &&
+            AddARMImplicitITArgs(Args, CmdArgs, Value.split("=").second))
+          continue;
         if (Value == "-mthumb")
           // -mthumb has already been processed in ComputeLLVMTriple()
           // recognize but skip over here.
           continue;
         break;
       case llvm::Triple::mips:
       case llvm::Triple::mipsel:
       case llvm::Triple::mips64:

clang/test/Driver/arm-target-as-mimplicit-it.s

+/// Simple tests for valid input.
+/// -Wa,-implicit-it=
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=never %s 2>&1 | FileCheck %s --check-prefix=NEVER
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=arm %s 2>&1 | FileCheck %s --check-prefix=ARM
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=thumb %s 2>&1 | FileCheck %s --check-prefix=THUMB
+/// -Xassembler -mimplicit-it=
+// RUN: %clang -target arm-linux-gnueabi -### -Xassembler -mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -Xassembler -mimplicit-it=never %s 2>&1 | FileCheck %s --check-prefix=NEVER
+// RUN: %clang -target arm-linux-gnueabi -### -Xassembler -mimplicit-it=arm %s 2>&1 | FileCheck %s --check-prefix=ARM
+// RUN: %clang -target arm-linux-gnueabi -### -Xassembler -mimplicit-it=thumb %s 2>&1 | FileCheck %s --check-prefix=THUMB
+/// Test space separated -Wa,- arguments (latter wins).
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=never -Wa,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always -Wa,-mimplicit-it=never %s 2>&1 | FileCheck %s --check-prefix=NEVER
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always -Wa,-mimplicit-it=arm %s 2>&1 | FileCheck %s --check-prefix=ARM
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always -Wa,-mimplicit-it=thumb %s 2>&1 | FileCheck %s --check-prefix=THUMB
+/// Test comma separated -Wa,- arguments (latter wins).
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=never,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always,-mimplicit-it=never %s 2>&1 | FileCheck %s --check-prefix=NEVER
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always,-mimplicit-it=arm %s 2>&1 | FileCheck %s --check-prefix=ARM
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always,-mimplicit-it=thumb %s 2>&1 | FileCheck %s --check-prefix=THUMB
+
+/// Mix -implicit-it= (compiler) with -Wa,-mimplicit-it= (assembler), assembler
+/// takes priority. -mllvm -arm-implicit-it= will be repeated, with the
+/// assembler flag appearing last (latter wins).
+// RUN: %clang -target arm-linux-gnueabi -### -mimplicit-it=never -Wa,-mimplicit-it=always %S/Inputs/wildcard1.c 2>&1 | FileCheck %s --check-prefix=NEVER_ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -mimplicit-it=always -Wa,-mimplicit-it=never %S/Inputs/wildcard1.c 2>&1 | FileCheck %s --check-prefix=ALWAYS_NEVER
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=never -mimplicit-it=always %S/Inputs/wildcard1.c 2>&1 | FileCheck %s --check-prefix=ALWAYS_NEVER
+
+/// Test invalid input.
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=foo %s 2>&1 | FileCheck %s --check-prefix=INVALID
+// RUN: %clang -target arm-linux-gnueabi -### -Xassembler -mimplicit-it=foo %s 2>&1 | FileCheck %s --check-prefix=XINVALID
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always -Wa,-mimplicit-it=foo %s 2>&1 | FileCheck %s --check-prefix=INVALID
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=always,-mimplicit-it=foo %s 2>&1 | FileCheck %s --check-prefix=INVALID
+
+
+// ALWAYS: "-mllvm" "-arm-implicit-it=always"
+// NEVER: "-mllvm" "-arm-implicit-it=never"
+// ARM: "-mllvm" "-arm-implicit-it=arm"
+// THUMB: "-mllvm" "-arm-implicit-it=thumb"
+// NEVER_ALWAYS: "-mllvm" "-arm-implicit-it=never" "-mllvm" "-arm-implicit-it=always"
+// ALWAYS_NEVER: "-mllvm" "-arm-implicit-it=always" "-mllvm" "-arm-implicit-it=never"
+// INVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Wa,'
+// XINVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Xassembler'