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

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> 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 <https://reviews.llvm.org/rG1d51c699b9e2ebc5bcfdbe85c74cc871426333d4>. 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.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2584
   }
+  if (!ImplicitItCompiler.empty() && !ImplicitItAsm.empty()) {
+    // Set both via compiler flag and asm flag; ok if they match.
----------------
```
if (ImplicitItCompiler.size() && ImplicitItAsm.size()) {
```


================
Comment at: clang/test/Driver/arm-target-as-mimplicit-it.s:39
 // ALWAYS: "-mllvm" "-arm-implicit-it=always"
+// ALWAYS-NOT: "-arm-implicit-it={{.*}}"
 // NEVER: "-mllvm" "-arm-implicit-it=never"
----------------
mstorsjo wrote:
> 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.
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.


================
Comment at: clang/test/Driver/arm-target-as-mimplicit-it.s:44
 // 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"
+// MISMATCH: error: unsupported argument '{{.*}}' to option '-mimplicit-it'
 // INVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Wa,'
----------------
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"
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102812/new/

https://reviews.llvm.org/D102812