[clang][PPC] Checking Unknown Values Passed to -mcpu

[qiongsiwu1]

Summary

Currently ppc::getPPCTargetCPU returns an empty string when it encounters an unknown value passed to -mcpu. This causes clang to ignore unknown -mcpu values silently.

This patch changes the behaviour of ppc::getPPCTargetCPU so that it passes the unknown option to the target info, so the target info can actually check if the CPU string is supported, and report an error when encountering unknown/unsupported CPU string.

[amyk]

It might be good to add a test case to illustrate the 'unknown target CPU' error that is issued as a result of this patch.

[qiongsiwu1]

Update to include a new test case against the unknown target error message. Thanks for the suggestion @amyk !

[jamieschmeiser]

clang/lib/Driver/ToolChains/Arch/PPC.cpp

54

This seems strange. If the option is "generic", it calls getPPCGenericTargetCPU(), but if it is "common", it returns "generic." I think you may want to also call getPPCGenericTargetCPU() here. There should probably also be an assume where this returns that it didn't return "generic" if that is the intended result. Also, there should also be tests for what happens when "generic" and "common" are specified.

77

Why did you change the type from const char *? Couldn't you use CPUName->data() in the default instead? With your change, I think it may need to create StringRefs around all of the choices and then just get the string from them.

[qiongsiwu1]

clang/lib/Driver/ToolChains/Arch/PPC.cpp

54

I agree this generic vs common behaviour is strange. They way this patch handles generic vs common keeps the existing behaviour. In short, before this patch, clang already treats -mcpu=generic and -mcpu=common differently.

When -mcpu=generic is specified, we do some processing depending on the OS and architecture (effectively calling getPPCGenericTargetCPU). This happens because generic is not one of the cases of the big StringSwitch, and we return an empty string from ppc::getPPCTargetCPU.

On the other hand, when -mcpu=common is specified, the StringSwitch maps common to generic, and we simply returns generic as the target CPU name here.

Is this behaviour what we expect? If it is not, I will add a bug fix (with this patch or with a different patch if a separate one is easier to review). We only have an existing test case for generic, but not for common. I will add one with the bug fix.

77

I was worried about the comment on top of StringRef::data() which says "Get a pointer to the start of the string (which may not be null terminated).". I was not sure what would happen if the data inside CPUName was not null terminated (it might be fine) so I thought it would be safer to use the StringRefs directly instead.

[jamieschmeiser]

clang/lib/Driver/ToolChains/Arch/PPC.cpp

54

Yeah, I saw that you weren't changing the behaviour; I just suspect that it was an existing bug...Not sure who to check with about that @nemanjai? @hubert.reinterpretcast?

77

Probably the same sort of thing that would happen with creating a StringRef since that calls strlen to set the size of the string it is wrapping :-)

[qiongsiwu1]

Updating to revert back to const char * in the big StringSwitch.

[qiongsiwu1]

clang/lib/Driver/ToolChains/Arch/PPC.cpp

77

Got it! Thanks for the suggestion/clarification! The patch is updated to use const char *. I looked around and I do see StringRef::data() used quite liberally here and there (e.g. here). So I agree in this case we should be fine.

[jamieschmeiser]

I'm fine with handling the return for common as a separate change, if necessary.
Is the error produced now because it passes back the incorrect option rather than quietly changing it to something appropriate as it did before?

[qiongsiwu1]

In D139720#3989651, @jamieschmeiser wrote:

I'm fine with handling the return for common as a separate change, if necessary.

Sounds good! In this case I will fix it in a different patch once we decide what exactly to do for the -mcpu=common case.

Is the error produced now because it passes back the incorrect option rather than quietly changing it to something appropriate as it did before?

Yes precisely. When a TargetInfo is created, we have this code that checks the validity of the CPU string. Currently, we silently change anything weird to something reasonable and use it to create the TargetInfo. This patch removes that filtering and passes on the incorrect CPU string so we can catch the error.

[jamieschmeiser]

LGTM

Thanks.

