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

Mon Aug 23 11:43:11 PDT 2021

JonChesterfield added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648

+void tools::addOpenMPRuntimeSpecificRPath(const ToolChain &TC,
+                                          const ArgList &Args,
----------------
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