[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Ye Luo via Phabricator via cfe-commits]

ye-luo added a comment.

In D101960#2961133 , @jdoerfert wrote:

> There are 3 problems here (ignoring our test setup which should be discussed 
> separately):
>
> 1. make sure clang finds libomp.so
> 2. make sure libomp.so (or clang?) finds libomptarget.so
> 3. make sure libomptartget.so finds the plugins
>
> All of which have to work nicely with LD_LIBRARY_PATH.
>
> I think for 3 you can look at the current dir. That was discussed and, as 
> long as it does work with LD_LIBRARY_PATH, that seems a win.
>
> For 2, why don't we install them in the same place? If so, we reduce it to 
> problem 1) [after applying solution to 3)] no?
>
> For 1, doing something always backwards compatible that may or may not work 
> on some platforms and configurations seems best. You had a proposal here 
> already. If that works, let's do it.

For 1. If users prefer linking an alternative libomp.so, they should not use 
-fopenmp at linking and link libomp.so explicitly as a regular library. If 
users add -fopenmp to clang at linking, clang needs to link the libomp.so 
shipped with clang and set rpath to ensure libomp.so can be found at run even 
libomp.so doesn't exist on LD_LIBRARY_PATH. In this way, if a user would like 
to switch to anther libomp.so, they can still specify LD_LIBRARY_PATH.
For 2. Is libomp.so aware of libomptarget.so? I doubt. Anyway I'd like to see a 
similar logic as case 1 and the control option is -fopenmp-targets.

So addOpenMPRuntimeSpecificRPath seems aligned with what I described.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Test setup can be fixed independently (and possibly should be).

D102043  is newly simplified. It looks for 
plugins next to libomptarget.so, which means it can find them even when the 
application uses a non-transitive method to find libomptarget.so.

Turns out libomptarget.so is linked directly to the application, not via 
libomp.so as I believed, and they're installed next to each other so whatever 
finds the first will find the second.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

There are 3 problems here (ignoring our test setup which should be discussed 
separately):

1. make sure clang finds libomp.so
2. make sure libomp.so (or clang?) finds libomptarget.so
3. make sure libomptartget.so finds the plugins

All of which have to work nicely with LD_LIBRARY_PATH.

I think for 3 you can look at the current dir. That was discussed and, as long 
as it does work with LD_LIBRARY_PATH, that seems a win.

For 2, why don't we install them in the same place? If so, we reduce it to 
problem 1) [after applying solution to 3)] no?

For 1, doing something always backwards compatible that may or may not work on 
some platforms and configurations seems best. You had a proposal here already. 
If that works, let's do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D101960#2960846 , @protze.joachim 
wrote:

> The lit config has platform-specific rules to build the environmental 
> variables (including the use of the proper separators). I suggest to used 
> these values to create the printed env expressions.

Yep. That seems an unconditional win, however the rpath/runpath stuff works out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

In D101960#2960641 , @JonChesterfield 
wrote:

> Pasting `env LD_LIBRARY_PATH=` and `env LIBRARY_PATH` into the printed 
> commandline, as opposed to through magic, would solve most of my day to day 
> frustration here. libomp.so and libomptarget.so are in two different 
> directories, LD_LIBRARY_PATH turns out to be colon delimited. LIBRARY_PATH is 
> presently used to find the deviceRTL though I'd like to change that.

The lit config has platform-specific rules to build the environmental variables 
(including the use of the proper separators). I suggest to used these values to 
create the printed env expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D101960#2960657 , @jdoerfert wrote:

> Do I understand correctly that adding runpath to libomp.so will help it find 
> libomptarget.so *and* still allows users to use LD_LIBRARY_PATH to make sure 
> a different libomptarget.so is found?
>
> If the above is the case, can't we do the same for clang?
>
> Asked differently, how does clang find things like compiler-rt?

If we set runpath on libomp.so it'll be able to find libomptarget.so without 
environment variables. If the user sets LD_LIBRARY_PATH, a libomptarget.so on 
that path will be used instead.

Clang can definitely set rpath or runpath on the user binary, that's what 
addOpenMPRuntimeSpecificRPath does. That way no environment variables are 
needed. The set of options are then:

- User has otherwise set nothing on the binary, either rpath or runpath works 
great
- User has set runpath and we set runpath, linker should put both in. Not sure 
which will go first. Probably fine
- User has set rpath and we set runpath. Linker will ignore the user rpath, so 
we've probably broken the binary
- User has set runpath and we set rpath. Harmless, but we'll be ignored

