[Openmp-commits] [PATCH] D87413: [OpenMP] Load plugins from same directory as the libomptarget.so

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

See comments below. High-level: Why don't we say `FullPluginName = Name`, then try to load, if that fails, get the CWD and prepend it to `FullPluginName` before trying again. So `LoadLibFromCWD` will become `bool getOrCreateCWD(std::string &CachedCWD) { if (CachedCWD) return true; ... }` and a lot of the logic should be simpler. Also consider moving the actual loading part `DP("Loading library '%s'...\n", Name);  void *dynlib_handle = dlopen(Name, RTLD_NOW);` into a helper so you can just call it twice instead of duplicating logic there again.



================
Comment at: openmp/libomptarget/src/rtl.cpp:65
+/// Load plugins from same directory as libomptarget.so
+static bool LoadLibFromCWD(char **PathName) {
+  struct link_map *Map;
----------------
Why `char**` ? I mean, you fiddle with a `char**` here and below you handle both a `char *` and a `std::string`. Just go with the string.


================
Comment at: openmp/libomptarget/src/rtl.cpp:123
+      }
     }
 
----------------
Now there are two problems.
1) You should not eagerly look into the CWD but only if we need to.
2) If we need to and it fails we should emit a proper error message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87413