clang/lib/Driver/ToolChains/Arch/PPC.h

 
 enum class ReadGOTPtrMode {
   Bss,
   SecurePlt,
 };
 
 FloatABI getPPCFloatABI(const Driver &D, const llvm::opt::ArgList &Args);
 
-std::string getPPCTargetCPU(const llvm::opt::ArgList &Args);
+std::string getPPCTargetCPU(const llvm::opt::ArgList &Args,
+                            const llvm::Triple &T);
 const char *getPPCAsmModeForCPU(StringRef Name);
 ReadGOTPtrMode getPPCReadGOTPtrMode(const Driver &D, const llvm::Triple &Triple,
                                     const llvm::opt::ArgList &Args);
 
 void getPPCTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                           const llvm::opt::ArgList &Args,
                           std::vector<llvm::StringRef> &Features);
 
 } // end namespace ppc
 } // end namespace target
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_PPC_H

clang/lib/Driver/ToolChains/Arch/PPC.cpp

 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Host.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
+static std::string getPPCGenericTargetCPU(const llvm::Triple &T) {
+  // LLVM may default to generating code for the native CPU,
+  // but, like gcc, we default to a more generic option for
+  // each architecture. (except on AIX)
+  if (T.isOSAIX())
+    return "pwr7";
+  else if (T.getArch() == llvm::Triple::ppc64le)
+    return "ppc64le";
+  else if (T.getArch() == llvm::Triple::ppc64)
+    return "ppc64";
+  else
+    return "ppc";
+}
+
 /// getPPCTargetCPU - Get the (LLVM) name of the PowerPC cpu we are targeting.
