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

[David Blaikie]

On Mon, Apr 6, 2015 at 3:51 AM, Pavel Karneliuk <pavel.karneliuk at gmail.com>
wrote:

> Hi chandlerc, Bigcheese,
>
> 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.
>
> http://reviews.llvm.org/D8838
>
> Files:
>   lib/Support/DynamicLibrary.cpp
>   lib/Support/Windows/DynamicLibrary.inc
>
> Index: lib/Support/DynamicLibrary.cpp
> ===================================================================
> --- lib/Support/DynamicLibrary.cpp
> +++ lib/Support/DynamicLibrary.cpp
> @@ -9,8 +9,6 @@
>  //
>  //  This file implements the operating system DynamicLibrary concept.
>  //
> -// FIXME: This file leaks ExplicitSymbols and OpenedHandles!
> -//
>
>  //===----------------------------------------------------------------------===//
>
>  #include "llvm/Support/DynamicLibrary.h"
> @@ -51,7 +49,7 @@
>  //===          independent code.
>
>  //===----------------------------------------------------------------------===//
>
> -static DenseSet<void *> *OpenedHandles = nullptr;
> +static llvm::ManagedStatic<DenseSet<void *> > OpenedHandles;
>

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)

- David


>
>  DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *filename,
>                                                     std::string *errMsg) {
> @@ -70,9 +68,6 @@
>      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)
> @@ -121,7 +116,7 @@
>
>  #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);
> Index: lib/Support/Windows/DynamicLibrary.inc
> ===================================================================
> --- lib/Support/Windows/DynamicLibrary.inc
> +++ lib/Support/Windows/DynamicLibrary.inc
> @@ -39,7 +39,7 @@
>  //===          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,
> @@ -54,8 +54,6 @@
>
>    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".
> @@ -77,9 +75,6 @@
>      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)
> @@ -125,7 +120,7 @@
>    }
>
>    // 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);
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150406/c23bc28d/attachment.html>