This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Fix for synthetic children memory leak
ClosedPublic

Authored by cameron314 on Oct 8 2019, 6:59 AM.

Details

Summary

This fixes a memory leak with several synthetic children front-ends.

The lifetime of a ValueObject and all its derivative ValueObjects (children, clones, etc.) is managed by a ClusterManager. These objects are only destroyed when every shared pointer to any of the managed objects in the cluster is destroyed. This means that no object in the cluster can store a shared pointer to another object in the cluster without creating a memory leak of the entire cluster. However, some of the synthetic children front-end implementations do exactly this; this patch fixes that.

The memory leak is actually much more important than just the lost memory, since it prevents the file handle to the binary (.elf in my case) from being closed due to references to the debug info from the ValueObject hierarchy. This means a build system is blocked from rebuilding the binary even after LLDB has disconnected.

Note that most of the existing synthetic children front-ends correctly avoid this buggy shared-pointer pattern with explicit comments about this exact type of memory leak. For example, in the NSErrorSyntheticFrontEnd:

// the child here can be "real" (i.e. an actual child of the root) or
// synthetized from raw memory if the former, I need to store a plain pointer
// to it - or else a loop of references will cause this entire hierarchy of
// values to leak if the latter, then I need to store a SharedPointer to it -
// so that it only goes away when everyone else in the cluster goes away oh
// joy!
ValueObject *m_child_ptr;
ValueObjectSP m_child_sp;

and the LibCxxMapIteratorSyntheticFrontEnd:

// this must be a ValueObject* because it is a child of the ValueObject we
// are producing children for it if were a ValueObjectSP, we would end up
// with a loop (iterator -> synthetic -> child -> parent == iterator) and
// that would in turn leak memory by never allowing the ValueObjects to die
// and free their memory

Apparently I'm not the first to lose a few hours debugging this :-)

Diff Detail

Event Timeline

cameron314 created this revision.Oct 8 2019, 6:59 AM

Disclaimer: I'm not familiar with this code. Would it be safe/better to store a weak pointer instead of a raw pointer?

@aprantl No. A weak pointer would still fix the memory leak, but it's safe to use a raw pointer because we only reference objects which are in the same cluster as the synthetic children front-end itself. The other (leak-free) synthetic front-ends do this as well. We want shared/weak pointers externally, but not within the cluster.

aprantl accepted this revision.Oct 8 2019, 10:07 AM

Cool. May be nice to put a comment on the pointer why this is safe.

This revision is now accepted and ready to land.Oct 8 2019, 10:07 AM
shafik accepted this revision.Oct 8 2019, 11:07 AM

LGTM

source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp
34

I feel like each of these pointer members deserve a comment.

Right, I'll add comments on the pointer declarations.

Added comments.

Any objections before I commit?

Please go ahead!

This revision was automatically updated to reflect the committed changes.
shafik added a comment.Oct 9 2019, 1:36 PM

This change broek the`TestDataFormatterInvalidStdUniquePtr.py` test, see: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2411/testReport/

I verified that reverting this commit fixes the test.

I've committed a fix in rG745e57c5939e. Sorry about that.