Allow externally dlopen-ed libraries to be registered as permanent libraries.

[v.g.vassilev]

Summary

This is very helpful when we want a specific library to be dlopen-ed with different flags.

[vsk]

What's the expected use of the new API? Do we provide a portable way to dlopen() something with special flags?

include/llvm/Support/DynamicLibrary.h

77

Please add some doxygen comments for the new function, with a description of what 'handle' is.

lib/Support/DynamicLibrary.cpp

71

You won't have the symbols mutex held here if you call this method directly. Is that fine? I'm not really sure what that mutex protects (seems like more than just the ExplicitSymbols map)..

[v.g.vassilev]

Addressed comments; removed old copy-paste code (we shouldn't dlclose if the handle is already registered).

[vsk]

lib/Support/DynamicLibrary.cpp

52

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

71

Is there a double-lock scenario here? getPermanentLibrary() would lock, then addPermanentLibrary() would lock again?

77–92

There is a leak here when this function is called by getPermanentLibrary().

[v.g.vassilev]

Address comments; fix an oversight for Windows.

lib/Support/DynamicLibrary.cpp

52

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.

71

Yeah, I am not sure how to avoid this...

[marsupial]

This is somewhat related to something I've been trying to get reviewed here: https://reviews.llvm.org/D30107
I think there is a serious problem with DynamicLibrary::getPermanentLibrary on Windows.
I believe D30107 handles the memory leak as well.

[vsk]

lib/Support/DynamicLibrary.cpp

52

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.

71

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.

[v.g.vassilev]

• Split the OpenedHandles leak fix D30178;
• Add more fine-grained locking, avoiding the double locking issue;
• Add more information if an error occurred.

[v.g.vassilev]

Typo.

[v.g.vassilev]

lib/Support/DynamicLibrary.cpp

71

It seems the locking should protect only dlerror. I decided to lock around dlopen/dlclose because I found some bug reports of them not being thread safe on some implementations.

[vsk]

• Can you add a private addPermanentLibraryWithLock(void *handle, SmartScopedLock &) method? You can then use this from {add,get}PermanentLibrary, to avoid acquiring the mutex multiple times. That kills the benign race where two clients try to add the same dylib in parallel calls to getPermanentLibrary.
• Could you rebase this patch on D30178 once it's updated?

As a general note (unrelated to this patch), I'm a bit unhappy with how there is no clear thing that SymbolMutex guards, but we don't have to fix that here.

[v.g.vassilev]

Typo.

It seems that we could even avoid using a private member but local static functions. Let me know I should remove the static private method.

[vsk]

Thanks, lgtm. There is some overlap with D30107, but it looks like it'll need to be rebased on this patch anyway.

[marsupial]

FWIW applying D30107 before this would mean addPermanentLibraryWithLock would no longer be necessary.

Otherwise, doesn't this make it impossible to get symbols from an already loaded library unless all libs are searched?
That is, assuming "libSomething" is already loaded:
DynamicLibrary DL = DynamicLibrary::addPermanentLibrary(dlopen("libSomething"));
DL.getAddressOfSymbol("func"); // Fails

In this case how is it possible to get symbols from "libSomething" without searching every loaded module?
On Windows this is a larger issue as it can cache all loaded libraries.

[vsk]

In D29955#683652, @marsupial wrote:

FWIW applying D30107 before this would mean addPermanentLibraryWithLock would no longer be necessary.

Yeah, I didn't mean to create more work for you in D30107! The 'with-lock' helper seemed like the right solution here, and I have still have some questions/comments about D30107, hence the lgtm.

Otherwise, doesn't this make it impossible to get symbols from an already loaded library unless all libs are searched?
That is, assuming "libSomething" is already loaded:
DynamicLibrary DL = DynamicLibrary::addPermanentLibrary(dlopen("libSomething"));
DL.getAddressOfSymbol("func"); // Fails

In that case, the client could check if DL is invalid, and then simply construct "DynamicLibrary(dlopen'd-value)". Perhaps the API could be cleaned up if we returned "Expected<DynamicLibrary>" from the static constructors, and got rid of the isValid method.

In this case how is it possible to get symbols from "libSomething" without searching every loaded module?

See above.

[marsupial]

Yeah, I didn't mean to create more work for you in D30107! The 'with-lock' helper seemed like the right solution here, and I have still have some questions/comments about D30107, hence the lgtm.

It shouldn't be that much work integrating it...was just saying as the addPermanentLibraryWithLock is a bit ugly and would go away anyway.

Otherwise, doesn't this make it impossible to get symbols from an already loaded library unless all libs are searched?
That is, assuming "libSomething" is already loaded:
DynamicLibrary DL = DynamicLibrary::addPermanentLibrary(dlopen("libSomething"));
DL.getAddressOfSymbol("func"); // Fails

In that case, the client could check if DL is invalid, and then simply construct "DynamicLibrary(dlopen'd-value)". Perhaps the API could be cleaned up if we returned "Expected<DynamicLibrary>" from the static constructors, and got rid of the isValid method.

Why not just have addPermanentLibrary take an optional bool* indicating whether it was already loaded or not?
Whether it was loaded or not seems orthogonal to the task of adding a permanent library/getting a platform independent way to do lookups on it?

Either way if D30107 is to be expanded per your comments this could also be discussed then rather than holding this up.

[vsk]

In D29955#684287, @marsupial wrote:

Yeah, I didn't mean to create more work for you in D30107! The 'with-lock' helper seemed like the right solution here, and I have still have some questions/comments about D30107, hence the lgtm.

It shouldn't be that much work integrating it...was just saying as the addPermanentLibraryWithLock is a bit ugly and would go away anyway.

Otherwise, doesn't this make it impossible to get symbols from an already loaded library unless all libs are searched?
That is, assuming "libSomething" is already loaded:
DynamicLibrary DL = DynamicLibrary::addPermanentLibrary(dlopen("libSomething"));
DL.getAddressOfSymbol("func"); // Fails

In that case, the client could check if DL is invalid, and then simply construct "DynamicLibrary(dlopen'd-value)". Perhaps the API could be cleaned up if we returned "Expected<DynamicLibrary>" from the static constructors, and got rid of the isValid method.

Why not just have addPermanentLibrary take an optional bool* indicating whether it was already loaded or not?
Whether it was loaded or not seems orthogonal to the task of adding a permanent library/getting a platform independent way to do lookups on it?

If you mean 'bool &', then your suggestion is actually very similar to mine (return 'Expected<DynamicLibrary>').

I agree that it's something we could improve, but think that it's something we can address as a follow-up (i.e I don't think the addPermanentLibrary API is broken as-is).

Either way if D30107 is to be expanded per your comments this could also be discussed then rather than holding this up.

Sure.

[v.g.vassilev]

Landed in r296442 with minimal modifications allowing multiple dlopen(nullptr).

[chapuni]

Reverted in r296463.

The module is loaded twice for clang static analyzer plugins.

• clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
• clang/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp

Please consider such a case, or tweak clang plugins.
FYI, clang's plugin test can be enabled with CLANG_BUILD_EXAMPLES=ON

[v.g.vassilev]

Thanks for reverting! I am working on this.

[v.g.vassilev]

Relanded in r296774.

include/llvm/Support/DynamicLibrary.h

 //===-- llvm/Support/DynamicLibrary.h - Portable Dynamic Library -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file declares the sys::DynamicLibrary class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_DYNAMICLIBRARY_H
 #define LLVM_SUPPORT_DYNAMICLIBRARY_H
 
+#include "llvm/Support/Mutex.h"
+
 #include <string>
 
 namespace llvm {
 
 class StringRef;
 
 namespace sys {
 
class DynamicLibrary {
     // Placeholder whose address represents an invalid library.
     // We use this instead of NULL or a pointer-int pair because the OS library
     // might define 0 or 1 to be "special" handles, such as "search all".
     static char Invalid;
 
     // Opaque data used to interface with OS-specific dynamic library handling.
     void *Data;
 
+    // Adds a opened library handle to the list of OpenedHandles.
+    static DynamicLibrary addPermanentLibraryWithLock(void *handle,
+                                                  sys::SmartScopedLock<true> &);
+
   public:
     explicit DynamicLibrary(void *data = &Invalid) : Data(data) {}
 
     /// Returns true if the object refers to a valid library.
     bool isValid() const { return Data != &Invalid; }
 
     /// Searches through the library for the symbol \p symbolName. If it is
     /// found, the address of that symbol is returned. If not, NULL is returned.
public:
     /// instance on failure (see isValid()). \p *errMsg will only be modified
     /// if the library fails to load.
     ///
     /// It is safe to call this function multiple times for the same library.
     /// @brief Open a dynamic library permanently.
     static DynamicLibrary getPermanentLibrary(const char *filename,
                                               std::string *errMsg = nullptr);
 
+    /// Registers an externally loaded library. The library will be unloaded
+    /// when the program terminates.
+    ///
+    /// It is safe to call this function multiple times for the same library.
+    ///
+    /// \returns An empty \p DynamicLibrary on failure.
+    static DynamicLibrary addPermanentLibrary(void *handle);
+
     /// This function permanently loads the dynamic library at the given path.
     /// Use this instead of getPermanentLibrary() when you won't need to get
     /// symbols from the library itself.
     ///
     /// It is safe to call this function multiple times for the same library.
     static bool LoadLibraryPermanently(const char *Filename,
                                        std::string *ErrMsg = nullptr) {
       return !getPermanentLibrary(Filename, ErrMsg).isValid();

lib/Support/DynamicLibrary.cpp

 using namespace llvm;
 using namespace llvm::sys;
 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only TRULY operating system
 //===          independent code.
 //===----------------------------------------------------------------------===//
 
 static llvm::ManagedStatic<DenseSet<void *> > OpenedHandles;
 
 DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *filename,
                                                    std::string *errMsg) {
   SmartScopedLock<true> lock(*SymbolsMutex);
 
   void *handle = dlopen(filename, RTLD_LAZY|RTLD_GLOBAL);
   if (!handle) {
     if (errMsg) *errMsg = dlerror();
     return DynamicLibrary();
   }
 
 #ifdef __CYGWIN__
   // Cygwin searches symbols only in the main
   // with the handle of dlopen(NULL, RTLD_GLOBAL).
   if (!filename)
     handle = RTLD_DEFAULT;
 #endif
 
+  DynamicLibrary dyLib = addPermanentLibraryWithLock(handle, lock);
+
   // If we've already loaded this library, dlclose() the handle in order to
   // keep the internal refcount at +1.
-  if (!OpenedHandles->insert(handle).second)
+  if (!dyLib.isValid()) {
+    if (errMsg) *errMsg = std::string(filename) + ": Library already loaded";
     dlclose(handle);
+  }
+
+  return dyLib;
+}
+
+DynamicLibrary DynamicLibrary::addPermanentLibrary(void *handle) {
+  SmartScopedLock<true> lock(*SymbolsMutex);
+  return addPermanentLibraryWithLock(handle, lock);
+}
+
+DynamicLibrary DynamicLibrary::addPermanentLibraryWithLock(void *handle,
+                                                 sys::SmartScopedLock<true> &) {
+  // If we've already loaded this library, tell the caller.
+  if (!OpenedHandles->insert(handle).second)
+    return DynamicLibrary();
 
   return DynamicLibrary(handle);
 }
 
 void *DynamicLibrary::getAddressOfSymbol(const char *symbolName) {
   if (!isValid())
     return nullptr;
   return dlsym(Data, symbolName);

lib/Support/Windows/DynamicLibrary.inc

 //===- Win32/DynamicLibrary.cpp - Win32 DL Implementation -------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides the Win32 specific implementation of DynamicLibrary.
 //
-// FIXME: This file leaks OpenedHandles!
-//
 //===----------------------------------------------------------------------===//
 
 #include "WindowsSupport.h"
 
 #ifdef __MINGW32__
  #include <imagehlp.h>
 #else
  #include <dbghelp.h>
 #endif
 
 #ifdef _MSC_VER
  #include <ntverp.h>
 #endif
 
 namespace llvm {
 using namespace sys;
 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only Win32 specific code
 //===          and must not be UNIX code.
 //===----------------------------------------------------------------------===//
 
 typedef BOOL (WINAPI *fpEnumerateLoadedModules)(HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID);
 static fpEnumerateLoadedModules fEnumerateLoadedModules;
-static DenseSet<HMODULE> *OpenedHandles;
+static llvm::ManagedStatic<DenseSet<HMODULE> > OpenedHandles;
 
 static bool loadDebugHelp(void) {
   HMODULE hLib = ::LoadLibraryW(L"Dbghelp.dll");
   if (hLib) {
     fEnumerateLoadedModules = (fpEnumerateLoadedModules)
       ::GetProcAddress(hLib, "EnumerateLoadedModules64");
   }
   return fEnumerateLoadedModules != 0;
 }
 
 static BOOL CALLBACK
 ELM_Callback(PCSTR ModuleName, DWORD64 ModuleBase,
              ULONG ModuleSize, PVOID UserContext) {
   OpenedHandles->insert((HMODULE)ModuleBase);
   return TRUE;
 }
 
 DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *filename,
                                                    std::string *errMsg) {
   SmartScopedLock<true> lock(*SymbolsMutex);
 
   if (!filename) {
     // When no file is specified, enumerate all DLLs and EXEs in the process.
-    if (OpenedHandles == 0)
-      OpenedHandles = new DenseSet<HMODULE>();
-
     if (!fEnumerateLoadedModules) {
       if (!loadDebugHelp()) {
         assert(false && "These APIs should always be available");
         return DynamicLibrary();
       }
     }
 
     fEnumerateLoadedModules(GetCurrentProcess(), ELM_Callback, 0);
     // Dummy library that represents "search all handles".
     // This is mostly to ensure that the return value still shows up as "valid".
     return DynamicLibrary(&OpenedHandles);
   }
 
   SmallVector<wchar_t, MAX_PATH> filenameUnicode;
   if (std::error_code ec = windows::UTF8ToUTF16(filename, filenameUnicode)) {
     SetLastError(ec.value());
     MakeErrMsg(errMsg, std::string(filename) + ": Can't convert to UTF-16");
     return DynamicLibrary();
   }
-  
   HMODULE a_handle = LoadLibraryW(filenameUnicode.data());
 
   if (a_handle == 0) {
     MakeErrMsg(errMsg, std::string(filename) + ": Can't open");
     return DynamicLibrary();
   }
 
-  if (OpenedHandles == 0)
-    OpenedHandles = new DenseSet<HMODULE>();
-
+  DynamicLibrary dyLib = addPermanentLibraryWithLock(a_handle, lock);
   // If we've already loaded this library, FreeLibrary() the handle in order to
   // keep the internal refcount at +1.
-  if (!OpenedHandles->insert(a_handle).second)
+  if (!dyLib.isValid()) {
+    MakeErrMsg(errMsg, std::string(filename) + ": Already loaded");
     FreeLibrary(a_handle);
+  }
+
+  return dyLib;
+}
+
+DynamicLibrary DynamicLibrary::addPermanentLibrary(void *handle) {
+  SmartScopedLock<true> lock(*SymbolsMutex);
+  return addPermanentLibraryWithLock(handle, lock);
+}
+
+DynamicLibrary DynamicLibrary::addPermanentLibraryWithLock(void *handle,
+                                                 sys::SmartScopedLock<true> &) {
+  // If we've already loaded this library, tell the caller.
+  if (!OpenedHandles->insert((const HMODULE)handle).second)
+    return DynamicLibrary();
 
-  return DynamicLibrary(a_handle);
+  return DynamicLibrary((HMODULE)handle);
 }
 
 // Stack probing routines are in the support library (e.g. libgcc), but we don't
 // have dynamic linking on windows. Provide a hook.
 #define EXPLICIT_SYMBOL(SYM)                    \
   extern "C" { extern void *SYM; }
 #define EXPLICIT_SYMBOL2(SYMFROM, SYMTO) EXPLICIT_SYMBOL(SYMTO)