Page MenuHomePhabricator

Add arch-specific directory to search path
ClosedPublic

Authored by pirama on Feb 15 2017, 5:22 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

pirama created this revision.Feb 15 2017, 5:22 PM
danalbert accepted this revision.Feb 15 2017, 5:44 PM

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

This revision is now accepted and ready to land.Feb 15 2017, 5:44 PM
srhines added inline comments.Feb 15 2017, 6:38 PM
include/clang/Driver/ToolChain.h
304 ↗(On Diff #88631)

Add a period at the end of the sentence.

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

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).

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 updated this revision to Diff 88662.Feb 16 2017, 12:02 AM

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

mgorny added inline comments.Feb 16 2017, 12:03 AM
lib/Driver/ToolChain.cpp
327 ↗(On Diff #88631)

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 added inline comments.Feb 16 2017, 12:04 AM
lib/Driver/Tools.cpp
3267 ↗(On Diff #88662)

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

pirama updated this revision to Diff 88663.Feb 16 2017, 12:14 AM

Use getArchTypeName() instead of getArchName().

pirama marked an inline comment as done.Feb 16 2017, 12:33 AM
pirama added a subscriber: openmp-commits.
pirama added inline comments.
lib/Driver/Tools.cpp
3267 ↗(On Diff #88662)

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.

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...)

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 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.

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 updated this revision to Diff 88768.Feb 16 2017, 12:47 PM
  • Arch-subdir is now always added to -L
  • It is added to -rpath during native compilation.
  • Tests have been updated
pirama added inline comments.Feb 16 2017, 12:52 PM
test/Driver/arch-specific-libdir-rpath.c
6 ↗(On Diff #88768)

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 added inline comments.Feb 16 2017, 2:14 PM
test/Driver/arch-specific-libdir.c
6 ↗(On Diff #88768)

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 updated this revision to Diff 88799.Feb 16 2017, 4:08 PM

Stricter tests.

pirama added inline comments.Feb 16 2017, 4:10 PM
test/Driver/arch-specific-libdir-rpath.c
6 ↗(On Diff #88768)

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

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

lib/Driver/Tools.cpp
284 ↗(On Diff #88799)

s/lined/linked/

test/Driver/arch-specific-libdir-rpath.c
6 ↗(On Diff #88768)

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

18 ↗(On Diff #88799)

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

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
6 ↗(On Diff #88768)

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 added inline comments.Feb 16 2017, 11:44 PM
test/Driver/arch-specific-libdir-rpath.c
6 ↗(On Diff #88768)

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 updated this revision to Diff 88864.Feb 17 2017, 1:36 AM

Fix typo.

pirama retitled this revision from [OpenMP] Add arch-specific directory to search path to Add arch-specific directory to search path.Feb 17 2017, 1:37 AM
pirama updated this revision to Diff 88865.Feb 17 2017, 1:49 AM
pirama marked an inline comment as done.

Add arch name to -rpath test.

mgorny added inline comments.Feb 17 2017, 2:12 AM
lib/Driver/Tools.cpp
3267 ↗(On Diff #88662)

@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 added inline comments.Feb 17 2017, 9:02 AM
test/Driver/arch-specific-libdir-rpath.c
18 ↗(On Diff #88799)

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/...

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 accepted this revision.Feb 17 2017, 2:39 PM

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 ↗(On Diff #88799)

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 added inline comments.Feb 17 2017, 2:42 PM
test/Driver/arch-specific-libdir-rpath.c
18 ↗(On Diff #88799)

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 added a reviewer: rnk.Feb 17 2017, 2:45 PM
pirama added a subscriber: rnk.
rnk added inline comments.Mar 2 2017, 9:05 AM
include/clang/Driver/ToolChain.h
305 ↗(On Diff #88865)

Why const qualify the std::string?

lib/Driver/Tools.cpp
289 ↗(On Diff #88865)

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 updated this revision to Diff 90373.Mar 2 2017, 12:38 PM

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

pirama edited the summary of this revision. (Show Details)Mar 2 2017, 12:39 PM
pirama edited the summary of this revision. (Show Details)
pirama marked 2 inline comments as done.
pirama added inline comments.
lib/Driver/Tools.cpp
289 ↗(On Diff #88865)

PTAL.

test/Driver/arch-specific-libdir-rpath.c
18 ↗(On Diff #88799)

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

rnk accepted this revision.Mar 3 2017, 1:41 PM

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

lib/Driver/Tools.cpp
2007–2009 ↗(On Diff #90373)

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
9 ↗(On Diff #90373)

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 updated this revision to Diff 90532.Mar 3 2017, 1:56 PM
pirama edited the summary of this revision. (Show Details)

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

pirama marked an inline comment as done.Mar 3 2017, 2:20 PM
pirama added inline comments.
lib/Driver/Tools.cpp
2007–2009 ↗(On Diff #90373)

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.

This revision was automatically updated to reflect the committed changes.
pirama added a comment.Mar 3 2017, 3:36 PM

Committed. Thanks for the reviews!