[PATCH] D29955: Allow externally dlopen-ed libraries to be registered as permanent libraries.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Support/DynamicLibrary.cpp:52
 
-static DenseSet<void *> *OpenedHandles = nullptr;
+static llvm::ManagedStatic<DenseSet<void *> > OpenedHandles;
 
----------------
v.g.vassilev wrote:
> vsk wrote:
> > This change is a bit out-of-scope for your patch, perhaps? A separate review may be worthwhile to figure out if we really need two layers of locking (one implicit in the ManagedStatic, another with the SymbolsMutex).
> Well, this follows the spirit of 'recent' (to us) changes in the area. I'd prefer to keep it together. It seems the ManagedStatic just handles the destruction at shutdown.
It seems ManagedStaticBase::RegisterManagedStatic does some locking, but I guess it's just a one-time thing.

At any rate, I'd suggest that the ManagedStatic change be left for a different commit, because it doesn't seem like a required change to make the addPermanentLibrary API work. 


================
Comment at: lib/Support/DynamicLibrary.cpp:74
+DynamicLibrary DynamicLibrary::addPermanentLibrary(void *handle) {
+  SmartScopedLock<true> lock(*SymbolsMutex);
 
----------------
v.g.vassilev wrote:
> vsk wrote:
> > Is there a double-lock scenario here? getPermanentLibrary() would lock, then addPermanentLibrary() would lock again?
> Yeah, I am not sure how to avoid this...
Maybe std::lock_guard would be a good solution. I.e add an API to grab a std::lock_guard from the DynamicLibrary class, then require the lock_guard to be moved into addPermanentLibrary.


https://reviews.llvm.org/D29955