-std::string ppc::getPPCTargetCPU(const ArgList &Args) {
+std::string ppc::getPPCTargetCPU(const ArgList &Args, const llvm::Triple &T) {
   if (Arg *A = Args.getLastArg(clang::driver::options::OPT_mcpu_EQ)) {
     StringRef CPUName = A->getValue();
 
+    if (CPUName == "generic")
+      return getPPCGenericTargetCPU(T);
+
     if (CPUName == "native") {
       std::string CPU = std::string(llvm::sys::getHostCPUName());
       if (!CPU.empty() && CPU != "generic")
         return CPU;
       else
-        return "";
+        return getPPCGenericTargetCPU(T);
     }
 
-    return llvm::StringSwitch<const char *>(CPUName)
+    auto TargetCPUName = llvm::StringSwitch<llvm::StringRef>(CPUName)
                              .Case("common", "generic")
-        .Case("440", "440")
                              .Case("440fp", "440")
-        .Case("450", "450")
-        .Case("601", "601")
-        .Case("602", "602")
-        .Case("603", "603")
-        .Case("603e", "603e")
-        .Case("603ev", "603ev")
-        .Case("604", "604")
-        .Case("604e", "604e")
-        .Case("620", "620")
                              .Case("630", "pwr3")
                              .Case("G3", "g3")
-        .Case("7400", "7400")
                              .Case("G4", "g4")
-        .Case("7450", "7450")
                              .Case("G4+", "g4+")
-        .Case("750", "750")
                              .Case("8548", "e500")
-        .Case("970", "970")
                              .Case("G5", "g5")
-        .Case("a2", "a2")
-        .Case("e500", "e500")
-        .Case("e500mc", "e500mc")
-        .Case("e5500", "e5500")
                              .Case("power3", "pwr3")
                              .Case("power4", "pwr4")
                              .Case("power5", "pwr5")
                              .Case("power5x", "pwr5x")
                              .Case("power6", "pwr6")
                              .Case("power6x", "pwr6x")
                              .Case("power7", "pwr7")
                              .Case("power8", "pwr8")
                              .Case("power9", "pwr9")
                              .Case("power10", "pwr10")
                              .Case("future", "future")
-        .Case("pwr3", "pwr3")
-        .Case("pwr4", "pwr4")
-        .Case("pwr5", "pwr5")
-        .Case("pwr5x", "pwr5x")
-        .Case("pwr6", "pwr6")
-        .Case("pwr6x", "pwr6x")
-        .Case("pwr7", "pwr7")
-        .Case("pwr8", "pwr8")
-        .Case("pwr9", "pwr9")
-        .Case("pwr10", "pwr10")
                              .Case("powerpc", "ppc")
                              .Case("powerpc64", "ppc64")
                              .Case("powerpc64le", "ppc64le")
-        .Default("");
+                             .Default(CPUName);
+    return TargetCPUName.str();
   }
 
-  return "";
+  return getPPCGenericTargetCPU(T);
 }
 
 const char *ppc::getPPCAsmModeForCPU(StringRef Name) {
   return llvm::StringSwitch<const char *>(Name)
       .Case("pwr7", "-mpower7")
       .Case("power7", "-mpower7")
       .Case("pwr8", "-mpower8")
       .Case("power8", "-mpower8")

clang/lib/Driver/ToolChains/CommonArgs.cpp

std::string tools::getCPUName(const Driver &D, const ArgList &Args,
   case llvm::Triple::nvptx64:
     if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
       return A->getValue();
     return "";
 
   case llvm::Triple::ppc:
   case llvm::Triple::ppcle:
   case llvm::Triple::ppc64:
-  case llvm::Triple::ppc64le: {
-    std::string TargetCPUName = ppc::getPPCTargetCPU(Args);
-    // LLVM may default to generating code for the native CPU,
-    // but, like gcc, we default to a more generic option for
-    // each architecture. (except on AIX)
-    if (!TargetCPUName.empty())
-      return TargetCPUName;
-
-    if (T.isOSAIX())
-      TargetCPUName = "pwr7";
-    else if (T.getArch() == llvm::Triple::ppc64le)
-      TargetCPUName = "ppc64le";
-    else if (T.getArch() == llvm::Triple::ppc64)
-      TargetCPUName = "ppc64";
-    else
-      TargetCPUName = "ppc";
+  case llvm::Triple::ppc64le:
+    return ppc::getPPCTargetCPU(Args, T);
 
-    return TargetCPUName;
-  }
   case llvm::Triple::csky:
     if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ))
       return A->getValue();
     else if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
       return A->getValue();
     else
       return "ck810";
   case llvm::Triple::riscv32:

clang/test/Driver/ppc-cpus.c

 // RUN: %clang -### -c -target powerpc64 %s -mcpu=ppc64 2>&1 | FileCheck --check-prefix=MCPU_PPC64 %s
 // MCPU_PPC64: "-target-cpu" "ppc64"
 
 /// We cannot check much for -mcpu=native, but it should be replaced by a CPU name.
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=native 2>&1 | FileCheck --check-prefix=MCPU_NATIVE %s
 // MCPU_NATIVE-NOT: "-target-cpu" "native"
 
+/// Check that we are passing unknown mcpu options to the backend so an error
+/// can be triggered.
+// RUN: %clang -### -c -target powerpc64 %s -mcpu=asdf1234 2>&1 | FileCheck --check-prefix=MCPU_UNKNOWN %s
+// MCPU_UNKNOWN: "-target-cpu" "asdf1234"
+
+/// Check for the unknown target error if an unknown mcpu option is used.
+// RUN: not %clang -c -target powerpc64 %s -mcpu=asdf1234 2>&1 | FileCheck --check-prefix=MCPU_ERR %s
+// MCPU_ERR: unknown target CPU 'asdf1234'
+
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=7400 2>&1 | FileCheck --check-prefix=MCPU_7400 %s
 // MCPU_7400: "-target-cpu" "7400"
 
 /// The following -mcpu= have their own -target-cpu values.
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=G4 2>&1 | FileCheck %s --check-prefix=NO_PPC64
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=7450 2>&1 | FileCheck %s --check-prefix=NO_PPC64
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=G4+ 2>&1 | FileCheck %s --check-prefix=NO_PPC64
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=970 2>&1 | FileCheck %s --check-prefix=NO_PPC64