[Driver] Update handling of c++ and runtime directories

[phosek]

Summary

This is a follow up to r361432 and r361504 which addresses issues
introduced by those changes. Specifically, it avoids duplicating
file and runtime paths in case when the effective triple is the
same as the cannonical one. Furthermore, it fixes the broken multilib
setup in the Fuchsia driver and deduplicates some of the code.

[mcgrathr]

lgtm contingent on verifying intended behavior changes and adding comments. The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.

clang/lib/Driver/Driver.cpp

4428 ↗

(On Diff #201383)

I see the renaming Library->Runtime throughout (not clear why, but OK by me). This also swaps the order of getFilePaths vs getRuntimePaths, which seems significant. There's a woeful lack of comments in this function about its essential logic, which is the order in which it consults all the available sources of paths.
So changing the ordering a bit without any comments is concerning.

I'd like to see more comments overall, but as this is a preexisting problem in this code, it's enough for this change that you affirm this ordering change was intentional and add some commentary somewhere about the material change in behavior (here or in the log message).

clang/lib/Driver/ToolChain.cpp

389 ↗

(On Diff #201383)

Can you explain the change to use the first entry rather than the first existing entry?
If the first one in the list is presumed to exist, then why is there a list instead of a single directory stored?

410 ↗

(On Diff #201383)

Comment about the distinction between D.getTargetTriple() and Triple.str() and why this order between them.

424 ↗

(On Diff #201383)

Same here. Can these use a common subroutine/template for trying something based on these two, so the logic about what D.getTargetTriple() and Triple.str() mean and both the code and justifying comments for their ordering logic is de-duplicated?

clang/test/Driver/fuchsia.c

96 ↗

(On Diff #201383)

To clarify, these are dropped because their sole intent was always to serve -lc++ and that's properly not enabled for this C-only link?

[phosek]

In D62442#1517183, @mcgrathr wrote:

lgtm contingent on verifying intended behavior changes and adding comments. The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.

I have undone that factoring as I have realized I could simplify the logic further, but as that's going to require more cleanup I'll do it in a follow up change.

clang/lib/Driver/ToolChain.cpp

389 ↗

(On Diff #201383)

That was unintentional, I've reverted that change.

424 ↗

(On Diff #201383)

I plan on cleaning up the runtime handling in a follow up change.

clang/test/Driver/fuchsia.c

96 ↗

(On Diff #201383)

Correct.

include/clang/Driver/ToolChain.h

public:
   virtual std::string getCompilerRT(const llvm::opt::ArgList &Args,
                                     StringRef Component,
                                     FileType Type = ToolChain::FT_Static) const;
 
   const char *
   getCompilerRTArgString(const llvm::opt::ArgList &Args, StringRef Component,
                          FileType Type = ToolChain::FT_Static) const;
 
+  // Returns target specific runtime path if it exists.
+  virtual Optional<std::string> getRuntimePath() const;
+
+  // Returns target specific C++ library path if it exists.
+  virtual Optional<std::string> getCXXStdlibPath() const;
+
   // Returns <ResourceDir>/lib/<OSName>/<arch>.  This is used by runtimes (such
   // as OpenMP) to find arch-specific libraries.
   std::string getArchSpecificLibPath() const;
 
   // Returns <OSname> part of above.
   StringRef getOSLibName() const;
 
   /// needsProfileRT - returns true if instrumentation profile is on.

lib/Driver/ToolChain.cpp

static ToolChain::RTTIMode CalculateRTTIMode(const ArgList &Args,
   // -frtti is default, except for the PS4 CPU.
   return (Triple.isPS4CPU()) ? ToolChain::RM_Disabled : ToolChain::RM_Enabled;
 }
 
 ToolChain::ToolChain(const Driver &D, const llvm::Triple &T,
                      const ArgList &Args)
     : D(D), Triple(T), Args(Args), CachedRTTIArg(GetRTTIArgument(Args)),
       CachedRTTIMode(CalculateRTTIMode(Args, Triple, CachedRTTIArg)) {
-  SmallString<128> P;
-
   if (D.CCCIsCXX()) {
-    P.assign(D.Dir);
-    llvm::sys::path::append(P, "..", "lib", D.getTargetTriple(), "c++");
-    if (getVFS().exists(P))
-      getLibraryPaths().push_back(P.str());
-
-    P.assign(D.Dir);
-    llvm::sys::path::append(P, "..", "lib", Triple.str(), "c++");
-    if (getVFS().exists(P))
-      getLibraryPaths().push_back(P.str());
+    if (auto CXXStdlibPath = getCXXStdlibPath())
+      getFilePaths().push_back(*CXXStdlibPath);
   }
 
-  P.assign(D.ResourceDir);
-  llvm::sys::path::append(P, D.getTargetTriple(), "lib");
-  if (getVFS().exists(P))
-    getLibraryPaths().push_back(P.str());
-
-  P.assign(D.ResourceDir);
-  llvm::sys::path::append(P, Triple.str(), "lib");
-  if (getVFS().exists(P))
-    getLibraryPaths().push_back(P.str());
+  if (auto RuntimePath = getRuntimePath())
+    getLibraryPaths().push_back(*RuntimePath);
 
   std::string CandidateLibPath = getArchSpecificLibPath();
   if (getVFS().exists(CandidateLibPath))
     getFilePaths().push_back(CandidateLibPath);
 }
 
 void ToolChain::setTripleEnvironment(llvm::Triple::EnvironmentType Env) {
   Triple.setEnvironment(Env);

 }
 
 const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList &Args,
                                               StringRef Component,
                                               FileType Type) const {
   return Args.MakeArgString(getCompilerRT(Args, Component, Type));
 }
 
+
+Optional<std::string> ToolChain::getRuntimePath() const {
+  SmallString<128> P;
+
+  // First try the triple passed to driver as --target=<triple>.
+  P.assign(D.ResourceDir);
+  llvm::sys::path::append(P, D.getTargetTriple(), "lib");
+  if (getVFS().exists(P))
+    return llvm::Optional<std::string>(P.str());
+
+  // Second try the normalized triple.
+  P.assign(D.ResourceDir);
+  llvm::sys::path::append(P, Triple.str(), "lib");
+  if (getVFS().exists(P))
+    return llvm::Optional<std::string>(P.str());
+
+  return None;
+}
+
+Optional<std::string> ToolChain::getCXXStdlibPath() const {
+  SmallString<128> P;
+
+  // First try the triple passed to driver as --target=<triple>.
+  P.assign(D.Dir);
+  llvm::sys::path::append(P, "..", "lib", D.getTargetTriple(), "c++");
+  if (getVFS().exists(P))
+    return llvm::Optional<std::string>(P.str());
+
+  // Second try the normalized triple.
+  P.assign(D.Dir);
+  llvm::sys::path::append(P, "..", "lib", Triple.str(), "c++");
+  if (getVFS().exists(P))
+    return llvm::Optional<std::string>(P.str());
+
+  return None;
+}
+
 std::string ToolChain::getArchSpecificLibPath() const {
   SmallString<128> Path(getDriver().ResourceDir);
   llvm::sys::path::append(Path, "lib", getOSLibName(),
                           llvm::Triple::getArchTypeName(getArch()));
   return Path.str();
 }
 
 bool ToolChain::needsProfileRT(const ArgList &Args) {
void ToolChain::AddCXXStdlibLibArgs(const ArgList &Args,
   case ToolChain::CST_Libstdcxx:
     CmdArgs.push_back("-lstdc++");
     break;
   }
 }
 
 void ToolChain::AddFilePathLibArgs(const ArgList &Args,
                                    ArgStringList &CmdArgs) const {
-  for (const auto &LibPath : getLibraryPaths())
-    if(LibPath.length() > 0)
-      CmdArgs.push_back(Args.MakeArgString(StringRef("-L") + LibPath));
-
   for (const auto &LibPath : getFilePaths())
     if(LibPath.length() > 0)
       CmdArgs.push_back(Args.MakeArgString(StringRef("-L") + LibPath));
 }
 
 void ToolChain::AddCCKextLibArgs(const ArgList &Args,
                                  ArgStringList &CmdArgs) const {
   CmdArgs.push_back("-lcc_kext");

lib/Driver/ToolChains/Fuchsia.cpp

if (getDriver().getInstalledDir() != D.Dir)
     getProgramPaths().push_back(D.Dir);
 
   if (!D.SysRoot.empty()) {
     SmallString<128> P(D.SysRoot);
     llvm::sys::path::append(P, "lib");
     getFilePaths().push_back(P.str());
   }
 
-  auto RuntimeDirs = [&](const Multilib &M) -> std::vector<std::string> {
-    SmallString<128> P;
-    std::vector<std::string> RD;
-
-    P.assign(D.ResourceDir);
-    llvm::sys::path::append(P, D.getTargetTriple(), "lib", M.gccSuffix());
-    if (getVFS().exists(P))
-      RD.push_back(P.str());
-
-    P.assign(D.ResourceDir);
-    llvm::sys::path::append(P, Triple.str(), "lib", M.gccSuffix());
-    if (getVFS().exists(P))
-      RD.push_back(P.str());
-
-    return RD;
+  auto FilePaths = [&](const Multilib &M) -> std::vector<std::string> {
+    std::vector<std::string> FP;
+    if (D.CCCIsCXX()) {
+      if (auto CXXStdlibPath = getCXXStdlibPath()) {
+        SmallString<128> P(*CXXStdlibPath);
+        llvm::sys::path::append(P, M.gccSuffix());
+        FP.push_back(P.str());
+      }
+    }
+    return FP;
   };
 
   Multilibs.push_back(Multilib());
   // Use the noexcept variant with -fno-exceptions to avoid the extra overhead.
   Multilibs.push_back(Multilib("noexcept", {}, {}, 1)
                           .flag("-fexceptions")
                           .flag("+fno-exceptions"));
   // ASan has higher priority because we always want the instrumentated version.
   Multilibs.push_back(Multilib("asan", {}, {}, 2)
                           .flag("+fsanitize=address"));
   Multilibs.FilterOut([&](const Multilib &M) {
-    std::vector<std::string> RD = RuntimeDirs(M);
+    std::vector<std::string> RD = FilePaths(M);
     return std::all_of(RD.begin(), RD.end(), [&](std::string P) {
       return !getVFS().exists(P);
     });
   });
 
   Multilib::flags_list Flags;
   addMultilibFlag(
       Args.hasFlag(options::OPT_fexceptions, options::OPT_fno_exceptions, true),
       "fexceptions", Flags);
   addMultilibFlag(getSanitizerArgs().needsAsanRt(), "fsanitize=address", Flags);
-  Multilibs.setFilePathsCallback(RuntimeDirs);
+  Multilibs.setFilePathsCallback(FilePaths);
 
   if (Multilibs.select(Flags, SelectedMultilib))
     if (!SelectedMultilib.isDefault())
       if (const auto &PathsCallback = Multilibs.filePathsCallback())
         for (const auto &Path : PathsCallback(SelectedMultilib))
-          // We need to prepend the multilib path to ensure it takes precedence.
-          getLibraryPaths().insert(getLibraryPaths().begin(), Path);
+          // Prepend the multilib path to ensure it takes the precedence.
+          getFilePaths().insert(getFilePaths().begin(), Path);
 }
 
 std::string Fuchsia::ComputeEffectiveClangTriple(const ArgList &Args,
                                                  types::ID InputType) const {
   llvm::Triple Triple(ComputeLLVMTriple(Args, InputType));
   return (Triple.getArchName() + "-" + Triple.getOSName()).str();
 }
 

test/Driver/fuchsia.c

 // RUN:     -fsanitize=address 2>&1 \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld \
 // RUN:     | FileCheck %s -check-prefix=CHECK-ASAN-X86
 // CHECK-ASAN-X86: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // CHECK-ASAN-X86: "-fsanitize=address"
 // CHECK-ASAN-X86: "-fsanitize-address-globals-dead-stripping"
 // CHECK-ASAN-X86: "-dynamic-linker" "asan/ld.so.1"
-// CHECK-ASAN-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib{{/|\\\\}}asan"
-// CHECK-ASAN-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib"
 // CHECK-ASAN-X86: "[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib{{/|\\\\}}libclang_rt.asan.so"
 // CHECK-ASAN-X86: "[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib{{/|\\\\}}libclang_rt.asan-preinit.a"
 
 // RUN: %clang %s -### --target=aarch64-fuchsia \
 // RUN:     -fsanitize=address 2>&1 \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld \
 // RUN:     | FileCheck %s -check-prefix=CHECK-ASAN-AARCH64
 // CHECK-ASAN-AARCH64: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // CHECK-ASAN-AARCH64: "-fsanitize=address"
 // CHECK-ASAN-AARCH64: "-fsanitize-address-globals-dead-stripping"
 // CHECK-ASAN-AARCH64: "-dynamic-linker" "asan/ld.so.1"
-// CHECK-ASAN-AARCH64: "-L[[RESOURCE_DIR]]{{/|\\\\}}aarch64-fuchsia{{/|\\\\}}lib{{/|\\\\}}asan"
-// CHECK-ASAN-AARCH64: "-L[[RESOURCE_DIR]]{{/|\\\\}}aarch64-fuchsia{{/|\\\\}}lib"
 // CHECK-ASAN-AARCH64: "[[RESOURCE_DIR]]{{/|\\\\}}aarch64-fuchsia{{/|\\\\}}lib{{/|\\\\}}libclang_rt.asan.so"
 // CHECK-ASAN-AARCH64: "[[RESOURCE_DIR]]{{/|\\\\}}aarch64-fuchsia{{/|\\\\}}lib{{/|\\\\}}libclang_rt.asan-preinit.a"
 
 // RUN: %clang %s -### --target=x86_64-fuchsia \
 // RUN:     -fsanitize=address -fPIC -shared 2>&1 \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld \
 // RUN:     | FileCheck %s -check-prefix=CHECK-ASAN-SHARED

test/Driver/fuchsia.cpp

 // RUN: %clangxx %s -### -no-canonical-prefixes --target=x86_64-fuchsia \
+// RUN:     -ccc-install-dir %S/Inputs/basic_fuchsia_tree/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     --sysroot=%S/platform -fuse-ld=lld 2>&1 | FileCheck %s
 // CHECK: {{.*}}clang{{.*}}" "-cc1"
 // CHECK: "-triple" "x86_64-fuchsia"
 // CHECK: "-fuse-init-array"
 // CHECK: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK: "-internal-isystem" "{{.*[/\\]}}include{{/|\\\\}}c++{{/|\\\\}}v1"

 // CHECK-STATIC: "--as-needed"
 // CHECK-STATIC: "-Bstatic"
 // CHECK-STATIC: "-lc++"
 // CHECK-STATIC: "-Bdynamic"
 // CHECK-STATIC: "-lm"
 // CHECK-STATIC: "--pop-state"
 // CHECK-STATIC: "-lc"
 
-// RUN: %clang %s -### --target=x86_64-fuchsia -nostdlib++ -fuse-ld=lld 2>&1 \
+// RUN: %clangxx %s -### --target=x86_64-fuchsia -nostdlib++ -fuse-ld=lld 2>&1 \
 // RUN:     | FileCheck %s -check-prefix=CHECK-NOSTDLIBXX
 // CHECK-NOSTDLIBXX-NOT: "-lc++"
 // CHECK-NOSTDLIBXX-NOT: "-lm"
 // CHECK-NOSTDLIBXX: "-lc"
 
-// RUN: %clang %s -### --target=x86_64-fuchsia \
+// RUN: %clangxx %s -### --target=x86_64-fuchsia \
+// RUN:     -ccc-install-dir %S/Inputs/basic_fuchsia_tree/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld 2>&1\
 // RUN:     | FileCheck %s -check-prefixes=CHECK-MULTILIB-X86
-// RUN: %clang %s -### --target=x86_64-fuchsia -fsanitize=address \
+// RUN: %clangxx %s -### --target=x86_64-fuchsia -fsanitize=address \
+// RUN:     -ccc-install-dir %S/Inputs/basic_fuchsia_tree/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld 2>&1\
 // RUN:     | FileCheck %s -check-prefixes=CHECK-MULTILIB-X86,CHECK-MULTILIB-ASAN-X86
-// RUN: %clang %s -### --target=x86_64-fuchsia -fno-exceptions \
+// RUN: %clangxx %s -### --target=x86_64-fuchsia -fno-exceptions \
+// RUN:     -ccc-install-dir %S/Inputs/basic_fuchsia_tree/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld 2>&1\
 // RUN:     | FileCheck %s -check-prefixes=CHECK-MULTILIB-X86,CHECK-MULTILIB-NOEXCEPT-X86
-// RUN: %clang %s -### --target=x86_64-fuchsia -fsanitize=address -fno-exceptions \
+// RUN: %clangxx %s -### --target=x86_64-fuchsia -fsanitize=address -fno-exceptions \
+// RUN:     -ccc-install-dir %S/Inputs/basic_fuchsia_tree/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -fuse-ld=lld 2>&1\
 // RUN:     | FileCheck %s -check-prefixes=CHECK-MULTILIB-X86,CHECK-MULTILIB-ASAN-X86
 // CHECK-MULTILIB-X86: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
-// CHECK-MULTILIB-ASAN-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib{{/|\\\\}}asan"
-// CHECK-MULTILIB-NOEXCEPT-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib{{/|\\\\}}noexcept"
-// CHECK-MULTILIB-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib"
+// CHECK-MULTILIB-ASAN-X86: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}lib{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}c++{{/|\\\\}}asan"
+// CHECK-MULTILIB-NOEXCEPT-X86: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}lib{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}c++{{/|\\\\}}noexcept"
+// CHECK-MULTILIB-X86: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}lib{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}c++"

test/Driver/linux-per-target-runtime-dir.c

 // RUN: %clangxx -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
 // RUN:     --target=x86_64-linux-gnu \
 // RUN:     -stdlib=libc++ \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -ccc-install-dir %S/Inputs/basic_linux_libcxx_tree/usr/bin \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/basic_linux_libcxx_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-PER-TARGET-RUNTIME %s
 // CHECK-PER-TARGET-RUNTIME: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-PER-TARGET-RUNTIME: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-PER-TARGET-RUNTIME: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}/../include/c++/v1"
 // CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}lib{{/|\\\\}}x86_64-linux-gnu{{/|\\\\}}c++"
-// CHECK-PER-TARGET-RUNTIME: "-L[[RESDIR]]{{/|\\\\}}x86_64-linux-gnu{{/|\\\\}}lib"
 
 // RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name 2>&1 \
 // RUN:     --target=x86_64-linux-gnu \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-X8664 %s
 // CHECK-CLANGRT-X8664: x86_64-linux-gnu{{/|\\}}lib{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN:     --target=x86_64-linux-gnu \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-X8664 %s
 // CHECK-FILE-NAME-X8664: x86_64-linux-gnu{{/|\\}}lib{{/|\\}}libclang_rt.builtins.a