compiler-rt is different because it's statically linked into every binary and 
every shared library, and is carefully written so that it doesn't matter if the 
final running system has loads of copies of it as a result. If we statically 
linked our libraries none of the above would be an issue but developers (and I 
suppose users) would no longer be able to pick and choose at runtime.

There might be a linker flag (which might even exist on all the linkers users 
might use) to control the behaviour where any runpath means ignore all rpath. 
What we'd really like is to set r(un)path on the user binary unless something 
else is already doing so, but I don't think we can have that information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Do I understand correctly that adding runpath to libomp.so will help it find 
libomptarget.so *and* still allows users to use LD_LIBRARY_PATH to make sure a 
different libomptarget.so is found?

If the above is the case, can't we do the same for clang?

Asked differently, how does clang find things like compiler-rt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
+  const ArgList ,

JonChesterfield wrote:
> protze.joachim wrote:
> > JonChesterfield wrote:
> > > lebedev.ri wrote:
> > > > JonChesterfield wrote:
> > > > > Similar to other functions in this file, derived from aomp (by 
> > > > > deleting some stuff for finding a debug version of the library)
> > > > > 
> > > > > I can make my peace with runpath instead if people are keen to 
> > > > > override this with LD_LIBRARY_PATH. The rest of clang's toolchains 
> > > > > wire things up with rpath but that doesn't mean we have to make the 
> > > > > same choice here.
> > > > I think it would be a shame if this would be the only thing
> > > > that *doesn't* support being overriden with `LD_LIBRARY_PATH`.
> > > > I'm not sure about `libomptarget`, but it i think it would be good to 
> > > > keep such possibility for `libomp`.
> > > The search order is 'rpath' then 'LD_LIBRARY_PATH' then 'runpath'. 
> > > Presently we set no default at all and require the user to set 
> > > LD_LIBRARY_PATH or manually link the right library. So using runpath here 
> > > is backwards compatible, in that all the scripts out there that use 
> > > LD_LIBRARY_PATH will continue to work. That may force our hand here.
> > Especially for debugging, I like the ability to exchange the OpenMP 
> > runtimes by adding the debugging build of the runtimes to LD_LIBRARY_PATH 
> > without needing to relink the application, so I'd also prefer runpath.
> > 
> > 
> > In the manpage of ld I found: 
> > > For a native ELF linker, the directories in "DT_RUNPATH" or "DT_RPATH" of 
> > > a shared library are searched for shared libraries needed by it. The 
> > > "DT_RPATH" entries are ignored if "DT_RUNPATH" entries exist.
> > 
> > Does this mean, that adding a runpath here will lead to ignoring the other 
> > rpath entries? Or does this only affect linking shared libraries and not 
> > linking an application?
> > 
> The internet also claims rpath is now deprecated, which I suppose is 
> consistent with ignoring rpath when both are present. So it seems likely that 
> setting runpath here will clobber any (possibly deprecated) rpath that is 
> added elsewhere.
> 
> I'm not sure what the right path is from there. People might set rpath on 
> openmp executables, and we shouldn't clobber that if they do. There may be a 
> linker flag along the lines of 'set runpath unless there is already an rpath 
> requested'.
> 
> I think it's a bug in the linker to ignore one when the other is present but 
> backwards compatibility will render that a feature.
> 
> There is a halfway step where we set rpath (or runpath) on libomp.so so that 
> it can find libomptarget, as we own the libomp.so that is being built at the 
> time, but that doesn't really solve the problem.
> 
> I'd note that compiler-rt is always statically linked, which seems a good 
> idea for a compiler runtime, but would totally thwart the desire to replace 
> it with some other runtime without relinking.
> 
> We could embed rpath, which gets definitely working out of the box behaviour, 
> with a compiler flag to leave that off for development builds that want 
> dynamic control over the pieces. Where I see that rpath is 'deprecated', but 
> also that the runpath behaviour of clobbering an unrelated rpath makes it 
> unusable in this context.
Mutating the binary to let it find libomp.so (like this, or with runpath) is 
convenient for users who want that and very inconvenient for those who are 
using rpath for some other purpose. What we could do is change the build script 
for libomp.so to add a relative runpath to let it find libomptarget.so. That 
makes the test infra slightly simpler.

