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

Thu Sep 29 11:09:18 PDT 2022

Holman added a comment.

In D134763#3821829 <https://reviews.llvm.org/D134763#3821829>, @lhames wrote:

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

I agree, I went this route because it was the least intrusive to the current API and I didn't fully understand the context of why getPermanentLibrary opens a handle and then closes the existing handle if one exists instead of something like the semantics I added, but I think I see now it was about lookup order.

> @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?

`HandleSet` has a destructor that I believe that should covers this case.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134763