Page MenuHomePhabricator

[OPENMP] Load plugins from same directory as the libomptarget.so and quick fail mechanism for offloading plugins
Needs RevisionPublic

Authored by gregrodgers on Jan 29 2020, 11:47 AM.

Details

Summary

This patch loads plugins from the same directory as libomptarget.so is found. This avoids the need to set LD_LIBRARY_PATH to find plugins whose relative names are hardcoded in this source file.

This patch also provides a quick fail mechanism to avoid doing a dlopen for a platform that is not operational.

This patch also adds the name for the amdgcn plugin.

Diff Detail

Event Timeline

gregrodgers created this revision.Jan 29 2020, 11:47 AM
ABataev added inline comments.Jan 29 2020, 11:58 AM
openmp/libomptarget/src/rtl.cpp
67

Why do we want to do this? Also, does it mean that cmake configuration must be changed to put plugins in the same directory as libomptarget.so?

72

Why need to use dynamic allocation here?

@Ravi you're marked as needs changes but I can't see any comments

openmp/libomptarget/src/rtl.cpp
67

Setting LD_LIBRARY_PATH in order to work is nasty. I didn't realise that was the current status.

Loading relative to libomptarget is nicely position independent. Do you have a preferred location?

72

To keep it off the stack, but char[256] seems fine as a local to me too.

Is there a cross platform equivalent to this in the llvm libraries? Seems a bit suspect that dlinfo doesn't take the size of the buffer it writes to.

Plus I thought PATH_MAX was 256 and Google suggests Linux sets it to 260 now, so a wrapper with associated ready made error handling would be nice.

93

I'm not as sure about this. Probably faster, but is dlopen that slow when the file doesn't exist? Seems to run at terminal print speed when I'm debugging

I don't recall saying needs changes.

I don't recall saying needs changes.

Apologies, I have misread phabricator. You have an icon that means 'blocking review', not 'request changes'. Thanks!

Hahnfeld requested changes to this revision.Jan 29 2020, 11:52 PM
Hahnfeld added a subscriber: Hahnfeld.

As a general comment, I think this patch should be split into at least two separate ones:

  1. Load plugins from the same directory as libomptarget.so is found.
  2. Quick fail mechanism, although I'm not sure how much this saves? In most cases, we won't have the plugins for a foreign architecture (PPC on x86) anyway, that might only be a concern for accelerators.

I would strongly prefer if adding the amdgcn plugin was part of upstreaming that plugin, otherwise this change is just pointless.

openmp/libomptarget/src/rtl.cpp
69

handle is never closed.

70–71

AFAICT DP doesn't return, so the code will just fall through and case issues down below.

73–74

Again, that's not a thorough error handling.

86

Are you sure that incrementing inside the for loop is legal? I don't know what C++ says about the number of evaluations.

In any case, the code would be more clear if RTLQuickCheckFiles was merged into RTLNames, these two arrays are supposed to belong together.

95–97

You need to handle the case where the code can't find out the location of libomptarget.so. Plus I think the library should allow the user to load plugins from different locations by setting LD_LIBRARY_PATH. At the very least this would preserve compatibility, but it will also be helpful while developing out-of-tree plugins.

This revision now requires changes to proceed.Jan 29 2020, 11:52 PM
gregrodgers marked an inline comment as done.Feb 6 2020, 8:37 PM

I will rework this patch to

  • Try dlopen on relative library name first to for LD_LIBRARY_PATH search. If that fails, I will load using full path name.
  • Reorganize the names arrays into a single array to avoid a counter.
openmp/libomptarget/src/rtl.cpp
69

can it be closed? This code is in this dl . That is why we are sure it is loaded.

72

Sadly, nothing in the OpenMP runtime uses llvm libraries.
We don't know the size till dlinfo gives us the name. Hmmm, maybe there is a dlinfo on the size of the name.

86

Good idea, that would make for better maintenance and avoid this counter. I will fix this.

93

I am sure this is much faster. Doing a stat on a synthetic file is much faster than opening a real file that may exist but is useless to the running platform. Then the dlopen will attempt to load the useless file. Without this fix, users will be discouraged from building portable applications because of the potential failure on platform A from platform B if application was built for platforms A and B.

95–97

This code src/rtl.cpp is part of libomptarget.so. and it is already loaded. How can it not be found.? But I do like the idea of doing a dlopen on the relative file name, then if that fails use the full_plugin_name. So I will fix this. This extra path search is another good reason to have a quick fail method that skips non-active platforms.

jdoerfert added inline comments.Feb 8 2020, 12:16 PM
openmp/libomptarget/src/rtl.cpp
72

This all sounds scary. I am by far no expert in this but maybe PATH_MAX is the right size here, see als llvm/lib/Support/Unix/Path.inc

lebedev.ri added inline comments.
openmp/libomptarget/src/rtl.cpp
72

It is most certainly not 260, and not 256 on linux, but 4095.
On hurd it is righfully not defined at all - the code should not be making assumptions
about the upper bound on it's length, because clearly it is easy to fall short.