It also means a user only has to arrange for their binary to find libomp.so on 
their own without worrying about the other pieces, so doesn't necessarily have 
to use LD_LIBRARY_PATH to do it.



Comment at: openmp/libomptarget/test/lit.cfg:182
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + 
config.cuda_path))

JonChesterfield wrote:
> protze.joachim wrote:
> > This would completely avoid the --cuda-path flag for non-nvptx tests
> Yes. That seems like a good thing. Is there a reason we want to pass 
> `--cuda-path=CUDA_TOOLKIT_ROOT_DIR-NOTFOUND` to all the non-nvptx tests?
This should disappear under a rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Pasting `env LD_LIBRARY_PATH=` and `env LIBRARY_PATH` into the printed 
commandline, as opposed to through magic, would solve most of my day to day 
frustration here. libomp.so and libomptarget.so are in two different 
directories, LD_LIBRARY_PATH turns out to be colon delimited. LIBRARY_PATH is 
presently used to find the deviceRTL though I'd like to change that.

That doesn't solve any of the experience for our users but does help with 
debugging the tests. I'm currently trying to work out why the set of tests that 
fail under lit are different to the set that fail outside of lit and decreasing 
magic there might help me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D101960#2960622 , @jdoerfert wrote:

> To summarize the conversation, can we do LD_LIBRARY_PATH overwrites after 
> this patch or not? If so, I feel everyone is in favor, if not, we need a 
> different solution.

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

To summarize the conversation, can we do LD_LIBRARY_PATH overwrites after this 
patch or not? If so, I feel everyone is in favor, if not, we need a different 
solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

To make debugging of failed tests easier, we could just explicitly include `env 
LD_LIBRARY_PATH=...` into each run line - by adding it to the `%libomp-run` 
substitution (and the libomptarget equivalent).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Just ran into this again. It's really annoying that a test fails, and prints a 
run line, which can be copied into a terminal where it abjectly fails to run 
because the environment variables aren't set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
+  const ArgList ,

protze.joachim wrote:
> JonChesterfield wrote:
> > lebedev.ri wrote:
> > > JonChesterfield wrote:
> > > > Similar to other functions in this file, derived from aomp (by deleting 
> > > > some stuff for finding a debug version of the library)
> > > > 
> > > > I can make my peace with runpath instead if people are keen to override 
> > > > this with LD_LIBRARY_PATH. The rest of clang's toolchains wire things 
> > > > up with rpath but that doesn't mean we have to make the same choice 
> > > > here.
> > > I think it would be a shame if this would be the only thing
> > > that *doesn't* support being overriden with `LD_LIBRARY_PATH`.
> > > I'm not sure about `libomptarget`, but it i think it would be good to 
> > > keep such possibility for `libomp`.
> > The search order is 'rpath' then 'LD_LIBRARY_PATH' then 'runpath'. 
> > Presently we set no default at all and require the user to set 
> > LD_LIBRARY_PATH or manually link the right library. So using runpath here 
> > is backwards compatible, in that all the scripts out there that use 
> > LD_LIBRARY_PATH will continue to work. That may force our hand here.
> Especially for debugging, I like the ability to exchange the OpenMP runtimes 
> by adding the debugging build of the runtimes to LD_LIBRARY_PATH without 
> needing to relink the application, so I'd also prefer runpath.
> 
> 
> In the manpage of ld I found: 
> > For a native ELF linker, the directories in "DT_RUNPATH" or "DT_RPATH" of a 
> > shared library are searched for shared libraries needed by it. The 
> > "DT_RPATH" entries are ignored if "DT_RUNPATH" entries exist.
> 
> Does this mean, that adding a runpath here will lead to ignoring the other 
> rpath entries? Or does this only affect linking shared libraries and not 
> linking an application?
> 
The internet also claims rpath is now deprecated, which I suppose is consistent 
with ignoring rpath when both are present. So it seems likely that setting 
runpath here will clobber any (possibly deprecated) rpath that is added 
elsewhere.

I'm not sure what the right path is from there. People might set rpath on 
openmp executables, and we shouldn't clobber that if they do. There may be a 
linker flag along the lines of 'set runpath unless there is already an rpath 
requested'.

I think it's a bug in the linker to ignore one when the other is present but 
backwards compatibility will render that a feature.

