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

[gregrodgers]

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.

[ABataev]

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?

[JonChesterfield]

@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

[RaviNarayanaswamy]

I don't recall saying needs changes.

[JonChesterfield]

In D73657#1848238, @RaviNarayanaswamy wrote:

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]

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.

[gregrodgers]

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]

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]

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.

[protze.joachim]

Not sure whether greg still plans to work on this. Anyway a inline comment to the PATH_MAX

openmp/libomptarget/src/rtl.cpp

72

260 seems to be the limit on windows according to this blog article: http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html

Since dlinfo does not use a size argument, I'd consider this prone to buffer overflow. Or in the best case you get a truncated path.
RTLD_DI_LINKMAP might be the better choice, as it should provide you access to the original name.

[Hahnfeld]

Superseded by D87413, I think.