[clang] Don't pass multiple backend options if mixing -mimplicit-it and -Wa,-mimplicit-it

[mstorsjo]

Summary

If multiple instances of the -arm-implicit-it option is passed to
the backend, it errors out.

Also fix cases where there are multiple -Wa,-mimplicit-it; the existing
tests indicate that the last one specified takes effect, while in
practice it passed double options, which didn't work as intended.

This goes on top of a reverted 2919222d8017f2425a85765b95e4b7c6f8e70ca4.

[mstorsjo]

Rebased on top of the revert.

[mstorsjo]

clang/lib/Driver/ToolChains/Clang.cpp

2371

I guess this could be a StringSwitch<bool> too if you think that's nicer.

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

42

This pattern wouldn't detct if there's e.g. -arm-implicit-it=never -arm-implicit-it=always, but I added test cases that pass -Wa,-mimplicit-it=always twice, where this check should be able to verify that we only output it once.

[mstorsjo]

clang/lib/Driver/ToolChains/Clang.cpp

2411–2429

We could also just add options::OPT_mimplicit_it_EQ into this loop here, and we'd know for certain which value was set last on the command line, so there wouldn't be any ambiguity and we could remove the error case altogether. (The fact that you can have several differing options and the last one takes effect seems to be the intended behaviour from D96285.)

[DavidSpickett]

If multiple instances of the -arm-implicit-it option is passed to
the backend, it errors out.

Does it make sense to fix that side too?

Seems like we have a few options to handle this on clang's side:

1. Last of -mimplicit-it and -Wa,-mimplicit-it wins
2. If they're the same, accept it, if they mismatch, error
3. Take the (last) one that matches the current input type

There is some precedent for number 3 in 1d51c699b9e2ebc5bcfdbe85c74cc871426333d4. Quoting from the commit msg:

Now the same rules will apply to -Wa,-march/-mcpu as would
if you just passed them to the compiler:
* -Wa/-Xassembler options only apply to assembly files.
* Architecture derived from mcpu beats any march options.
* When there are multiple mcpu or multiple march, the last
  one wins.
* If there is a compiler option and an assembler option of
  the same type, we prefer the one that fits the input type.
* If there is an applicable mcpu option but it is overruled
  by an march, the cpu value is still used for the "-target-cpu"
  cc1 option.

I'm not up on all the background to this change so perhaps doing number 3 is going to break existing use cases.

clang/lib/Driver/ToolChains/Clang.cpp

2585

if (ImplicitItCompiler.size() && ImplicitItAsm.size()) {

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

42

Could you do:

// ALWAYS-NOT: "-mllvm" "-arm-implicit-it=never"
// ALWAYS: "-mllvm" "-arm-implicit-it=always"

Not sure if doing the NOT moves the check forward to beyond where the always would be.

47–48

Can you do this check without the regex?

Reading it as is this doesn't look like a very helpful error. I'd expect something like:

error: mismatched values for implicit-it "thumb" and "arm"

[mstorsjo]

In D102812#2770516, @DavidSpickett wrote:

If multiple instances of the -arm-implicit-it option is passed to
the backend, it errors out.

Does it make sense to fix that side too?

I guess it could, although I didn't look into whether it's trivial or tricky. This particular case was at least easy, as all options are handled at once place in one single function.

Seems like we have a few options to handle this on clang's side:

1. Last of -mimplicit-it and -Wa,-mimplicit-it wins
2. If they're the same, accept it, if they mismatch, error
3. Take the (last) one that matches the current input type

There is some precedent for number 3 in 1d51c699b9e2ebc5bcfdbe85c74cc871426333d4. Quoting from the commit msg:

Now the same rules will apply to -Wa,-march/-mcpu as would
if you just passed them to the compiler:
* -Wa/-Xassembler options only apply to assembly files.
* Architecture derived from mcpu beats any march options.
* When there are multiple mcpu or multiple march, the last
  one wins.
* If there is a compiler option and an assembler option of
  the same type, we prefer the one that fits the input type.
* If there is an applicable mcpu option but it is overruled
  by an march, the cpu value is still used for the "-target-cpu"
  cc1 option.

I'm not up on all the background to this change so perhaps doing number 3 is going to break existing use cases.

If case 3 would mean that -mimplicit-it= is ignored when compiling assembly files, that would indeed break existing use cases. If using the input file type to disambiguate cases when there's conflicts, then that's fine, although I'm not sure if there's need for it really.

In this case, the compiler level option is around for historical reasons, and we're aiming to phase out its use within some time frame (although there's much less urgency with it now if we get this resolved more cleanly), so I'm not sure if it's worth to complicate things for it.

