Add arch-specific directory to search path

[pirama]

Summary

This change adds an arch-specific subdirectory in <ResourceDir>/lib/<OS>
to the linker search path. This path also gets added as '-rpath' for
native compilation if a runtime is linked in as a shared object. This
allows arch-specific libraries to be installed alongside clang.

[danalbert]

LGTM, but should probably get signoff from someone else as well.

[srhines]

include/clang/Driver/ToolChain.h

304

Add a period at the end of the sentence.

[Hahnfeld]

Why is this only for OpenMP? I imagine this should be done for all runtime libraries

[pirama]

In D30015#678243, @Hahnfeld wrote:

Why is this only for OpenMP? I imagine this should be done for all runtime libraries

The other runtime libraries have the arch included in the library's name and the driver links the correct file. cbergstrom preferred to look in an arch-specific directory rather (http://lists.llvm.org/pipermail/openmp-dev/2017-February/001659.html). It makes sense for OpenMP because other toolchains may assume the standard library name (libomp.so).

[Hahnfeld]

Yes, that's the current state. We had some discussion on cfe-dev on that matter: http://lists.llvm.org/pipermail/cfe-dev/2017-January/052512.html

Could you slightly adapt your patch so that other runtime libraries can follow this idea in the future?

[pirama]

Changed the name of the utility that builds the arch-specific directory.

[mgorny]

lib/Driver/ToolChain.cpp

332

I would suggest using arch type, to avoid e.g. i386/i486/i586 mess. It's really hard to clean it up afterwards, see D26796.

[mgorny]

lib/Driver/Tools.cpp

3278

Don't you also need rpath for it? Or is this purely for static runtime?

[pirama]

Use getArchTypeName() instead of getArchName().

[pirama]

lib/Driver/Tools.cpp

3278

I am doing this for a cross-compiling toolchain (Android NDK) where the actual rpath is not valid at runtime. The runtime is packaged with the application and made available to the loader behind the scenes.

That said, I don't think providing an rpath doesn't hurt. I'll wait for input from @cbergstrom and OpenMP folks to see if this'd be useful.

[Hahnfeld]

I still think this should not be done for OpenMP only and hence move out of addOpenMPRuntime.

