Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silence false positive memory leaks reported by MSVC debug CRT #892

Closed
wants to merge 5 commits into from
Closed

Silence false positive memory leaks reported by MSVC debug CRT #892

wants to merge 5 commits into from

Conversation

scottslacksmith
Copy link
Contributor

@scottslacksmith scottslacksmith commented Oct 5, 2016

There are few Issues (e.g. #624 , #692 and #757) requesting a "memory leak" fix for MSVC debug builds. In fact these are not true memory leaks but false positives due to limitations of MSVC's lightweight memory leak detection mechanism (see below). But I think this is missing the real point that #624 , #692 and #757 are trying to solve. It's highly desirable that all false positive leaks be either fixed or silenced otherwise it becomes impractical to identify real leaks using MSVC's leak report. I normally endeavour to fix all false positive leaks (e.g. as suggested by #757) but in this instance silencing is the best solution as it's impossible to test that the destructors aren't called too early for all possible build configuration that may exist in the wild.

As background, MSVC debug CRT can only detect if a memory allocation is missing a matching deallocation. Note MSVC debug CRT is always tracking memory allocations however the memory leak report is disabled by default. As you can’t avoid paying for its cost, you may as well use it. The memory leak report can be enabled by calling the following function
_CrtSetDbgFlag(_CrtSetDbgFlag(_CRTDBG_REPORT_FLAG) | _CRTDBG_LEAK_CHECK_DF);
anywhere before exiting main.
For example, the following are the false positive leaks reported by gtest_prod_test.exe before this change;

Detected memory leaks!
Dumping objects ->
{750} normal block at 0x015DF938, 8 bytes long.
Data: <  ]     > 00 F9 5D 01 00 00 00 00
{749} normal block at 0x015DEE60, 32 bytes long.
Data: <` ] ` ] ` ]     > 60 EE 5D 01 60 EE 5D 01 60 EE 5D 01 01 01 CD CD
{748} normal block at 0x015DF900, 12 bytes long.
Data: <8 ] ` ]     > 38 F9 5D 01 60 EE 5D 01 00 00 00 00
{747} normal block at 0x015DA0F8, 24 bytes long.
Data: <                > FF FF FF FF FF FF FF FF 00 00 00 00 00 00 00 00
Object dump complete.

As you can see from above it’s not easy to identify which leaks are false
positives or not. Consequently, if false positive leaks are not fixed or
silenced, then it becomes impractical to identify real memory leaks.

Add a RAII MemoryIsNotDeallocated class that excludes memory allocations
in scope from Microsoft’s debug CRT leak detection report. Use the this
RAII class to silence 2 false positive leaks that are caused by memory
allocations that are intentionally never deallocated.
*Background*
The MS debug CRT has a lightweight memory leak detection mechanism that
can only detect if a memory allocation is missing a matching
deallocation. Consequently, it will report a false positive leak for
memory that’s intentionally never deallocated. For example, memory
that’s reachable for the entire lifetime of a app.
The MS debug CRT is always tracking memory allocations however the final
memory leak report is disabled by default. As you can’t avoid paying for
its cost, you may as well use it. The memory leak report can be enabled
by calling the following function
::_CrtSetDbgFlag(::_CrtSetDbgFlag(_CRTDBG_REPORT_FLAG) |
_CRTDBG_LEAK_CHECK_DF);
anywhere before exiting main.
For example, the following are the false positive leaks reported before
this change;
Detected memory leaks!
Dumping objects ->
{750} normal block at 0x015DF938, 8 bytes long.
Data: <  ]     > 00 F9 5D 01 00 00 00 00
{749} normal block at 0x015DEE60, 32 bytes long.
Data: <` ] ` ] ` ]     > 60 EE 5D 01 60 EE 5D 01 60 EE 5D 01 01 01 CD CD
{748} normal block at 0x015DF900, 12 bytes long.
Data: <8 ] ` ]     > 38 F9 5D 01 60 EE 5D 01 00 00 00 00
{747} normal block at 0x015DA0F8, 24 bytes long.
Data: <                > FF FF FF FF FF FF FF FF 00 00 00 00 00 00 00 00
Object dump complete.

As you can see from above it’s not easy to identify the above are false
positives. Consequently, if false positive leaks are not fixed or
silenced, then it becomes impractical to identify real memory leaks.
@scottslacksmith
Copy link
Contributor Author

scottslacksmith commented Oct 6, 2016

A possibly more elegant solution is to defined an immortal pointer. For example

template<typename T>
struct immortal_delete {
  void operator()(T*) const noexcept {}
};

// Is an unique_ptr where the held object can never be destroyed.
// The motivation is to self-document objects that are intentionally 
// never destroy/deallocated because they are reachable for the entire
// lifetime of an app.
template<typename T>
using immortal_unique_ptr = std::unique_ptr<T, immortal_delete<T>>;

template<class T, class... Args> 
immortal_unique_ptr<T> make_immortal_unique(Args&&... args)
{
  MemoryIsNotDeallocated memoryIsNotDeallocated;
  return (immortal_unique_ptr<T>(new T(std::forward<Args>(args)...)));
}

(reusing std::unique_ptr may not work if an implementation's debug version of std::unique_ptr's dtor resets the naked pointer to nullptr to uncover hidden bugs, in which case we would be necessary to create a new immortal_unique_ptr class that mimics unique_ptr)

In which case

  // Returns map of thread local instances.
  static ThreadIdToThreadLocals* GetThreadLocalsMapLocked() {
    mutex_.AssertHeld();
    static ThreadIdToThreadLocals* map = new ThreadIdToThreadLocals;
    return map;
  }

becomes

  // Returns map of thread local instances.
  static ThreadIdToThreadLocals* GetThreadLocalsMapLocked() {
    mutex_.AssertHeld();
    static immortal_unique_ptr<ThreadIdToThreadLocals> map = immortal_unique_ptr<ThreadIdToThreadLocals>();
    return map.get();
  }

The advantage is that we clearly document which object constructions are intentionally never destroyed. For example, it could handy in a different context where an app is constantly loading and unloading .so/.dll plugins, in which case I believe a function static like
static ThreadIdToThreadLocals* map = new ThreadIdToThreadLocals;
could be classified as a true memory leak.

@gennadiycivil
Copy link
Contributor

Apologies for not getting to this sooner. As a preparation for 1.8.x release I am cleaning up older PR that are in CI failed status and/or have merge conflicts.
Please submit a new PR if you think this is still important/relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants