[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;
 
----------------
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).


================
Comment at: lib/Support/DynamicLibrary.cpp:74
+DynamicLibrary DynamicLibrary::addPermanentLibrary(void *handle) {
+  SmartScopedLock<true> lock(*SymbolsMutex);
 
----------------
Is there a double-lock scenario here? getPermanentLibrary() would lock, then addPermanentLibrary() would lock again?


================
Comment at: lib/Support/DynamicLibrary.cpp:78
   if (!OpenedHandles->insert(handle).second)
-    dlclose(handle);
+    return DynamicLibrary();
 
----------------
There is a leak here when this function is called by getPermanentLibrary().


https://reviews.llvm.org/D29955