[Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

[vsk]

Summary

Fix a crash-on-invalid in which -mtune/-mcpu are set to nonsense values while -arch=arm64.

This patch extends an arm cortex test out of convenience. I'd be happy to move the test if there is a better spot for it.

[ahatanak]

Thanks Vedant, this also fixes the crash that occurs when -mtune=native is provided.

https://reviews.llvm.org/D14471.

lib/Driver/Tools.cpp

1163 ↗

(On Diff #68481)

Is there a test case for this change? I don't think this was needed to pass the tests you added?

[vsk]

lib/Driver/Tools.cpp

1163 ↗

(On Diff #68481)

Good point, I'll work up a test case.

[vsk]

lib/Driver/Tools.cpp

1163 ↗

(On Diff #68481)

Actually, none of the callers of `getAArch64TargetCPU' fail when CPU="cyclone" (afaict).

Do you have a suggestion for how I can test this? I made the change here to make the function's contract more consistent, but can leave it out if you object.

[ahatanak]

lib/Driver/Tools.cpp

1163 ↗

(On Diff #68481)

I couldn't find a test case for this either.

I think we should leave it out if the contract of the function is to get the string passed by either -mtune or -mcpu in "A", but we can leave it in if it's supposed to get one of the strings passed by -mtune, -mcpu, or -arch.

[vsk]

lib/Driver/Tools.cpp

1163 ↗

(On Diff #68481)

All right, IMO it makes more sense to leave out -arch. I'll omit this change and update the doxygen for the function.

[vsk]

Per Akira's suggestion, don't pretend that the Arg* for -arch is a user-specified CPU name (and update the doxygen to reflect this).

[t.p.northover]

test/Driver/arm-cortex-cpus.c

306 ↗

(On Diff #68630)

I think these lines are relying on a Darwin host. -arch isn't a supported option on Linux & elsewhere and Clang will use its default triple. So you probably also need to specify a "-target" to put the driver into Darwin mode.

Either way, please make sure it's tested on a different platform before committing.

[vsk]

• Specify a target per Tim's comments. This now passes 'check-clang' on Ubuntu Xenial.

[vsk]

Ping.

[ahatanak]

LGTM. We should fix this longstanding bug.

cfe/trunk/lib/Driver/Tools.cpp

void Clang::AddARMTargetArgs(const llvm::Triple &Triple, const ArgList &Args,
 
   if (!Args.hasFlag(options::OPT_mimplicit_float,
                     options::OPT_mno_implicit_float, true))
     CmdArgs.push_back("-no-implicit-float");
 }
 // ARM tools end.
 
 /// getAArch64TargetCPU - Get the (LLVM) name of the AArch64 cpu we are
-/// targeting.
-static std::string getAArch64TargetCPU(const ArgList &Args) {
-  Arg *A;
+/// targeting. Set \p A to the Arg corresponding to the -mcpu or -mtune
+/// arguments if they are provided, or to nullptr otherwise.
+static std::string getAArch64TargetCPU(const ArgList &Args, Arg *&A) {
   std::string CPU;
   // If we have -mtune or -mcpu, use that.
   if ((A = Args.getLastArg(options::OPT_mtune_EQ))) {
     CPU = StringRef(A->getValue()).lower();
   } else if ((A = Args.getLastArg(options::OPT_mcpu_EQ))) {
     StringRef Mcpu = A->getValue();
     CPU = Mcpu.split("+").first.lower();
   }
#endif
     return CPU;
   }
 
   return "generic";
 }
 
 static std::string getCPUName(const ArgList &Args, const llvm::Triple &T,
                               bool FromAs = false) {
+  Arg *A;
+
   switch (T.getArch()) {
   default:
     return "";
 
   case llvm::Triple::aarch64:
   case llvm::Triple::aarch64_be:
-    return getAArch64TargetCPU(Args);
+    return getAArch64TargetCPU(Args, A);
 
   case llvm::Triple::arm:
   case llvm::Triple::armeb:
   case llvm::Triple::thumb:
   case llvm::Triple::thumbeb: {
     StringRef MArch, MCPU;
     getARMArchCPUFromArgs(Args, MArch, MCPU, FromAs);
     return arm::getARMTargetCPU(MCPU, MArch, T);
static void getAArch64TargetFeatures(const Driver &D, const ArgList &Args,
   bool success = true;
   // Enable NEON by default.
   Features.push_back("+neon");
   if ((A = Args.getLastArg(options::OPT_march_EQ)))
     success = getAArch64ArchFeaturesFromMarch(D, A->getValue(), Args, Features);
   else if ((A = Args.getLastArg(options::OPT_mcpu_EQ)))
     success = getAArch64ArchFeaturesFromMcpu(D, A->getValue(), Args, Features);
   else if (Args.hasArg(options::OPT_arch))
-    success = getAArch64ArchFeaturesFromMcpu(D, getAArch64TargetCPU(Args), Args,
-                                             Features);
+    success = getAArch64ArchFeaturesFromMcpu(D, getAArch64TargetCPU(Args, A),
+                                             Args, Features);
 
   if (success && (A = Args.getLastArg(options::OPT_mtune_EQ)))
     success =
         getAArch64MicroArchFeaturesFromMtune(D, A->getValue(), Args, Features);
   else if (success && (A = Args.getLastArg(options::OPT_mcpu_EQ)))
     success =
         getAArch64MicroArchFeaturesFromMcpu(D, A->getValue(), Args, Features);
-  else if (Args.hasArg(options::OPT_arch))
-    success = getAArch64MicroArchFeaturesFromMcpu(D, getAArch64TargetCPU(Args),
-                                                  Args, Features);
+  else if (success && Args.hasArg(options::OPT_arch))
+    success = getAArch64MicroArchFeaturesFromMcpu(
+        D, getAArch64TargetCPU(Args, A), Args, Features);
 
   if (!success)
     D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 
   if (Args.getLastArg(options::OPT_mgeneral_regs_only)) {
     Features.push_back("-fp-armv8");
     Features.push_back("-crypto");
     Features.push_back("-neon");

cfe/trunk/test/Driver/arm-cortex-cpus.c

 // CHECK-BOGUS-HF: error: {{.*}} does not support '-march=armbogusv7' 
 // RUN: %clang -target arm -march=armv6bogus -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BOGUS2 %s
 // CHECK-BOGUS2: error: {{.*}} does not support '-march=armv6bogus'
 // RUN: %clang -target arm -march=bogus -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BOGUS3 %s
 // CHECK-BOGUS3: error: {{.*}} does not support '-march=bogus'
 
 // ================== Check that a bogus CPU gives an error
 // RUN: %clang -target arm -mcpu=bogus -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BOGUS-CPU %s
+// RUN: %clang -target armv8-apple-darwin -arch arm64 -mcpu=bogus -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BOGUS-CPU %s
 // CHECK-BOGUS-CPU: error: {{.*}} does not support '-mcpu=bogus'
+// RUN: %clang -target armv8-apple-darwin -arch arm64 -mtune=bogus -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BOGUS-TUNE %s
+// CHECK-BOGUS-TUNE: error: {{.*}} does not support '-mtune=bogus'
 
 // ================== Check default Architecture on each ARM11 CPU
 // RUN: %clang -target arm-linux-gnueabi -mcpu=arm1136j-s -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CPUV6 %s
 // RUN: %clang -target arm-linux-gnueabi -mcpu=arm1136jf-s -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CPUV6 %s
 // CHECK-CPUV6: "-cc1"{{.*}} "-triple" "armv6-{{.*}}
 
 // RUN: %clang -target arm-linux-gnueabi -mcpu=mpcore -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CPUV6K %s
 // RUN: %clang -target arm-linux-gnueabi -mcpu=mpcorenovfp -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CPUV6K %s