[PATCH] D14471: [AArch64] Fix a crash in driver

[Renato Golin via cfe-commits]

rengolin added subscribers: labrinea, bsmith.
rengolin added a comment.

Hi,

I think it's clear now to me that this strategy isn't going to work. What you need is to add support for AArch64 in the TargetParser and simplify the name matching in the same way we did for ARM. There really is no other meaningful way of doing this.

Please check with Bradley and Alexandros, as they were the ones dealing with this last.

cheers,
--renato


================
Comment at: lib/Driver/Tools.cpp:1026
@@ +1025,3 @@
+/// valid AArch64 cpu.
+static std::pair<std::vector<const char *>, bool>
+getAArch64FeaturesFromCPU(StringRef CPU) {
----------------
You don't need the IsValid flag, since if the returned vector is empty, it has the same semantics.

Also, you're returning the vector by value, whereas the pattern used in this file is to pass a reference to the vector as an argument.

================
Comment at: lib/Driver/Tools.cpp:1042
@@ +1041,3 @@
+
+  return std::make_pair(Features, IsValid);
+}
----------------
You don't need to return a pair. Pass the vector by reference.

You may return a boolean for the IsValid, if there is a case where an empty vector is valid (no default properties). But as it stands, you don't really need to.

================
Comment at: lib/Driver/Tools.cpp:1047
@@ +1046,3 @@
+/// targeting. Return false to indicate the cpu is not a valid AArch64 cpu.
+static std::pair<std::string, bool>
+getAArch64TargetCPU(const ArgList &Args) {
----------------
Avoid using pairs as return type. This is really meant for extremely connected concepts, not to make C++ look like Perl.

================
Comment at: lib/Driver/Tools.cpp:1073
@@ +1072,3 @@
+    IsNative = true;
+    CPU = llvm::sys::getHostCPUName();
+  }
----------------
You could have used a different variable and avoided the ternary operator below.

================
Comment at: lib/Driver/Tools.cpp:1077
@@ +1076,3 @@
+  // Check if CPU is a valid aarch64 cpu.
+  if (getAArch64FeaturesFromCPU(CPU).second)
+    return std::make_pair(CPU, true);
----------------
That's a misuse of this function. You're building a vector, filling it with stuff, copying it back to a temp and throwing it away just for the CPU name detection.

This is clearly a job for the TargetParser.


http://reviews.llvm.org/D14471