[PATCH] D134763: Add functionality to load dynamic libraries temporarily

Wed Sep 28 12:45:36 PDT 2022

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

I think it makes sense to introduce a `closeLibrary` operation, and I'm ok with this as an initial implementation.

I'm a little worried about introducing the separate `TemporaryHandles` set since that will affect lookup order, but we never really promised a sensible lookup order before anyway.

Long term we should refactor `sys::DynamicLibrary` to (1) be the thinnest possible wrapper around the OS library load / close calls, (2) provide guarantees on lookup order if possible (should we match the order that libraries were opened in? or use system RTLD_GLOBAL or equivalent?). I don't think that kind of refactoring should block this patch though.

@nhaehnle -- There was no library unloading story in https://reviews.llvm.org/D129127 beyond "they'll all get unloaded when the process terminates", right? I think we need a bulk-unloading story. Would it make sense to add a destructor to the `Globals` struct so that if a library with `sys::DynamicLibrary` compiled in is closed then all libraries loaded through that interface are closed too?

================
Comment at: llvm/include/llvm/Support/DynamicLibrary.h:99
+  static void closeLibrary(DynamicLibrary &Lib);
+
+  enum SearchOrdering {
----------------
Since you're touching these APIs anyway, could you add a note that they forward to the underlying OS library load/close operations, and that we make no guarantees about when those will actually close them library. (e.g. on Darwin, libraries containing Swift or ObjC will never be closed).

================
Comment at: llvm/lib/Support/DynamicLibrary.cpp:49-50
 #endif
+    assert(!AllowDuplicates ||
+           !CanClose && "CanClose must be false if AllowDuplicates is true.");

----------------
`&&` binds tighter than `||` -- I think you'll want parens here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134763