Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-09-08 Thread Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL280998: [Driver] Report invalid -mtune/-mcpu parameters when 
-arch=arm64 (authored by vedantk).

Changed prior to commit:
  https://reviews.llvm.org/D23643?vs=68747&id=70761#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D23643

Files:
  cfe/trunk/lib/Driver/Tools.cpp
  cfe/trunk/test/Driver/arm-cortex-cpus.c

Index: cfe/trunk/lib/Driver/Tools.cpp
===
--- cfe/trunk/lib/Driver/Tools.cpp
+++ cfe/trunk/lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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))) {
@@ -1919,13 +1919,15 @@
 
 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:
@@ -2443,18 +2445,18 @@
   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);
Index: cfe/trunk/test/Driver/arm-cortex-cpus.c
===
--- cfe/trunk/test/Driver/arm-cortex-cpus.c
+++ cfe/trunk/test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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


Index: cfe/trunk/lib/Driver/Tools.cpp
===
--- cfe/trunk/lib/Driver/Tools.cpp
+++ cfe/trunk/lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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))) {
@@ -1919,13 +1919,15 @@
 
 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:
@@ -2443,18 +2445,18 @@
   else if ((A = Args.getLastArg(options::OPT_mcpu_EQ)))

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-09-08 Thread Akira Hatanaka via cfe-commits 
ahatanak accepted this revision.
ahatanak added a reviewer: ahatanak.
ahatanak added a comment.
This revision is now accepted and ready to land.

LGTM. We should fix this longstanding bug.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-09-08 Thread Vedant Kumar via cfe-commits 
vsk added a comment.

Ping.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-19 Thread Vedant Kumar via cfe-commits 
vsk updated this revision to Diff 68747.
vsk added a comment.

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


https://reviews.llvm.org/D23643

Files:
  lib/Driver/Tools.cpp
  test/Driver/arm-cortex-cpus.c

Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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))) {
@@ -1919,13 +1919,15 @@
 
 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:
@@ -2443,18 +2445,18 @@
   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);


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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::strin

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-19 Thread Tim Northover via cfe-commits 
t.p.northover added inline comments.


Comment at: test/Driver/arm-cortex-cpus.c:306
@@ -305,2 +305,3 @@
 // RUN: %clang -target arm -mcpu=bogus -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-BOGUS-CPU %s
+// RUN: %clang -arch arm64 -mcpu=bogus -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-BOGUS-CPU %s
 // CHECK-BOGUS-CPU: error: {{.*}} does not support '-mcpu=bogus'

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.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-18 Thread Vedant Kumar via cfe-commits 
vsk updated this revision to Diff 68630.
vsk added a comment.

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


https://reviews.llvm.org/D23643

Files:
  lib/Driver/Tools.cpp
  test/Driver/arm-cortex-cpus.c

Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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 -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 -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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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))) {
@@ -1919,13 +1919,15 @@
 
 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:
@@ -2443,18 +2445,18 @@
   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);


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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 -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 -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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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::

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-18 Thread Vedant Kumar via cfe-commits 
vsk added inline comments.


Comment at: lib/Driver/Tools.cpp:1163
@@ -1162,3 +1162,3 @@
   // FIXME: Should this be picked by checking the target triple instead?
-  if (Args.getLastArg(options::OPT_arch))
+  if ((A = Args.getLastArg(options::OPT_arch)))
 return "cyclone";

ahatanak wrote:
> vsk wrote:
> > vsk wrote:
> > > ahatanak wrote:
> > > > Is there a test case for this change? I don't think this was needed to 
> > > > pass the tests you added?
> > > Good point, I'll work up a test case.
> > 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.
> 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.
All right, IMO it makes more sense to leave out -arch. I'll omit this change 
and update the doxygen for the function.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-18 Thread Akira Hatanaka via cfe-commits 
ahatanak added inline comments.


Comment at: lib/Driver/Tools.cpp:1163
@@ -1162,3 +1162,3 @@
   // FIXME: Should this be picked by checking the target triple instead?
-  if (Args.getLastArg(options::OPT_arch))
+  if ((A = Args.getLastArg(options::OPT_arch)))
 return "cyclone";

vsk wrote:
> vsk wrote:
> > ahatanak wrote:
> > > Is there a test case for this change? I don't think this was needed to 
> > > pass the tests you added?
> > Good point, I'll work up a test case.
> 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.
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.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-18 Thread Vedant Kumar via cfe-commits 
vsk added inline comments.


Comment at: lib/Driver/Tools.cpp:1163
@@ -1162,3 +1162,3 @@
   // FIXME: Should this be picked by checking the target triple instead?
-  if (Args.getLastArg(options::OPT_arch))
+  if ((A = Args.getLastArg(options::OPT_arch)))
 return "cyclone";

vsk wrote:
> ahatanak wrote:
> > Is there a test case for this change? I don't think this was needed to pass 
> > the tests you added?
> Good point, I'll work up a test case.
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.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-18 Thread Vedant Kumar via cfe-commits 
vsk added inline comments.


Comment at: lib/Driver/Tools.cpp:1163
@@ -1162,3 +1162,3 @@
   // FIXME: Should this be picked by checking the target triple instead?
-  if (Args.getLastArg(options::OPT_arch))
+  if ((A = Args.getLastArg(options::OPT_arch)))
 return "cyclone";

ahatanak wrote:
> Is there a test case for this change? I don't think this was needed to pass 
> the tests you added?
Good point, I'll work up a test case.


https://reviews.llvm.org/D23643



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

Re: [PATCH] D23643: [Driver] Report invalid -mtune/-mcpu parameters when -arch=arm64

2016-08-18 Thread Akira Hatanaka via cfe-commits 
ahatanak added a subscriber: ahatanak.
ahatanak added a comment.

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

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



Comment at: lib/Driver/Tools.cpp:1163
@@ -1162,3 +1162,3 @@
   // FIXME: Should this be picked by checking the target triple instead?
-  if (Args.getLastArg(options::OPT_arch))
+  if ((A = Args.getLastArg(options::OPT_arch)))
 return "cyclone";

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


https://reviews.llvm.org/D23643



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

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

2016-08-17 Thread Vedant Kumar via cfe-commits 
vsk created this revision.
vsk added reviewers: t.p.northover, rengolin.
vsk added a subscriber: cfe-commits.
Herald added subscribers: samparker, rengolin, aemerson.

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.

https://reviews.llvm.org/D23643

Files:
  lib/Driver/Tools.cpp
  test/Driver/arm-cortex-cpus.c

Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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 -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 -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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1140,9 +1140,9 @@
 // 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 name if one exists, and
+/// 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))) {
@@ -1160,7 +1160,7 @@
 
   // Make sure we pick "cyclone" if -arch is used.
   // FIXME: Should this be picked by checking the target triple instead?
-  if (Args.getLastArg(options::OPT_arch))
+  if ((A = Args.getLastArg(options::OPT_arch)))
 return "cyclone";
 
   return "generic";
@@ -1919,13 +1919,15 @@
 
 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:
@@ -2443,18 +2445,18 @@
   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);


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -303,7 +303,10 @@
 
 // == 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 -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 -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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp