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

Fri May 7 10:22:50 PDT 2021

protze.joachim added inline comments.

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

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