Fix memory leak in lib/Support/DynamicLibrary.cpp wrap OpenedHandles in llvm::ManagedStatic<>

[pkarneliuk]

Summary

This patch fixes a memory leak in Support library. Currently, the OpenedHandles is allocated dynamically and never deallocated. This patch wraps a raw pointer in llvm::ManagedStatic<> template. So, the registry of open handles will be managed by LLVM routines for static variables (llvm::llvm_shutdown()).

This patch is close to that https://github.com/llvm-mirror/llvm/commit/b4b9b3b24c03f414c54f55e1b34b4d9124c55883 patch.
I have removed fixme-message in the header.

[pkarneliuk]

Ping!

[chandlerc]

Please see the comments David sent to the mailing list about this patch. I've quoted them below:

This is one of those "what is a leak" existential questions.

There are two common attitudes/interpretations:

1. Memory that is unreachable
2. Memory that is never deallocated

Most tools have been taught to only report (1) and ignore (2).

This idiom (allocating and never deallocating globals) is sometimes used to avoid global destructors which can be dangerously racy (if a multithreaded program is shutting down (possibly due to exceptions, etc) and hasn't stopped all its worker threads, it could be problematic to try to destroy globals that may still be in use by other threads - leading to failure-on-failure modes that can make debugging the original failure more difficult)

[pkarneliuk]

Hi Chandler,

Ok, this object is reachable and is never deallocated. Is it design
decision or a piece of legacy code?
We can deallocate it via llvm::ManagedStatic<> at the shutdown. Similar to
next object:
static llvm::ManagedStatic<llvm::sys::SmartMutex<true> > SymbolsMutex;
This SymbolsMutex are used in static functions of DynamicLibrary module. We
already rely on this ownership routines for static variables.

This patch fixes legacy code and deallocates object stored in static
variable.
It looks like the patch requires some changes.
How I should rewrite it?

There is already accepted patch with similar changes:
https://github.com/llvm-mirror/llvm/commit/b4b9b3b24c03f414c54f55e1b34b4d9124c55883
This patch is a finish of his work.

Thanks,
Pavel K

2015-04-14 17:56 GMT+03:00 Chandler Carruth <[e-mail]>:

Please see the comments David sent to the mailing list about this patch.
I've quoted them below:

This is one of those "what is a leak" existential questions.

There are two common attitudes/interpretations:

1. Memory that is unreachable

2. Memory that is never deallocated

Most tools have been taught to only report (1) and ignore (2).

This idiom (allocating and never deallocating globals) is sometimes used

to avoid global destructors which can be dangerously racy (if a
multithreaded program is shutting down (possibly due to exceptions, etc)
and hasn't stopped all its worker threads, it could be problematic to try
to destroy globals that may still be in use by other threads - leading to
failure-on-failure modes that can make debugging the original failure more
difficult)

http://reviews.llvm.org/D8838

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

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 HAVE_DLFCN_H
   // 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

 namespace llvm {
 using namespace sys;
 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only Win32 specific code
 //===          and must not be UNIX code.
 //===----------------------------------------------------------------------===//
 
-static DenseSet<HMODULE> *OpenedHandles;
+static llvm::ManagedStatic<DenseSet<HMODULE> > OpenedHandles;
 
 static BOOL CALLBACK
 ELM_Callback(WIN32_ELMCB_PCSTR ModuleName, ULONG_PTR 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>();
 
     EnumerateLoadedModules(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>();
-
   // 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)
     FreeLibrary(a_handle);
 
   return DynamicLibrary(a_handle);
 }
 
void* DynamicLibrary::SearchForAddressOfSymbol(const char* symbolName) {
   if (ExplicitSymbols.isConstructed()) {
     StringMap<void *>::iterator i = ExplicitSymbols->find(symbolName);
 
     if (i != ExplicitSymbols->end())
       return i->second;
   }
 
   // Now search the libraries.
-  if (OpenedHandles) {
+  if (OpenedHandles.isConstructed()) {
     for (DenseSet<HMODULE>::iterator I = OpenedHandles->begin(),
          E = OpenedHandles->end(); I != E; ++I) {
       FARPROC ptr = GetProcAddress((HMODULE)*I, symbolName);
       if (ptr) {
         return (void *)(intptr_t)ptr;
       }
     }
   }