[Openmp-commits] [PATCH] D102043: [libomptarget] Look for plugins next to libomptarget.so

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/src/rtl.cpp:97
   DP("Loading RTLs...\n");
+  std::string LibomptargetPath = findLibomptargetDirectory();
 
----------------
If the dladdr failed (which I'm pretty sure it can't) this will gracefully fall back to the unadorned plugin name which will be found (or not) via LD_LIBRARY_PATH as before


================
Comment at: openmp/libomptarget/src/rtl.cpp:104
+    std::string AdjacentPluginName = LibomptargetPath + std::string(Name);
+    void *dynlib_handle = dlopen(AdjacentPluginName.c_str(), RTLD_NOW);
 
----------------
Could skip the loading if the plugin path is empty. I can't think of an instance where dladdr on ourself will fail but it makes for clearer debugging if we load no plugin at all in such a case instead of finding some other one through library search paths.


================
Comment at: openmp/libomptarget/src/rtl.cpp:68
 
+namespace {
+std::string findLibomptargetDirectory() {
----------------
JonChesterfield wrote:
> Would rather use llvm filesystem here but that patch hasn't landed yet. Should we use std::filesystem?
> 
> I don't think dlopen works on windows anyway so one more unix convention in this file seems acceptable.
Tried it locally - turns out the support lib containing sys::path isn't linked into libomptarget at present, and changing to that + llvm's dynamic library makes the code quite a lot longer. Suggest we stick with raw dl calls as that's used elsewhere in the library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102043