[PATCH] D142174: [OpenMP] Don't set rpath for system paths

[Tom Stellard via Phabricator via cfe-commits]

tstellar abandoned this revision.
tstellar added a comment.

-fopenmp-add-rpath has been removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142174/new/

https://reviews.llvm.org/D142174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142174: [OpenMP] Don't set rpath for system paths

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D142174#4171731 , @JonChesterfield 
wrote:

> I'm happy with this but agree that "what might be a system path?" is a tricky 
> heuristic. What we want is to exclude the places that the application will 
> search anyway, but that's OS dependent. I'm mostly worried about toolchains 
> installed under /opt or under $HOME so this is good for me. It's also easy to 
> pull later if we reconsider.

Thank you for being considerate the use case. But I don't think we should base 
the decision on whether `/usr/lib` or `/usr/lib64` is used and make directories 
different. We should unconditionally disable rpath, regardless of the directory 
path.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142174/new/

https://reviews.llvm.org/D142174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142174: [OpenMP] Don't set rpath for system paths

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

I'm happy with this but agree that "what might be a system path?" is a tricky 
heuristic. What we want is to exclude the places that the application will 
search anyway, but that's OS dependent. I'm mostly worried about toolchains 
installed under /opt or under $HOME so this is good for me. It's also easy to 
pull later if we reconsider.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142174/new/

https://reviews.llvm.org/D142174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142174: [OpenMP] Don't set rpath for system paths

[Tom Stellard via Phabricator via cfe-commits]

tstellar added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:799
+.Default(false);
+}
+

I wasn't sure exactly how to determine if something is a system path, so I'm 
open to other ideas here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142174/new/

https://reviews.llvm.org/D142174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142174: [OpenMP] Don't set rpath for system paths

[Tom Stellard via Phabricator via cfe-commits]

tstellar created this revision.
tstellar added reviewers: JonChesterfield, MaskRay.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
tstellar requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

Suggested in https://reviews.llvm.org/D118493#3402001


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142174

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -791,6 +791,13 @@
  /*IsLTO=*/true, PluginOptPrefix);
 }
 
+static bool isSystemPath(const StringRef ) {
+  return llvm::StringSwitch(path)
+.Case("/usr/lib", true)
+.Case("/usr/lib64", true)
+.Default(false);
+}
+
 void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
   const ArgList ,
   ArgStringList ) {
@@ -802,6 +809,11 @@
 SmallString<256> DefaultLibPath =
 llvm::sys::path::parent_path(TC.getDriver().Dir);
 llvm::sys::path::append(DefaultLibPath, CLANG_INSTALL_LIBDIR_BASENAME);
+
+// If clang is installed to a 'system path' there is no reason to add 
rpath.
+if (isSystemPath(DefaultLibPath))
+  return;
+
 CmdArgs.push_back("-rpath");
 CmdArgs.push_back(Args.MakeArgString(DefaultLibPath));
   }


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -791,6 +791,13 @@
  /*IsLTO=*/true, PluginOptPrefix);
 }
 
+static bool isSystemPath(const StringRef ) {
+  return llvm::StringSwitch(path)
+.Case("/usr/lib", true)
+.Case("/usr/lib64", true)
+.Default(false);
+}
+
 void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
   const ArgList ,
   ArgStringList ) {
@@ -802,6 +809,11 @@
 SmallString<256> DefaultLibPath =
 llvm::sys::path::parent_path(TC.getDriver().Dir);
 llvm::sys::path::append(DefaultLibPath, CLANG_INSTALL_LIBDIR_BASENAME);
+
+// If clang is installed to a 'system path' there is no reason to add rpath.
+if (isSystemPath(DefaultLibPath))
+  return;
+
 CmdArgs.push_back("-rpath");
 CmdArgs.push_back(Args.MakeArgString(DefaultLibPath));
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits