Page MenuHomePhabricator

[openmp] Drop requirement on library path environment variables
Needs ReviewPublic

Authored by JonChesterfield on May 5 2021, 4:37 PM.

Details

Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
820 msx64 debian > Clang.Driver::amdgpu-openmp-toolchain.c
Script: -- : 'RUN: at line 3'; env LIBRARY_PATH=/mnt/disks/ssd0/agent/llvm-project/clang/test/Driver/Inputs/hip_dev_lib /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -### --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 /mnt/disks/ssd0/agent/llvm-project/clang/test/Driver/amdgpu-openmp-toolchain.c 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/Driver/amdgpu-openmp-toolchain.c
1,900 msx64 windows > Clang.Driver::amdgpu-openmp-toolchain.c
Script: -- : 'RUN: at line 3'; env LIBRARY_PATH=C:\ws\w16-1\llvm-project\premerge-checks\clang\test\Driver/Inputs/hip_dev_lib c:\ws\w16-1\llvm-project\premerge-checks\build\bin\clang.exe -### --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 C:\ws\w16-1\llvm-project\premerge-checks\clang\test\Driver\amdgpu-openmp-toolchain.c 2>&1 | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16-1\llvm-project\premerge-checks\clang\test\Driver\amdgpu-openmp-toolchain.c

Event Timeline

JonChesterfield created this revision.May 5 2021, 4:37 PM
JonChesterfield requested review of this revision.May 5 2021, 4:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 5 2021, 4:37 PM
JonChesterfield added inline comments.May 5 2021, 5:03 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
648

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.

1703

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

clang/tools/amdgpu-arch/CMakeLists.txt
17 ↗(On Diff #343235)

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

openmp/libomptarget/src/rtl.cpp
99

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.

openmp/libomptarget/test/lit.cfg
72–73

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

182

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

JonChesterfield added inline comments.May 5 2021, 5:10 PM
openmp/libomptarget/src/rtl.cpp
99

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.

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

clang/lib/Driver/ToolChains/CommonArgs.cpp
648

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.

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.

clang/lib/Driver/ToolChains/CommonArgs.cpp
648

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.

  • rework logic for finding libomptarget.so
JonChesterfield added inline comments.May 6 2021, 7:40 PM
openmp/libomptarget/src/rtl.cpp
99

This part is factored out as D102043

protze.joachim added inline comments.May 7 2021, 10:22 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
648

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?

openmp/libomptarget/test/lit.cfg
182

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

clang/lib/Driver/ToolChains/CommonArgs.cpp
648

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.

openmp/libomptarget/test/lit.cfg
182

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?

JonChesterfield edited the summary of this revision. (Show Details)Tue, May 25, 3:18 PM