(Why the heck are there two places to add -lomp?!? I'll clean that up later...)

[pirama]

I am fine to do this more generally, but not sure of the scope. Should this be always added or only if a runtime (sanitizer or openmp or xray) is requested?

[Hahnfeld]

In D30015#678294, @pirama wrote:

I am fine to do this more generally, but not sure of the scope. Should this be always added or only if a runtime (sanitizer or openmp or xray) is requested?

I think always. From my point of view, this would be the right solution for my D26244.

[Hahnfeld]

Maybe something like the following (without tests):

diff --git a/include/clang/Driver/ToolChain.h b/include/clang/Driver/ToolChain.h
index ffb0d60..0f3507d 100644
--- a/include/clang/Driver/ToolChain.h
+++ b/include/clang/Driver/ToolChain.h
@@ -299,6 +299,11 @@ public:
   const char *getCompilerRTArgString(const llvm::opt::ArgList &Args,
                                      StringRef Component,
                                      bool Shared = false) const;
+
+  /// Returns <ResourceDir>/lib/<OSName>/<arch>.  This is used by runtimes (such
+  /// as OpenMP) to find arch-specific libraries.
+  const std::string getArchSpecificLibPath() const;
+
   /// needsProfileRT - returns true if instrumentation profile is on.
   static bool needsProfileRT(const llvm::opt::ArgList &Args);
 
diff --git a/lib/Driver/ToolChain.cpp b/lib/Driver/ToolChain.cpp
index 6adc038..ae2c08e 100644
--- a/lib/Driver/ToolChain.cpp
+++ b/lib/Driver/ToolChain.cpp
@@ -10,6 +10,7 @@
 #include "clang/Driver/ToolChain.h"
 #include "Tools.h"
 #include "clang/Basic/ObjCRuntime.h"
+#include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Driver.h"
@@ -74,6 +75,10 @@ ToolChain::ToolChain(const Driver &D, const llvm::Triple &T,
     if (!isThreadModelSupported(A->getValue()))
       D.Diag(diag::err_drv_invalid_thread_model_for_target)
           << A->getValue() << A->getAsString(Args);
+
+  std::string CandidateLibPath = getArchSpecificLibPath();
+  if (getVFS().exists(CandidateLibPath))
+    getFilePaths().push_back(CandidateLibPath);
 }
 
 ToolChain::~ToolChain() {
@@ -320,6 +325,14 @@ const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList &Args,
   return Args.MakeArgString(getCompilerRT(Args, Component, Shared));
 }
 
+const std::string ToolChain::getArchSpecificLibPath() const {
+  SmallString<128> Path(getDriver().ResourceDir);
+  StringRef OSLibName = getTriple().isOSFreeBSD() ? "freebsd" : getOS();
+  llvm::sys::path::append(Path, "lib", OSLibName,
+                          llvm::Triple::getArchTypeName(getArch()));
+  return Path.str();
+}
+
 bool ToolChain::needsProfileRT(const ArgList &Args) {
   if (Args.hasFlag(options::OPT_fprofile_arcs, options::OPT_fno_profile_arcs,
                    false) ||
diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp
index cc7d9e1..c0fb4bc 100644
--- a/lib/Driver/Tools.cpp
+++ b/lib/Driver/Tools.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/Version.h"
+#include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
@@ -276,8 +277,15 @@ static void AddLinkerInputs(const ToolChain &TC, const InputInfoList &Inputs,
 
   // LIBRARY_PATH - included following the user specified library paths.
   //                and only supported on native toolchains.
-  if (!TC.isCrossCompiling())
+  if (!TC.isCrossCompiling()) {
     addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
+
+    std::string CandidateRPath = TC.getArchSpecificLibPath();
+    if (D.getVFS().exists(CandidateRPath)) {
+      CmdArgs.push_back("-rpath");
+      CmdArgs.push_back(Args.MakeArgString(CandidateRPath.c_str()));
+    }
+  }
 }
 
 /// Add OpenMP linker script arguments at the end of the argument list so that

[pirama]

• Arch-subdir is now always added to -L
• It is added to -rpath during native compilation.
• Tests have been updated

[pirama]

test/Driver/arch-specific-libdir-rpath.c

7

I feel this test is fragile. Any idea how to further restrict and require that the default target triple has linux and one of i386, x86_64, arm, aarch64?

I could create sub-dirs for all archs returned by Triple::getArchTypeName (llvm/lib/Support/Triple.cpp) but it seems overkill.

[mgorny]

test/Driver/arch-specific-libdir.c

7

Please be more specific in the tests, i.e. check if the correct arch dir is selected. Also add a test for i386-* triple, and make sure that both i386-* and i686-* give the same result.

[pirama]

Stricter tests.

[pirama]

test/Driver/arch-specific-libdir-rpath.c

7

I've restricted the test to just x86_64-linux.

[Hahnfeld]

Please adapt the title and summary for the more general changes this has evolved to.

lib/Driver/Tools.cpp

284

s/lined/linked/

test/Driver/arch-specific-libdir-rpath.c

7

Instead of REQUIRES, you should probably add -target x86_64-unknown-linux

18

Can you split that into two lines? Then it won't fail if there is some argument added in between

[mgorny]

Thanks. The -L tests look good, -rpath is not perfect but I don't think you can improve it without additional changes to the Driver.

test/Driver/arch-specific-libdir-rpath.c

7

Hmm, I don't see any good solution for this. Looking at the code, the host triple is hardcoded, so you'd indeed have to either restrict the test to triples matching LLVM_HOST_TRIPLE. Or maybe add a command-line option to override the host triple.

[Hahnfeld]

test/Driver/arch-specific-libdir-rpath.c

7

Sorry, I missed that it's cross-compiling then. x86_64 is in that case probably the best thing to do because it's most widespread, also among the buildbots.

[pirama]

Fix typo.

[pirama]

Add arch name to -rpath test.

[mgorny]

lib/Driver/Tools.cpp

3278

@cbergstrom says: 'let them know that your comments are equivalent (or better) than mine'. But yeah, PathScale's adding rpaths for the libraries on our end, so this is consistent with what we do.

[pirama]

test/Driver/arch-specific-libdir-rpath.c

18

Splitting into two lines makes FileCheck eagerly match the arch-subdir and -rpath into the {{.*}} next to the "-L". This causes the check for -rpath to fail.

The test accepts intermediate arguments because of the wildcard right after the -L...Inputs/...

[pirama]

I believe I've addressed all open issues, but this patch has changed a lot since Dan marked it accepted. @Hahnfeld or @mgorny: Can one of you give a LGTM if you are satisfied?

[mgorny]

LGTM modulo the test match split. But please wait for someone who has been longer here to confirm.

test/Driver/arch-specific-libdir-rpath.c

18

Please do not rely on implicit assumptions like that. One day someone may decide to 'fix' the wildcard not to match whitespace, and make the wrong assumption about order. If for anything, splitting in two would make this more readable.

[pirama]

test/Driver/arch-specific-libdir-rpath.c

18

Do you have any suggestions? Right now, if I split it into two lines, the second check fails due to the eager regex match.

[pirama]

@rnk: This patch was to address discussion in http://lists.llvm.org/pipermail/openmp-dev/2017-February/001659.html but now also addresses http://lists.llvm.org/pipermail/cfe-dev/2017-January/052512.html. Can you take a look?

[rnk]

include/clang/Driver/ToolChain.h

305

Why const qualify the std::string?

lib/Driver/Tools.cpp

289

We shouldn't add rpath to every binary linked by clang. We should only add rpath when we know the user is linking against a shared library provided by clang. See for example the way the darwin toolchain handles this in Toolchains.cpp.

Right now most libraries provided by clang are static, so there hasn't been much need to add rpath, but if we add more shared libraries (xray, sanitizers, profiling, etc), then I think we'll want to do this along with adding an option to suppress the implicit addition of rpath.

[pirama]

Add -rpath only when a runtime is included as a shared library. And, cleanup tests.

[pirama]

lib/Driver/Tools.cpp

289

PTAL.

test/Driver/arch-specific-libdir-rpath.c

18

Done. I was able to get the exact path from the -cc1 invocation and use it to remove the wildcards.

[rnk]

Looks good with a minor comment about a comment in the test case.

lib/Driver/Tools.cpp

3262–3264

This seems like a really poor heuristic for "will the user ship this binary to another computer that doesn't have clang installed in the same location", but the convenience of not having to add clang's unpredictably named resource library directory to LD_LIBRARY_PATH seems worth baking in a possibly-wrong rpath.

test/Driver/arch-specific-libdir-rpath.c

10

This comment seems wrong, with -shared-libasan we add it to rpath. It's really, only add rpath if we are using shared libraries.

[pirama]

Fixed comment in test and added a test for -fsanitize=address without -shared-libasan.

[pirama]

lib/Driver/Tools.cpp

3262–3264

This is indeed poor, but a good check that omits rpath when it is definitely useless but leave it in case it might be useful.

[pirama]

Committed. Thanks for the reviews!

include/clang/Driver/ToolChain.h

public:
 
   virtual std::string getCompilerRT(const llvm::opt::ArgList &Args,
                                     StringRef Component,
                                     bool Shared = false) const;
 
   const char *getCompilerRTArgString(const llvm::opt::ArgList &Args,
                                      StringRef Component,
                                      bool Shared = false) const;
+
+  // Returns <ResourceDir>/lib/<OSName>/<arch>.  This is used by runtimes (such
+  // as OpenMP) to find arch-specific libraries.
+  const std::string getArchSpecificLibPath() const;
+
   /// needsProfileRT - returns true if instrumentation profile is on.
   static bool needsProfileRT(const llvm::opt::ArgList &Args);
 
   /// IsUnwindTablesDefault - Does this tool chain use -funwind-tables
   /// by default.
   virtual bool IsUnwindTablesDefault() const;
 
   /// \brief Test whether this toolchain defaults to PIC.

lib/Driver/ToolChain.cpp

 //===--- ToolChain.cpp - Collections of tools for one platform ------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Driver/ToolChain.h"
 #include "Tools.h"
 #include "clang/Basic/ObjCRuntime.h"
+#include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/SanitizerArgs.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Option/Arg.h"
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)),
       EffectiveTriple() {
   if (Arg *A = Args.getLastArg(options::OPT_mthread_model))
     if (!isThreadModelSupported(A->getValue()))
       D.Diag(diag::err_drv_invalid_thread_model_for_target)
           << A->getValue() << A->getAsString(Args);
+
+  std::string CandidateLibPath = getArchSpecificLibPath();
+  if (getVFS().exists(CandidateLibPath))
+    getFilePaths().push_back(CandidateLibPath);
 }
 
 ToolChain::~ToolChain() {
 }
 
 vfs::FileSystem &ToolChain::getVFS() const { return getDriver().getVFS(); }
 
 bool ToolChain::useIntegratedAs() const {

 }
 
 const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList &Args,
                                               StringRef Component,
                                               bool Shared) const {
   return Args.MakeArgString(getCompilerRT(Args, Component, Shared));
 }
 
+const std::string ToolChain::getArchSpecificLibPath() const {
+  SmallString<128> Path(getDriver().ResourceDir);
+  StringRef OSLibName = getTriple().isOSFreeBSD() ? "freebsd" : getOS();
+  llvm::sys::path::append(Path, "lib", OSLibName,
+                          llvm::Triple::getArchTypeName(getArch()));
+  return Path.str();
+}
+
 bool ToolChain::needsProfileRT(const ArgList &Args) {
   if (Args.hasFlag(options::OPT_fprofile_arcs, options::OPT_fno_profile_arcs,
                    false) ||
       Args.hasArg(options::OPT_fprofile_generate) ||
       Args.hasArg(options::OPT_fprofile_generate_EQ) ||
       Args.hasArg(options::OPT_fprofile_instr_generate) ||
       Args.hasArg(options::OPT_fprofile_instr_generate_EQ) ||
       Args.hasArg(options::OPT_fcreate_profile) ||

lib/Driver/Tools.cpp

 //===--- Tools.cpp - Tools Implementations ----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Tools.h"
 #include "InputInfo.h"
 #include "ToolChains.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/Version.h"
+#include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Job.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/SanitizerArgs.h"
else if (A.getOption().matches(options::OPT_z)) {
       A.render(Args, CmdArgs);
     } else {
       A.renderAsInput(Args, CmdArgs);
     }
   }
 
   // LIBRARY_PATH - included following the user specified library paths.
   //                and only supported on native toolchains.
-  if (!TC.isCrossCompiling())
+  if (!TC.isCrossCompiling()) {
     addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
+
+    // If we are not cross-compling, add '-rpath' with architecture-specific
+    // library path so libraries lined from this path can be automatically
+    // found.
+    std::string CandidateRPath = TC.getArchSpecificLibPath();
+    if (D.getVFS().exists(CandidateRPath)) {
+      CmdArgs.push_back("-rpath");
+      CmdArgs.push_back(Args.MakeArgString(CandidateRPath.c_str()));
+    }
+  }
 }
 
 /// Add OpenMP linker script arguments at the end of the argument list so that
 /// the fat binary is built by embedding each of the device images into the
 /// host. The linker script also defines a few symbols required by the code
 /// generation so that the images can be easily retrieved at runtime by the
 /// offloading library. This should be used only in tool chains that support
 /// linker scripts.
static void CollectArgsForIntegratedAssembler(Compilation &C,
   if (MipsTargetFeature != nullptr) {
     CmdArgs.push_back("-target-feature");
     CmdArgs.push_back(MipsTargetFeature);
   }
 }
 
 // This adds the static libclang_rt.builtins-arch.a directly to the command line
 // FIXME: Make sure we can also emit shared objects if they're requested
 // and available, check for possible errors, etc.
 static void addClangRT(const ToolChain &TC, const ArgList &Args,
                        ArgStringList &CmdArgs) {
   CmdArgs.push_back(TC.getCompilerRTArgString(Args, "builtins"));
 }
 
 static void addOpenMPRuntime(ArgStringList &CmdArgs, const ToolChain &TC,
                               const ArgList &Args) {
   if (!Args.hasFlag(options::OPT_fopenmp, options::OPT_fopenmp_EQ,
                     options::OPT_fno_openmp, false))
     return;
 
   switch (TC.getDriver().getOpenMPRuntime(Args)) {
   case Driver::OMPRT_OMP:
     CmdArgs.push_back("-lomp");
     break;
   case Driver::OMPRT_GOMP:
     CmdArgs.push_back("-lgomp");
     break;
   case Driver::OMPRT_IOMP5:
     CmdArgs.push_back("-liomp5");
     break;
   case Driver::OMPRT_Unknown:
     // Already diagnosed.
     break;

test/Driver/arch-specific-libdir-rpath.c

+// Test that the driver adds an arch-specific subdirectory in
+// {RESOURCE_DIR}/lib/linux to the linker search path and to '-rpath' for native
+// compilations.
+//
+// -rpath only gets added during native compilation.  To keep the test simple,
+// just test for x86_64-linux native compilation.
+// REQUIRES: x86_64-linux
+//
+// RUN: %clang %s -### 2>&1 \
+// RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-ARCHDIR %s
+//
+// RUN: %clang %s -### 2>&1 \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-ARCHDIR %s
+//
+//
+// CHECK-ARCHDIR: -L{{.*}}/Inputs/resource_dir_with_arch_subdir/lib/linux{{.*}} "-rpath" {{.*}}/Inputs/resource_dir_with_arch_subdir/lib/linux
+// CHECK-NO-ARCHDIR-NOT: -L{{.*}}Inputs/resource_dir
+// CHECK-NO-ARCHDIR-NOT: "-rpath" {{.*}}/Inputs/resource_dir

test/Driver/arch-specific-libdir.c

+// Test that the driver adds an arch-specific subdirectory in
+// {RESOURCE_DIR}/lib/linux to the search path.
+//
+// RUN: %clang %s -### 2>&1 -target i386-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-ARCHDIR-i386 %s
+//
+// RUN: %clang %s -### 2>&1 -target i386-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-ARCHDIR %s
+//
+// RUN: %clang %s -### 2>&1 -target i686-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-ARCHDIR-i386 %s
+//
+// RUN: %clang %s -### 2>&1 -target i686-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-ARCHDIR %s
+//
+// RUN: %clang %s -### 2>&1 -target x86_64-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-ARCHDIR-x86_64 %s
+//
+// RUN: %clang %s -### 2>&1 -target x86_64-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-ARCHDIR %s
+//
+// RUN: %clang %s -### 2>&1 -target arm-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-ARCHDIR-arm %s
+//
+// RUN: %clang %s -### 2>&1 -target arm-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-ARCHDIR %s
+//
+// RUN: %clang %s -### 2>&1 -target aarch64-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-ARCHDIR-aarch64 %s
+//
+// RUN: %clang %s -### 2>&1 -target aarch64-unknown-linux \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-ARCHDIR %s
+//
+//
+// CHECK-ARCHDIR-i386: -L{{.*}}/Inputs/resource_dir_with_arch_subdir/lib/linux/i386
+// CHECK-ARCHDIR-x86_64: -L{{.*}}/Inputs/resource_dir_with_arch_subdir/lib/linux/x86_64
+// CHECK-ARCHDIR-arm: -L{{.*}}/Inputs/resource_dir_with_arch_subdir/lib/linux/arm
+// CHECK-ARCHDIR-aarch64: -L{{.*}}/Inputs/resource_dir_with_arch_subdir/lib/linux/aarch64
+//
+// Have a stricter check for no-archdir - that the driver doesn't add any
+// subdirectory from the provided resource directory.
+// CHECK-NO-ARCHDIR-NOT: -L{{.*}}Inputs/resource_dir

test/lit.cfg

if platform.system() not in ['Darwin', 'Fuchsia']:
     config.available_features.add('libgcc')
 
 # Native compilation: Check if triples match.
 # FIXME: Consider cases that target can be executed
 # even if host_triple were different from target_triple.
 if config.host_triple == config.target_triple:
     config.available_features.add("native")
 
+# Test Driver/arch-specific-libdir-rpath.c is restricted to x86_64-linux
+if re.match(r'^x86_64.*-linux', config.target_triple):
+    config.available_features.add("x86_64-linux")
+
 # Case-insensitive file system
 def is_filesystem_case_insensitive():
     handle, path = tempfile.mkstemp(prefix='case-test', dir=config.test_exec_root)
     isInsensitive = os.path.exists(
         os.path.join(
             os.path.dirname(path),
             os.path.basename(path).upper()
             ))