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

[Martin Storsjö via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4468e5b89992: [clang] Dont pass multiple backend 
options if mixing -mimplicit-it and -Wa… (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/arm-target-as-mimplicit-it.s

Index: clang/test/Driver/arm-target-as-mimplicit-it.s
===
--- clang/test/Driver/arm-target-as-mimplicit-it.s
+++ clang/test/Driver/arm-target-as-mimplicit-it.s
@@ -10,22 +10,23 @@
 // 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
@@ -34,11 +35,15 @@
 // 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'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 346856.
mstorsjo added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/arm-target-as-mimplicit-it.s

Index: clang/test/Driver/arm-target-as-mimplicit-it.s
===
--- clang/test/Driver/arm-target-as-mimplicit-it.s
+++ clang/test/Driver/arm-target-as-mimplicit-it.s
@@ -10,22 +10,23 @@
 // 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
@@ -34,11 +35,15 @@
 // 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'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2366,15 

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2369
 
-static bool AddARMImplicitITArgs(const ArgList , ArgStringList ,
+static bool CheckARMImplicitITArg(StringRef Value) {
+  return Value == "always" || Value == "never" || Value == "arm" ||

MaskRay wrote:
> Use `functionName` for new functions.
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2369
 
-static bool AddARMImplicitITArgs(const ArgList , ArgStringList ,
+static bool CheckARMImplicitITArg(StringRef Value) {
+  return Value == "always" || Value == "never" || Value == "arm" ||

Use `functionName` for new functions.



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:
> DavidSpickett wrote:
> > 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.
> Thanks, I think that could work to further guard against that case.
The NOT pattern can omit the value to catch more cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.




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.

DavidSpickett wrote:
> ```
> if (ImplicitItCompiler.size() && ImplicitItAsm.size()) {
> ```
Thanks, that'd be even nicer.



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"

DavidSpickett wrote:
> 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.
Thanks, I think that could work to further guard against that case.



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,'

DavidSpickett wrote:
> 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"
> ```
> 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2428
   for (const Arg *A :
Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
 A->claim();

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2371
+  return Value == "always" || Value == "never" || Value == "arm" ||
+ Value == "thumb";
+}

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



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"

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 346568.
mstorsjo added a comment.

Rebased on top of the revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102812

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/arm-target-as-mimplicit-it.s

Index: clang/test/Driver/arm-target-as-mimplicit-it.s
===
--- clang/test/Driver/arm-target-as-mimplicit-it.s
+++ clang/test/Driver/arm-target-as-mimplicit-it.s
@@ -10,22 +10,22 @@
 // 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). If
+/// they match, emit only one argument, if mismatch, error out.
+// 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=MISMATCH
 
 /// Test invalid input.
 // RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=foo %s 2>&1 | FileCheck %s --check-prefix=INVALID
@@ -34,11 +34,13 @@
 // 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 following the matched one.
 // ALWAYS: "-mllvm" "-arm-implicit-it=always"
+// ALWAYS-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"
+// MISMATCH: error: unsupported argument '{{.*}}' to option '-mimplicit-it'
 // INVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Wa,'
 // XINVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Xassembler'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2366,15 +2366,15 @@
   DumpCompilationDatabase(C, "", Target, Output, Input, Args);
 }
 
-static bool AddARMImplicitITArgs(const ArgList , ArgStringList ,
+static bool CheckARMImplicitITArg(StringRef Value) {
+  return Value == "always" || Value == "never" || Value == 

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: MaskRay, nickdesaulniers, raj.khem.
Herald added a subscriber: kristof.beyls.
mstorsjo requested review of this revision.
Herald added a project: clang.

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 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102812

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/arm-target-as-mimplicit-it.s

Index: clang/test/Driver/arm-target-as-mimplicit-it.s
===
--- clang/test/Driver/arm-target-as-mimplicit-it.s
+++ clang/test/Driver/arm-target-as-mimplicit-it.s
@@ -10,22 +10,22 @@
 // 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). If
+/// they match, emit only one argument, if mismatch, error out.
+// 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=MISMATCH
 
 /// Test invalid input.
 // RUN: %clang -target arm-linux-gnueabi -### -Wa,-mimplicit-it=foo %s 2>&1 | FileCheck %s --check-prefix=INVALID
@@ -34,11 +34,13 @@
 // 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 following the matched one.
 // ALWAYS: "-mllvm" "-arm-implicit-it=always"
+// ALWAYS-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"
+// MISMATCH: error: unsupported argument '{{.*}}' to option '-mimplicit-it'
 // INVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Wa,'
 // XINVALID: error: unsupported argument '-mimplicit-it=foo' to option