Do not leak OpenedHandles

[vsk]

lib/Support/DynamicLibrary.cpp

72

I'd expect all initializations of OpenedHandles to be unnecessary after your change.

lib/Support/Windows/DynamicLibrary.inc

12

@marsupial do you think it'd be a good idea to fix up the Windows version of OpenedHandles here, or would you rather do it in your patch?

[marsupial]

Believe it is fixed in my patch: allocating into a std::unique_ptr which should auto-destruct the contents on exit.
The ManagedStatic docs seems to talk about startup time, but I find it hard to believe that allocating space for a single pointer would be slower than the three it allocates plus other internals.

I'd prefer to use it in the patch I have as that addresses a more severe problem and this causes conflict resolution on my end.

[vsk]

In D30178#681805, @marsupial wrote:

Believe it is fixed in my patch: allocating into a std::unique_ptr which should auto-destruct the contents on exit.
The ManagedStatic docs seems to talk about startup time, but I find it hard to believe that allocating space for a single pointer would be slower than the three it allocates plus other internals.

It's not only about the space the unique_ptr would occupy -- startup times are impacted by global constructors. The advantage of ManagedStatic is that you don't pay the construction cost until you need to.

I'd prefer to use it in the patch I have as that addresses a more severe problem and this causes conflict resolution on my end.

Ok, well it sounds like you'd like to take care of it, so this patch will lgtm once the OpenedHandles initialization issues are fixed.

[marsupial]

I get what ManagedStatic is supposed to be doing, but versus using a std::unique_ptr the cost seems more with it (both in terms of size and startup time).
But I guess that conversation would add a bit of noise and doesn't have much to do with this patch.

[vsk]

In D30178#681888, @marsupial wrote:

I get what ManagedStatic is supposed to be doing, but versus using a std::unique_ptr the cost seems more with it (both in terms of size and startup time).

I'm not so sure. First, the unique_ptr isn't very helpful, because it's destructor can't be called until the program terminates, at which point it just creates needless allocator traffic. Clang has -disable-free to work around this problem. This is why ManagedStatic allows compile servers to work: because llvm_shutdown() actually frees the memory held by ManagedStatics. Second, it looks like ManagedStatic has a trivial constructor, making it cheap in the case where you don't use this API.

But I guess that conversation would add a bit of noise and doesn't have much to do with this patch.

I suppose so, its use here won't be changed.

[marsupial]

Ok, point taken about llvm_shutdown...I'll change it over there.

[v.g.vassilev]

Landed in r295737.

lib/Support/DynamicLibrary.cpp

 //===-- DynamicLibrary.cpp - Runtime link/load libraries --------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the operating system DynamicLibrary concept.
 //
-// FIXME: This file leaks ExplicitSymbols and OpenedHandles!
-//
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm-c/Support.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/ManagedStatic.h"

 using namespace llvm;
 using namespace llvm::sys;
 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only TRULY operating system
 //===          independent code.
 //===----------------------------------------------------------------------===//
 
-static DenseSet<void *> *OpenedHandles = nullptr;
+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
 
   if (!OpenedHandles)
     OpenedHandles = new DenseSet<void *>();
 
   // If we've already loaded this library, dlclose() the handle in order to
   // keep the internal refcount at +1.
   if (!OpenedHandles->insert(handle).second)
     dlclose(handle);
 
   return DynamicLibrary(handle);
 }
if (ExplicitSymbols.isConstructed()) {
     StringMap<void *>::iterator i = ExplicitSymbols->find(symbolName);
 
     if (i != ExplicitSymbols->end())
       return i->second;
   }
 
 #if defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN)
   // Now search the libraries.
-  if (OpenedHandles) {
+  if (OpenedHandles.isConstructed()) {
     for (DenseSet<void *>::iterator I = OpenedHandles->begin(),
          E = OpenedHandles->end(); I != E; ++I) {
       //lt_ptr ptr = lt_dlsym(*I, symbolName);
       void *ptr = dlsym(*I, symbolName);
       if (ptr) {
         return ptr;
       }
     }

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>