There is a halfway step where we set rpath (or runpath) on libomp.so so that it 
can find libomptarget, as we own the libomp.so that is being built at the time, 
but that doesn't really solve the problem.

I'd note that compiler-rt is always statically linked, which seems a good idea 
for a compiler runtime, but would totally thwart the desire to replace it with 
some other runtime without relinking.

We could embed rpath, which gets definitely working out of the box behaviour, 
with a compiler flag to leave that off for development builds that want dynamic 
control over the pieces. Where I see that rpath is 'deprecated', but also that 
the runpath behaviour of clobbering an unrelated rpath makes it unusable in 
this context.



Comment at: openmp/libomptarget/test/lit.cfg:182
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + 
config.cuda_path))

protze.joachim wrote:
> This would completely avoid the --cuda-path flag for non-nvptx tests
Yes. That seems like a good thing. Is there a reason we want to pass 
`--cuda-path=CUDA_TOOLKIT_ROOT_DIR-NOTFOUND` to all the non-nvptx tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
+  const ArgList ,

JonChesterfield wrote:
> lebedev.ri wrote:
> > JonChesterfield wrote:
> > > Similar to other functions in this file, derived from aomp (by deleting 
> > > some stuff for finding a debug version of the library)
> > > 
> > > I can make my peace with runpath instead if people are keen to override 
> > > this with LD_LIBRARY_PATH. The rest of clang's toolchains wire things up 
> > > with rpath but that doesn't mean we have to make the same choice here.
> > I think it would be a shame if this would be the only thing
> > that *doesn't* support being overriden with `LD_LIBRARY_PATH`.
> > I'm not sure about `libomptarget`, but it i think it would be good to keep 
> > such possibility for `libomp`.
> The search order is 'rpath' then 'LD_LIBRARY_PATH' then 'runpath'. Presently 
> we set no default at all and require the user to set LD_LIBRARY_PATH or 
> manually link the right library. So using runpath here is backwards 
> compatible, in that all the scripts out there that use LD_LIBRARY_PATH will 
> continue to work. That may force our hand here.
Especially for debugging, I like the ability to exchange the OpenMP runtimes by 
adding the debugging build of the runtimes to LD_LIBRARY_PATH without needing 
to relink the application, so I'd also prefer runpath.


In the manpage of ld I found: 
> For a native ELF linker, the directories in "DT_RUNPATH" or "DT_RPATH" of a 
> shared library are searched for shared libraries needed by it. The "DT_RPATH" 
> entries are ignored if "DT_RUNPATH" entries exist.

Does this mean, that adding a runpath here will lead to ignoring the other 
rpath entries? Or does this only affect linking shared libraries and not 
linking an application?




Comment at: openmp/libomptarget/test/lit.cfg:182
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + 
config.cuda_path))

This would completely avoid the --cuda-path flag for non-nvptx tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: openmp/libomptarget/src/rtl.cpp:76
 
+  std::string full_plugin_name;
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);

JonChesterfield wrote:
> JonChesterfield wrote:
> > This logic is cut from D73657 without addressing any of the review 
> > comments. Idea is to look for the offloading plugins next to libomptarget, 
> > instead of in dlopen's default search path. Will address the previous 
> > comments when splitting out to a separate patch.
> There's a D87413 that I didn't know about before seeing a comment on D73657 
> just now. That uses RTLD_DI_LINKMAP with dlinfo instead. I'm leaning towards 
> dladdr because it seems to exist on more platforms but haven't looked into 
> the options closely yet.
This part is factored out as D102043


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield updated this revision to Diff 343557.
JonChesterfield added a comment.

- rework logic for finding libomptarget.so


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  openmp/libomptarget/src/rtl.cpp
  openmp/libomptarget/test/lit.cfg

Index: openmp/libomptarget/test/lit.cfg
===
--- openmp/libomptarget/test/lit.cfg
+++ openmp/libomptarget/test/lit.cfg
@@ -70,14 +70,10 @@
 config.test_flags += " -Wl,-rpath," + config.library_dir
 config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
 else: # Unices
-append_dynamic_library_path('LD_LIBRARY_PATH', config.library_dir, ":")
-append_dynamic_library_path('LD_LIBRARY_PATH', \
-config.omp_host_rtl_directory, ":")
+config.test_flags += " -Wl,-rpath," + config.library_dir
+config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
 if config.cuda_libdir:
-append_dynamic_library_path('LD_LIBRARY_PATH', config.cuda_libdir, ":")
-append_dynamic_library_path('LIBRARY_PATH', config.library_dir, ":")
-append_dynamic_library_path('LIBRARY_PATH', \
-config.omp_host_rtl_directory, ":")
+config.test_flags += " -Wl,-rpath," + config.cuda_libdir
 
 # substitutions
 # - for targets that exist in the system create the actual command.
@@ -182,7 +178,8 @@
 config.substitutions.append(("%clangxx", config.test_cxx_compiler))
 config.substitutions.append(("%clang", config.test_c_compiler))
 config.substitutions.append(("%openmp_flags", config.test_openmp_flags))
-if config.cuda_path:
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + config.cuda_path))
 else:
   config.substitutions.append(("%cuda_flags", ""))
Index: openmp/libomptarget/src/rtl.cpp
===
--- openmp/libomptarget/src/rtl.cpp
+++ openmp/libomptarget/src/rtl.cpp
@@ -65,6 +65,30 @@
 #endif
 }
 
+namespace {
+std::string findLibomptargetDirectory() {
+  Dl_info dl_info;
+  // look up a symbol which is known to be from libomptarget
+  if (dladdr((void *)&__tgt_register_lib, _info) != 0) {
+std::string libomptargetPath = std::string(dl_info.dli_fname);
+size_t slash = libomptargetPath.find_last_of('/');
+if (slash != std::string::npos) {
+  return libomptargetPath.substr(0, slash + 1); // keep the /
+}
+  }
+  return "";
+}
+
+void *verbose_dlopen(const char *Name) {
+  DP("Loading library '%s'...\n", Name);
+  void *dynlib_handle = dlopen(Name, RTLD_NOW);
+  if (!dynlib_handle) {
+DP("Unable to load library '%s': %s!\n", Name, dlerror());
+  }
+  return dynlib_handle;
+}
+} // namespace
+
 void RTLsTy::LoadRTLs() {
   // Parse environment variable OMP_TARGET_OFFLOAD (if set)
   PM->TargetOffloadPolicy =
@@ -72,19 +96,22 @@
   if (PM->TargetOffloadPolicy == tgt_disabled) {
 return;
   }
-
   DP("Loading RTLs...\n");
+  std::string libomptargetPath = findLibomptargetDirectory();
 
   // Attempt to open all the plugins and, if they exist, check if the interface
   // is correct and if they are supporting any devices.
   for (auto *Name : RTLNames) {
-DP("Loading library '%s'...\n", Name);
-void *dynlib_handle = dlopen(Name, RTLD_NOW);
+std::string adjacentPluginName = libomptargetPath + std::string(Name);
 
+void *dynlib_handle = verbose_dlopen(adjacentPluginName.c_str());
 if (!dynlib_handle) {
-  // Library does not exist or cannot be found.
-  DP("Unable to load library '%s': %s!\n", Name, dlerror());
-  continue;
+  // Try again without the libomptarget library path prefix
+  dynlib_handle = verbose_dlopen(Name);
+  if (!dynlib_handle) {
+// Library does not exist or cannot be found.
+continue;
+  }
 }
 
 DP("Successfully loaded library '%s'!\n", Name);
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -73,6 +73,10 @@
   const llvm::opt::ArgList ,
   llvm::opt::ArgStringList );
 
+void addOpenMPRuntimeSpecificRPath(const ToolChain ,
+   const llvm::opt::ArgList ,
+   llvm::opt::ArgStringList );
+
 void addArchSpecificRPath(const ToolChain , const llvm::opt::ArgList ,
   llvm::opt::ArgStringList );
 /// Returns true, if an OpenMP runtime has been added.
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ 

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Yes. Libomp and libomptarget are in the same directory, so the rpath/runpath 
change catches both of them. Libomptarget then needs to find the plugins to 
load, either through library path or something equivalent to the above.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
+  const ArgList ,

lebedev.ri wrote:
> JonChesterfield wrote:
> > Similar to other functions in this file, derived from aomp (by deleting 
> > some stuff for finding a debug version of the library)
> > 
> > I can make my peace with runpath instead if people are keen to override 
> > this with LD_LIBRARY_PATH. The rest of clang's toolchains wire things up 
> > with rpath but that doesn't mean we have to make the same choice here.
> I think it would be a shame if this would be the only thing
> that *doesn't* support being overriden with `LD_LIBRARY_PATH`.
> I'm not sure about `libomptarget`, but it i think it would be good to keep 
> such possibility for `libomp`.
The search order is 'rpath' then 'LD_LIBRARY_PATH' then 'runpath'. Presently we 
set no default at all and require the user to set LD_LIBRARY_PATH or manually 
link the right library. So using runpath here is backwards compatible, in that 
all the scripts out there that use LD_LIBRARY_PATH will continue to work. That 
may force our hand here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for looking into this!
This also fixes the problem of not being able to find `libomp`, right?




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
+  const ArgList ,

JonChesterfield wrote:
> Similar to other functions in this file, derived from aomp (by deleting some 
> stuff for finding a debug version of the library)
> 
> I can make my peace with runpath instead if people are keen to override this 
> with LD_LIBRARY_PATH. The rest of clang's toolchains wire things up with 
> rpath but that doesn't mean we have to make the same choice here.
I think it would be a shame if this would be the only thing
that *doesn't* support being overriden with `LD_LIBRARY_PATH`.
I'm not sure about `libomptarget`, but it i think it would be good to keep such 
possibility for `libomp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: openmp/libomptarget/src/rtl.cpp:76
 
+  std::string full_plugin_name;
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);

JonChesterfield wrote:
> This logic is cut from D73657 without addressing any of the review comments. 
> Idea is to look for the offloading plugins next to libomptarget, instead of 
> in dlopen's default search path. Will address the previous comments when 
> splitting out to a separate patch.
There's a D87413 that I didn't know about before seeing a comment on D73657 
just now. That uses RTLD_DI_LINKMAP with dlinfo instead. I'm leaning towards 
dladdr because it seems to exist on more platforms but haven't looked into the 
options closely yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648
 
+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain ,
+  const ArgList ,

Similar to other functions in this file, derived from aomp (by deleting some 
stuff for finding a debug version of the library)

I can make my peace with runpath instead if people are keen to override this 
with LD_LIBRARY_PATH. The rest of clang's toolchains wire things up with rpath 
but that doesn't mean we have to make the same choice here.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1703
 
+  // Add path to runtimes binary folder, used by ENABLE_RUNTIMES build
+  SmallString<256> RuntimesBinPath = llvm::sys::path::parent_path(D.Dir);

Otherwise we need LIBRARY_PATH to run from the build tree, which is awkward for 
tests and for ad hoc running from the build tree. This path is checked last, so 
only changes behaviour from error message to success when the file exists here. 
Split out in D101935



Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:17
 
+set_target_properties(amdgpu-arch PROPERTIES INSTALL_RPATH_USE_LINK_PATH ON)
+

Lets amdgpu-arch work from the build tree, split out in D101926



Comment at: openmp/libomptarget/src/rtl.cpp:76
 
+  std::string full_plugin_name;
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);

This logic is cut from D73657 without addressing any of the review comments. 
Idea is to look for the offloading plugins next to libomptarget, instead of in 
dlopen's default search path. Will address the previous comments when splitting 
out to a separate patch.



Comment at: openmp/libomptarget/test/lit.cfg:72
 config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
 else: # Unices
+config.test_flags += " -Wl,-rpath," + config.library_dir

For copy & paste of a RUN line from a failing test



Comment at: openmp/libomptarget/test/lit.cfg:182
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + 
config.cuda_path))

Because otherwise we write `--cuda-path=CUDA_TOOLKIT_ROOT_DIR-NOTFOUND` at the 
top of non-nvptx tests, which is harmless but ugly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101960

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

[PATCH] D101960: [openmp] Drop requirement on library path environment variables

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield created this revision.
JonChesterfield added reviewers: jdoerfert, grokos, ABataev, gregrodgers, 
RaviNarayanaswamy, pdhaliwal, hfinkel, tlwilmar, AndreyChurbanov, jlpeyton, 
protze.joachim, ye-luo, tianshilei1992.
Herald added subscribers: kerbowa, guansong, yaxunl, mgorny, nhaehnle, jvesely.
JonChesterfield requested review of this revision.
Herald added subscribers: openmp-commits, cfe-commits, sstefan1.
Herald added projects: clang, OpenMP.

[openmp] Drop requirement on library path environment variables

Involves multiple independent changes, intent is to land one piece at a time.
This diff provides a big picture of one way to avoid needing to specify
LD_LIBRARY_PATH and/or LIBRARY_PATH in order to run any openmp offloading code.
Specific details of the implementation are not necessarily interesting - e.g.
dlinfo appears to be impossible to use safely, so will drop that.

This diff (and associated openmp-dev email to be written shortly) is to start
a discussion on whether requiring users to set LD_LIBRARY_PATH in order to run
any openmp application is what we want.

Reviewers are a guess at interested parties, feel free to add anyone else.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101960

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/tools/amdgpu-arch/CMakeLists.txt
  openmp/libomptarget/src/rtl.cpp
  openmp/libomptarget/test/lit.cfg

Index: openmp/libomptarget/test/lit.cfg
===
--- openmp/libomptarget/test/lit.cfg
+++ openmp/libomptarget/test/lit.cfg
@@ -70,14 +70,10 @@
 config.test_flags += " -Wl,-rpath," + config.library_dir
 config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
 else: # Unices
-append_dynamic_library_path('LD_LIBRARY_PATH', config.library_dir, ":")
-append_dynamic_library_path('LD_LIBRARY_PATH', \
-config.omp_host_rtl_directory, ":")
+config.test_flags += " -Wl,-rpath," + config.library_dir
+config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
 if config.cuda_libdir:
-append_dynamic_library_path('LD_LIBRARY_PATH', config.cuda_libdir, ":")
-append_dynamic_library_path('LIBRARY_PATH', config.library_dir, ":")
-append_dynamic_library_path('LIBRARY_PATH', \
-config.omp_host_rtl_directory, ":")
+config.test_flags += " -Wl,-rpath," + config.cuda_libdir
 
 # substitutions
 # - for targets that exist in the system create the actual command.
@@ -182,7 +178,8 @@
 config.substitutions.append(("%clangxx", config.test_cxx_compiler))
 config.substitutions.append(("%clang", config.test_c_compiler))
 config.substitutions.append(("%openmp_flags", config.test_openmp_flags))
-if config.cuda_path:
+
+if config.cuda_path and config.cuda_path != 'CUDA_TOOLKIT_ROOT_DIR-NOTFOUND':
   config.substitutions.append(("%cuda_flags", "--cuda-path=" + config.cuda_path))
 else:
   config.substitutions.append(("%cuda_flags", ""))
Index: openmp/libomptarget/src/rtl.cpp
===
--- openmp/libomptarget/src/rtl.cpp
+++ openmp/libomptarget/src/rtl.cpp
@@ -73,13 +73,24 @@
 return;
   }
 
+  std::string full_plugin_name;
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);
+  if (!handle)
+DP("dlopen() failed: %s\n", dlerror());
+  char *libomptarget_dir_name = new char[256];
+  if (dlinfo(handle, RTLD_DI_ORIGIN, libomptarget_dir_name) == -1)
+DP("RTLD_DI_ORIGIN failed: %s\n", dlerror());
+
   DP("Loading RTLs...\n");
 
   // Attempt to open all the plugins and, if they exist, check if the interface
   // is correct and if they are supporting any devices.
   for (auto *Name : RTLNames) {
 DP("Loading library '%s'...\n", Name);
-void *dynlib_handle = dlopen(Name, RTLD_NOW);
+
+full_plugin_name.assign(libomptarget_dir_name).append("/").append(Name);
+DP("Loading library '%s'...\n", full_plugin_name.c_str());
+void *dynlib_handle = dlopen(full_plugin_name.c_str(), RTLD_NOW);
 
 if (!dynlib_handle) {
   // Library does not exist or cannot be found.
Index: clang/tools/amdgpu-arch/CMakeLists.txt
===
--- clang/tools/amdgpu-arch/CMakeLists.txt
+++ clang/tools/amdgpu-arch/CMakeLists.txt
@@ -14,4 +14,6 @@
 
 add_clang_tool(amdgpu-arch AMDGPUArch.cpp)
 
+set_target_properties(amdgpu-arch PROPERTIES INSTALL_RPATH_USE_LINK_PATH ON)
+
 clang_target_link_libraries(amdgpu-arch PRIVATE hsa-runtime64::hsa-runtime64)
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -73,6 +73,10 @@
   const llvm::opt::ArgList ,
   llvm::opt::ArgStringList );
 
+void addOpenMPRuntimeSpecificRPath(const ToolChain ,
+