Page MenuHomePhabricator

[OPENMP] Load plugins from same directory as the and quick fail mechanism for offloading plugins
Needs ReviewPublic

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



This patch loads plugins from the same directory as 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

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


Why need to use dynamic allocation here?

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


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?


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.


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 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.


handle is never closed.


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


Again, that's not a thorough error handling.


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.


You need to handle the case where the code can't find out the location of 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.

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


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.


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


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.


This code src/rtl.cpp is part of 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

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/

lebedev.ri added inline comments.

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.

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


260 seems to be the limit on windows according to this blog article:

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.

Superseded by D87413, I think.

This revision now requires review to proceed.Mar 30 2021, 10:12 AM