Also, if considering the predecent for how the option behaves from GCC/binutils; there, there's only the -Wa level option, and one could plausibly compile a C file with inline assembly that requires adding implicit IT instructions, so there, the -Wa option needs to be respected even when the original source is C.

clang/lib/Driver/ToolChains/Clang.cpp

2585

Thanks, that'd be even nicer.

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

42

Thanks, I think that could work to further guard against that case.

47–48

Yeah this is mostly me being lazy in the first iteration of the patch (I wrote it late last night and wanted to have something to show before finishing) and reusing existing diagnostic messages - it does print unsupported argument 'always and never' to option '-mimplicit-it' in the current form, which is a bit odd indeed.

But I guess if I'd handle both option variants in the same loop, so we can be certain about which one was set last, we could simply define it as last one set wins and we could get rid of the error case altogether.

[DavidSpickett]

If case 3 would mean that -mimplicit-it= is ignored when compiling assembly files, that would indeed break existing use cases. If using the input file type to disambiguate cases when there's conflicts, then that's fine, although I'm not sure if there's need for it really.

Yes, I mis-stated that one. Let me try again.

You have both options: use the one that applies to the input type
You have 2 of one option: choose the last one
You have 2 of each option: choose the last one that applies to the input type

I think that would accommodate the C with inline asm case. I only suggest it because that's what I did in the past, I don't have specific use cases to support it.

That said we've had issues downstream with -mcpu and -march options that conflict, or should conflict, and they don't warn in any way. I would like to avoid another situation like that.

Given that using both options currently crashes clang (therefore no one is relying on this yet) and GCC doesn't have -mimplicit-it I think what you have is actually the best way to go. If the two options agree, fine, if they don't, error. Let's not make it more complicated than it has to be.

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

2369–2374

Use functionName for new functions.

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

42

The NOT pattern can omit the value to catch more cases.

[mstorsjo]

In D102812#2770821, @DavidSpickett wrote:

Given that using both options currently crashes clang (therefore no one is relying on this yet) and GCC doesn't have -mimplicit-it I think what you have is actually the best way to go. If the two options agree, fine, if they don't, error. Let's not make it more complicated than it has to be.

Well it's complicated in the sense that there is an error diagnostic that we need to provide, and all that. Just picking the last set value should be fine too and simplifies the code in that sense.

I'll update the patch with the code in that form, and let you say what you think of that form. If you think we should keep the error though, I'll add it back (and see if I need to add a new diagnostic id for that case, to get a proper error mesasge). It's a bit inconsistent in the sense that one can pass multiple conflicting options via each of the options (where the last one of each wins); gas does handle it in that way too, but two different kinds of options error out instead.

clang/lib/Driver/ToolChains/Clang.cpp

2369–2374

Hmm, the whole rest of the file uses function names starting with a capital, so they do look a bit out of place if adding one new that differs...

[mstorsjo]

Updated to allow mismatches and just picking the value that is set last on the command line - what do you think of this version?

[MaskRay]

Thanks!

[DavidSpickett]

Updated to allow mismatches and just picking the value that is set last on the command line - what do you think of this version?

My initial reaction was that I'd prefer to take the last one that matches the input type, to be consistent with -march and -mcpu. march and mcpu effect both so it makes sense to switch on the input type.
However this is an option that only effects asm/inline asm (even when passed as a compiler option) so you could think of -mimplicit-it as an alias to -Wa,-mimplicit-it. So taking the last value we find sounds good to me.

Ideally we would warn about conflicting values but that's an enhancement that can be done another time if we see situations where it could have been useful.

LGTM

[mstorsjo]

In D102812#2773132, @DavidSpickett wrote:

