[Driver] Prevent Mips specific code from claiming -mabi argument on other targets.

[craig.topper]

Summary

Fixes PR57976.

[arichardson]

clang/lib/Driver/Driver.cpp

636–637

Would it make more sense to move this into the if? It makes the diff bigger since everything inside has to be reindented, but only querying the flag if target is MIPS seems cleaner to me than the noclaim+a->claim approach.

660–661

Don't we have the same problem here?

[MaskRay]

clang/test/Driver/mabi.c

2

-target is legacy. Use --target=

[craig.topper]

clang/lib/Driver/Driver.cpp

636–637

I wasn't sure if I should expect someone might want to add another target. I'll change it.

[craig.topper]

Address review comments.
I didn't test the -march because all of the target I'm familiar with use -march.

[arichardson]

LGTM

[nickdesaulniers]

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

[aaron.ballman]

In D134671#3824644, @nickdesaulniers wrote:

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

If that's intentional, should we call this out as a potentially breaking change in the release notes and post an announcement?

[MaskRay]

In D134671#3824672, @aaron.ballman wrote:

In D134671#3824644, @nickdesaulniers wrote:

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

If that's intentional, should we call this out as a potentially breaking change in the release notes and post an announcement?

I feel that this is still a minor issue. If we consider this as potentially breaking change, then we'd add many many driver changes as potentially breaking.
I think we should wait for more evidence.

[aaron.ballman]

In D134671#3824868, @MaskRay wrote:

In D134671#3824672, @aaron.ballman wrote:

In D134671#3824644, @nickdesaulniers wrote:

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

If that's intentional, should we call this out as a potentially breaking change in the release notes and post an announcement?

I feel that this is still a minor issue. If we consider this as potentially breaking change, then we'd add many many driver changes as potentially breaking.
I think we should wait for more evidence.

Perfect, thank you for verifying! (That's my intuition as well, but I was on the fence and wondering if my intuition was wrong.)

[craig.topper]

In D134671#3824644, @nickdesaulniers wrote:

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

Is there an expectation that we honor -mabi=ms and something that matches gcc? The original bug report I was fixing was that we silently ignored it.

I appears clang 7 did warn for this. clang 8 stopped warning, maybe that's when the Mips code was added. If it was, I don't think it was intentional.

[nathanchance]

In D134671#3828197, @craig.topper wrote:

In D134671#3824644, @nickdesaulniers wrote:

I don't think it's an issue for us to work around downstream, but this did regress support for -mabi=ms used in UEFI related build scripts.
https://github.com/ClangBuiltLinux/linux/issues/1725
Noting it in case others find their way here via bisection. Thanks to @nathanchance for the report.

Is there an expectation that we honor -mabi=ms and something that matches gcc? The original bug report I was fixing was that we silently ignored it.

No, I don't think so. The kernel was checking -mabi=ms to check for the existence of __attribute__((ms_abi)), which is not as accurate of a check as it could have been, since the kernel has the ability to run small programs during configuration time, which could have just checked for __attribute__((ms_abi)) directly. The kernel does not use -mabi=ms as part of its compiler flags anywhere, it only uses the attribute, so I don't think there is any value to trying to support -mabi=ms unless there is another use case for it. We are just going to clean up the -mabi=ms check and backport it: https://lore.kernel.org/[e-mail]/

I appears clang 7 did warn for this. clang 8 stopped warning, maybe that's when the Mips code was added. If it was, I don't think it was intentional.

Right, I would agree with that assessment.

clang/lib/Driver/Driver.cpp

if (Args.hasFlag(options::OPT_miamcu, options::OPT_mno_iamcu, false)) {
     Target.setEnvironment(llvm::Triple::UnknownEnvironment);
     Target.setEnvironmentName("");
     Target.setOS(llvm::Triple::ELFIAMCU);
     Target.setVendor(llvm::Triple::UnknownVendor);
     Target.setVendorName("intel");
   }
 
   // If target is MIPS adjust the target triple
   // accordingly to provided ABI name.
-  A = Args.getLastArg(options::OPT_mabi_EQ);
-  if (A && Target.isMIPS()) {
+  if (Target.isMIPS()) {
+    if (A = Args.getLastArg(options::OPT_mabi_EQ)) {
       StringRef ABIName = A->getValue();
       if (ABIName == "32") {
         Target = Target.get32BitArchVariant();
         if (Target.getEnvironment() == llvm::Triple::GNUABI64 ||
             Target.getEnvironment() == llvm::Triple::GNUABIN32)
           Target.setEnvironment(llvm::Triple::GNU);
       } else if (ABIName == "n32") {
         Target = Target.get64BitArchVariant();
         if (Target.getEnvironment() == llvm::Triple::GNU ||
             Target.getEnvironment() == llvm::Triple::GNUABI64)
           Target.setEnvironment(llvm::Triple::GNUABIN32);
       } else if (ABIName == "64") {
         Target = Target.get64BitArchVariant();
         if (Target.getEnvironment() == llvm::Triple::GNU ||
             Target.getEnvironment() == llvm::Triple::GNUABIN32)
           Target.setEnvironment(llvm::Triple::GNUABI64);
       }
     }
+  }
 
   // If target is RISC-V adjust the target triple according to
   // provided architecture name
-  A = Args.getLastArg(options::OPT_march_EQ);
-  if (A && Target.isRISCV()) {
+  if (Target.isRISCV()) {
+    if (A = Args.getLastArg(options::OPT_march_EQ)) {
       StringRef ArchName = A->getValue();
       if (ArchName.startswith_insensitive("rv32"))
         Target.setArch(llvm::Triple::riscv32);
       else if (ArchName.startswith_insensitive("rv64"))
         Target.setArch(llvm::Triple::riscv64);
     }
+  }
 
   return Target;
 }
 
 // Parse the LTO options and record the type of LTO compilation
 // based on which -f(no-)?lto(=.*)? or -f(no-)?offload-lto(=.*)?
 // option occurs last.
 static driver::LTOKind parseLTOMode(Driver &D, const llvm::opt::ArgList &Args,

clang/test/Driver/mabi.c

+// RUN: %clang --target=i386-unknown-linux -mabi=ms -S %s -### 2>&1 | FileCheck --check-prefix=CHECK %s
+
+int f() {
+  // CHECK: warning: argument unused during compilation: '-mabi=ms'
+  return 0;
+}