[Openmp-commits] [PATCH] D73657: [OPENMP] Load plugins from same directory as the libomptarget.so and quick fail mechanism for offloading plugins

[Greg Rodgers via Phabricator via Openmp-commits]

gregrodgers marked an inline comment as done.
gregrodgers added a comment.

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.



================
Comment at: openmp/libomptarget/src/rtl.cpp:69
+  std::string full_plugin_name;
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);
+  if (!handle)
----------------
Hahnfeld wrote:
> `handle` is never closed.
can it be closed?  This code is in this dl .  That is why we are sure it is loaded.


================
Comment at: openmp/libomptarget/src/rtl.cpp:72
+    DP("dlopen() failed: %s\n", dlerror());
+  char *libomptarget_dir_name = new char[256];
+  if (dlinfo(handle, RTLD_DI_ORIGIN, libomptarget_dir_name) == -1)
----------------
JonChesterfield wrote:
> ABataev wrote:
> > Why need to use dynamic allocation here?
> 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.
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.


================
Comment at: openmp/libomptarget/src/rtl.cpp:86
+    bool found = false;
+    for (auto *QuickCheckName : RTLQuickCheckFiles[platform_num++]) {
+      if (QuickCheckName) {
----------------
Hahnfeld wrote:
> 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.
Good idea, that would make for better maintenance and avoid this counter.   I will fix this. 


================
Comment at: openmp/libomptarget/src/rtl.cpp:93
+    }
+    if (!found) // Not finding quick check files is a faster fail than dlopen
+      continue;
----------------
JonChesterfield wrote:
> 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 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.  


================
Comment at: openmp/libomptarget/src/rtl.cpp:95-97
+    full_plugin_name.assign(libomptarget_dir_name).append("/").append(Name);
+    DP("Loading library '%s'...\n", full_plugin_name.c_str());
+    void *dynlib_handle = dlopen(full_plugin_name.c_str(), RTLD_NOW);
----------------
Hahnfeld wrote:
> 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 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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73657/new/

https://reviews.llvm.org/D73657