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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld requested changes to this revision.
Hahnfeld added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: openmp/libomptarget/src/rtl.cpp:69
+  std::string full_plugin_name;
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);
+  if (!handle)
----------------
`handle` is never closed.


================
Comment at: openmp/libomptarget/src/rtl.cpp:70-71
+  void *handle = dlopen("libomptarget.so", RTLD_NOW);
+  if (!handle)
+    DP("dlopen() failed: %s\n", dlerror());
+  char *libomptarget_dir_name = new char[256];
----------------
AFAICT `DP` doesn't return, so the code will just fall through and case issues down below.


================
Comment at: openmp/libomptarget/src/rtl.cpp:73-74
+  char *libomptarget_dir_name = new char[256];
+  if (dlinfo(handle, RTLD_DI_ORIGIN, libomptarget_dir_name) == -1)
+    DP("RTLD_DI_ORIGIN failed: %s\n", dlerror());
+  struct stat stat_buffer;
----------------
Again, that's not a thorough error handling.


================
Comment at: openmp/libomptarget/src/rtl.cpp:86
+    bool found = false;
+    for (auto *QuickCheckName : RTLQuickCheckFiles[platform_num++]) {
+      if (QuickCheckName) {
----------------
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.


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


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

https://reviews.llvm.org/D73657