Updated to allow mismatches and just picking the value that is set last on the command line - what do you think of this version?

My initial reaction was that I'd prefer to take the last one that matches the input type, to be consistent with -march and -mcpu. march and mcpu effect both so it makes sense to switch on the input type.
However this is an option that only effects asm/inline asm (even when passed as a compiler option) so you could think of -mimplicit-it as an alias to -Wa,-mimplicit-it. So taking the last value we find sounds good to me.

Ideally we would warn about conflicting values but that's an enhancement that can be done another time if we see situations where it could have been useful.

Thanks for your input!

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,
+static bool CheckARMImplicitITArg(StringRef Value) {
+  return Value == "always" || Value == "never" || Value == "arm" ||
+         Value == "thumb";
+}
+
+static void 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 (!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".
 
   // 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;
+  StringRef ImplicitIt;
   for (const Arg *A :
-       Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+       Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler,
+                     options::OPT_mimplicit_it_EQ)) {
+    if (A->getOption().getID() == options::OPT_mimplicit_it_EQ) {
+      switch (C.getDefaultToolChain().getArch()) {
+      case llvm::Triple::arm:
+      case llvm::Triple::armeb:
+      case llvm::Triple::thumb:
+      case llvm::Triple::thumbeb:
+        // Only store the value; the last value set takes effect.
+        ImplicitIt = A->getValue();
+        if (!CheckARMImplicitITArg(ImplicitIt))
+          D.Diag(diag::err_drv_unsupported_option_argument)
+              << A->getOption().getName() << ImplicitIt;
+        continue;
+      default:
+        break;
+      }
+    }
+
     A->claim();
 
     for (StringRef Value : A->getValues()) {
       if (TakeNextArg) {
         CmdArgs.push_back(Value.data());
         TakeNextArg = false;
         continue;
       }
 
       if (C.getDefaultToolChain().getTriple().isOSBinFormatCOFF() &&
           Value == "-mbig-obj")
         continue; // LLVM handles bigobj automatically
 
       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))
+        if (Value.startswith("-mimplicit-it=")) {
+          // Only store the value; the last value set takes effect.
+          ImplicitIt = Value.split("=").second;
+          if (CheckARMImplicitITArg(ImplicitIt))
             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:
for (StringRef Value : A->getValues()) {
       } else if (Value == "--version") {
         D.PrintVersion(C, llvm::outs());
       } else {
         D.Diag(diag::err_drv_unsupported_option_argument)
             << A->getOption().getName() << Value;
       }
     }
   }
+  if (ImplicitIt.size())
+    AddARMImplicitITArgs(Args, CmdArgs, ImplicitIt);
   if (UseRelaxRelocations)
     CmdArgs.push_back("--mrelax-relocations");
   if (UseNoExecStack)
     CmdArgs.push_back("-mnoexecstack");
   if (MipsTargetFeature != nullptr) {
     CmdArgs.push_back("-target-feature");
     CmdArgs.push_back(MipsTargetFeature);
   }

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=always -Wa,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
 // 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=always,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
 // 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
+/// Mix -implicit-it= (compiler) with -Wa,-mimplicit-it= (assembler), the
+/// last one set takes priority.
+// RUN: %clang -target arm-linux-gnueabi -### -mimplicit-it=always -Wa,-mimplicit-it=always %S/Inputs/wildcard1.c 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -mimplicit-it=never -Wa,-mimplicit-it=always %S/Inputs/wildcard1.c 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=never -mimplicit-it=always %S/Inputs/wildcard1.c 2>&1 | FileCheck %s --check-prefix=ALWAYS
 
 /// 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
 
 
+/// Check that there isn't a second -arm-implicit-it before or after the one
+/// that was the indended match.
+// ALWAYS-NOT: "-arm-implicit-it={{.*}}"
 // ALWAYS: "-mllvm" "-arm-implicit-it=always"
+// ALWAYS-NOT: "-arm-implicit-it={{.*}}"
+// NEVER-NOT: "-arm-implicit-it={{.*}}"
 // NEVER: "-mllvm" "-arm-implicit-it=never"
+// NEVER-NOT: "-arm-implicit-it={{.*}}"
 // 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'