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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

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



================
Comment at: openmp/libomptarget/src/rtl.cpp:67
 
+  // Load plugins from same directory as libomptarget.so
+  std::string full_plugin_name;
----------------
ABataev wrote:
> 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?
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?


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


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


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

https://reviews.llvm.org/D73657