Call attention to strange case in getMultiarchTriple, and DRY so much.

[dougk]

Summary

There is a suspicious fallthrough at ppc64 which on first glance seems unlikely to be right, but also seems unlikely to cause a problem, and remains as-is but with a comment. It seems like it should have resembled the pattern of the cases above it:
if (test) return "/some/path"; return TargetTriple.str();
whereas it actually fell into the ppc64le case.
I am not sure what the intent was, so this patch adds an explicit "// Fallthrough" comment.

Additionally there is a uniform catch-all case at the end rather than having every architecture recognize both a special case (or two) and the general case.

[chandlerc]

I think you should just fix the code. This seems clearly a bug.

With that fix, this LGTM.

lib/Driver/ToolChains.cpp

3096 ↗

(On Diff #28121)

This is certainly wrong. Please just break here, that is what the code should have been doing. We don't want to synthesize a LE multiarch location for a non-LE triple ever.

(Also, good catch!)

cfe/trunk/lib/Driver/ToolChains.cpp

 /// \brief Get our best guess at the multiarch triple for a target.
 ///
 /// Debian-based systems are starting to use a multiarch setup where they use
 /// a target-triple directory in the library and header search paths.
 /// Unfortunately, this triple does not align with the vanilla target triple,
 /// so we provide a rough mapping here.
 static std::string getMultiarchTriple(const llvm::Triple &TargetTriple,
                                       StringRef SysRoot) {
+  llvm::Triple::EnvironmentType TargetEnvironment = TargetTriple.getEnvironment();
+
   // For most architectures, just use whatever we have rather than trying to be
   // clever.
   switch (TargetTriple.getArch()) {
   default:
-    return TargetTriple.str();
+    break;
 
   // We use the existence of '/lib/<triple>' as a directory to detect some
   // common linux triples that don't quite match the Clang triple for both
   // 32-bit and 64-bit targets. Multiarch fixes its install triples to these
   // regardless of what the actual target triple is.
   case llvm::Triple::arm:
   case llvm::Triple::thumb:
-    if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF) {
+    if (TargetEnvironment == llvm::Triple::GNUEABIHF) {
       if (llvm::sys::fs::exists(SysRoot + "/lib/arm-linux-gnueabihf"))
         return "arm-linux-gnueabihf";
     } else {
       if (llvm::sys::fs::exists(SysRoot + "/lib/arm-linux-gnueabi"))
         return "arm-linux-gnueabi";
     }
-    return TargetTriple.str();
+    break;
   case llvm::Triple::armeb:
   case llvm::Triple::thumbeb:
-    if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF) {
+    if (TargetEnvironment == llvm::Triple::GNUEABIHF) {
       if (llvm::sys::fs::exists(SysRoot + "/lib/armeb-linux-gnueabihf"))
         return "armeb-linux-gnueabihf";
     } else {
       if (llvm::sys::fs::exists(SysRoot + "/lib/armeb-linux-gnueabi"))
         return "armeb-linux-gnueabi";
     }
-    return TargetTriple.str();
+    break;
   case llvm::Triple::x86:
     if (llvm::sys::fs::exists(SysRoot + "/lib/i386-linux-gnu"))
       return "i386-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::x86_64:
     // We don't want this for x32, otherwise it will match x86_64 libs
-    if (TargetTriple.getEnvironment() != llvm::Triple::GNUX32 &&
+    if (TargetEnvironment != llvm::Triple::GNUX32 &&
         llvm::sys::fs::exists(SysRoot + "/lib/x86_64-linux-gnu"))
       return "x86_64-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::aarch64:
     if (llvm::sys::fs::exists(SysRoot + "/lib/aarch64-linux-gnu"))
       return "aarch64-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::aarch64_be:
     if (llvm::sys::fs::exists(SysRoot + "/lib/aarch64_be-linux-gnu"))
       return "aarch64_be-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::mips:
     if (llvm::sys::fs::exists(SysRoot + "/lib/mips-linux-gnu"))
       return "mips-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::mipsel:
     if (llvm::sys::fs::exists(SysRoot + "/lib/mipsel-linux-gnu"))
       return "mipsel-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::mips64:
     if (llvm::sys::fs::exists(SysRoot + "/lib/mips64-linux-gnu"))
       return "mips64-linux-gnu";
     if (llvm::sys::fs::exists(SysRoot + "/lib/mips64-linux-gnuabi64"))
       return "mips64-linux-gnuabi64";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::mips64el:
     if (llvm::sys::fs::exists(SysRoot + "/lib/mips64el-linux-gnu"))
       return "mips64el-linux-gnu";
     if (llvm::sys::fs::exists(SysRoot + "/lib/mips64el-linux-gnuabi64"))
       return "mips64el-linux-gnuabi64";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::ppc:
     if (llvm::sys::fs::exists(SysRoot + "/lib/powerpc-linux-gnuspe"))
       return "powerpc-linux-gnuspe";
     if (llvm::sys::fs::exists(SysRoot + "/lib/powerpc-linux-gnu"))
       return "powerpc-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::ppc64:
     if (llvm::sys::fs::exists(SysRoot + "/lib/powerpc64-linux-gnu"))
       return "powerpc64-linux-gnu";
+    break;
   case llvm::Triple::ppc64le:
     if (llvm::sys::fs::exists(SysRoot + "/lib/powerpc64le-linux-gnu"))
       return "powerpc64le-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::sparc:
     if (llvm::sys::fs::exists(SysRoot + "/lib/sparc-linux-gnu"))
       return "sparc-linux-gnu";
-    return TargetTriple.str();
+    break;
   case llvm::Triple::sparcv9:
     if (llvm::sys::fs::exists(SysRoot + "/lib/sparc64-linux-gnu"))
       return "sparc64-linux-gnu";
-    return TargetTriple.str();
+    break;
   }
+  return TargetTriple.str();
 }
 
 static void addPathIfExists(Twine Path, ToolChain::path_list &Paths) {
   if (llvm::sys::fs::exists(Path))
     Paths.push_back(Path.str());
 }
 
 static StringRef getOSLibDir(const llvm::Triple &Triple, const ArgList &Args) {

cfe/trunk/test/Driver/linux-ld.c

 // CHECK-DEBIAN-PPC: "{{.*}}/usr/lib/gcc/powerpc-linux-gnu/4.5{{/|\\\\}}crtbegin.o"
 // CHECK-DEBIAN-PPC: "-L[[SYSROOT]]/usr/lib/gcc/powerpc-linux-gnu/4.5"
 // CHECK-DEBIAN-PPC: "-L[[SYSROOT]]/usr/lib/gcc/powerpc-linux-gnu/4.5/../../../powerpc-linux-gnu"
 // CHECK-DEBIAN-PPC: "-L[[SYSROOT]]/usr/lib/powerpc-linux-gnu"
 // CHECK-DEBIAN-PPC: "-L[[SYSROOT]]/usr/lib/gcc/powerpc-linux-gnu/4.5/../../.."
 // CHECK-DEBIAN-PPC: "-L[[SYSROOT]]/lib"
 // CHECK-DEBIAN-PPC: "-L[[SYSROOT]]/usr/lib"
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=powerpc64le-linux-gnu \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/debian_multiarch_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-DEBIAN-PPC64LE %s
+// CHECK-DEBIAN-PPC64LE: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-DEBIAN-PPC64LE: "{{.*}}/usr/lib/gcc/powerpc64le-linux-gnu/4.5{{/|\\\\}}crtbegin.o"
+// CHECK-DEBIAN-PPC64LE: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64le-linux-gnu/4.5"
+// CHECK-DEBIAN-PPC64LE: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64le-linux-gnu/4.5/../../../powerpc64le-linux-gnu"
+// CHECK-DEBIAN-PPC64LE: "-L[[SYSROOT]]/usr/lib/powerpc64le-linux-gnu"
+// CHECK-DEBIAN-PPC64LE: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64le-linux-gnu/4.5/../../.."
+// CHECK-DEBIAN-PPC64LE: "-L[[SYSROOT]]/lib"
+// CHECK-DEBIAN-PPC64LE: "-L[[SYSROOT]]/usr/lib"
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     --target=powerpc64-linux-gnu \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/debian_multiarch_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-DEBIAN-PPC64 %s
 // CHECK-DEBIAN-PPC64: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-DEBIAN-PPC64: "{{.*}}/usr/lib/gcc/powerpc64-linux-gnu/4.5{{/|\\\\}}crtbegin.o"
 // CHECK-DEBIAN-PPC64: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64-linux-gnu/4.5"
 // CHECK-DEBIAN-PPC64: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64-linux-gnu/4.5/../../../powerpc64-linux-gnu"