[LLDB] Fix for synthetic children memory leak

[cameron314]

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 :-)

[aprantl]

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

[cameron314]

@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]

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

[shafik]

LGTM

source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp

34 ↗

(On Diff #223650)

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

[cameron314]

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

[cameron314]

Added comments.

[cameron314]

Any objections before I commit?

[aprantl]

Please go ahead!

[shafik]

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.

[cameron314]

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

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

public:
   }
 
   bool MightHaveChildren() override { return true; }
   bool Update() override;
   size_t CalculateNumChildren() override { return m_elements.size(); }
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
+  // 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 them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  // Value objects created from raw data (i.e. in a different cluster) must
+  // be referenced via shared pointer to keep them alive, however.
   std::vector<ValueObjectSP> m_elements;
-  ValueObjectSP m_first;
+  ValueObject* m_first = nullptr;
   CompilerType m_bool_type;
   ByteOrder m_byte_order = eByteOrderInvalid;
   uint8_t m_byte_size = 0;
 };
 } // namespace
 
 BitsetFrontEnd::BitsetFrontEnd(ValueObject &valobj)
     : SyntheticChildrenFrontEnd(valobj) {
   m_bool_type = valobj.GetCompilerType().GetBasicTypeFromAST(eBasicTypeBool);
   if (auto target_sp = m_backend.GetTargetSP()) {
     m_byte_order = target_sp->GetArchitecture().GetByteOrder();
     m_byte_size = target_sp->GetArchitecture().GetAddressByteSize();
     Update();
   }
 }
 
 bool BitsetFrontEnd::Update() {
   m_elements.clear();
-  m_first.reset();
+  m_first = nullptr;
 
   TargetSP target_sp = m_backend.GetTargetSP();
   if (!target_sp)
     return false;
   size_t capping_size = target_sp->GetMaximumNumberOfChildrenToDisplay();
 
   size_t size = 0;
   if (auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0))
     size = arg->value.getLimitedValue(capping_size);
 
   m_elements.assign(size, ValueObjectSP());
 
-  m_first = m_backend.GetChildMemberWithName(ConstString("__first_"), true);
+  m_first = m_backend.GetChildMemberWithName(ConstString("__first_"), true).get();
   return false;
 }
 
 ValueObjectSP BitsetFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx >= m_elements.size() || !m_first)
     return ValueObjectSP();
 
   if (m_elements[idx])
     return m_elements[idx];
 
   ExecutionContext ctx = m_backend.GetExecutionContextRef().Lock(false);
   CompilerType type;
   ValueObjectSP chunk;
   // For small bitsets __first_ is not an array, but a plain size_t.
   if (m_first->GetCompilerType().IsArrayType(&type, nullptr, nullptr)) {
     llvm::Optional<uint64_t> bit_size =
         type.GetBitSize(ctx.GetBestExecutionContextScope());
     if (!bit_size || *bit_size == 0)
       return {};
     chunk = m_first->GetChildAtIndex(idx / *bit_size, true);
   } else {
     type = m_first->GetCompilerType();
-    chunk = m_first;
+    chunk = m_first->GetSP();
   }
   if (!type || !chunk)
     return {};
 
   llvm::Optional<uint64_t> bit_size =
       type.GetBitSize(ctx.GetBestExecutionContextScope());
   if (!bit_size || *bit_size == 0)
     return {};

lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp

public:
 
   bool MightHaveChildren() override { return true; }
   bool Update() override;
   size_t CalculateNumChildren() override { return m_size; }
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
   size_t m_size = 0;
-  ValueObjectSP m_base_sp;
 };
 } // namespace
 
 bool OptionalFrontEnd::Update() {
   ValueObjectSP engaged_sp(
       m_backend.GetChildMemberWithName(ConstString("__engaged_"), true));
 
   if (!engaged_sp)

lldb/source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp

public:
   }
 
   ValueObjectSP GetChildAtIndex(size_t idx) override {
     return m_container_sp ? m_container_sp->GetChildAtIndex(idx, true)
                           : nullptr;
   }
 
 private:
-  ValueObjectSP m_container_sp;
+  // 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 them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  ValueObject* m_container_sp = nullptr;
 };
 } // namespace
 
 bool QueueFrontEnd::Update() {
-  m_container_sp.reset();
+  m_container_sp = nullptr;
   ValueObjectSP c_sp = m_backend.GetChildMemberWithName(ConstString("c"), true);
   if (!c_sp)
     return false;
-  m_container_sp = c_sp->GetSyntheticValue();
+  m_container_sp = c_sp->GetSyntheticValue().get();
   return false;
 }
 
 SyntheticChildrenFrontEnd *
 formatters::LibcxxQueueFrontEndCreator(CXXSyntheticChildren *,
                                        lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
     return new QueueFrontEnd(*valobj_sp);
   return nullptr;
 }

lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp

public:
   }
 
   bool MightHaveChildren() override { return true; }
   bool Update() override;
   size_t CalculateNumChildren() override { return m_elements.size(); }
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
-  std::vector<ValueObjectSP> m_elements;
-  ValueObjectSP m_base_sp;
+  // 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 them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  std::vector<ValueObject*> m_elements;
+  ValueObject* m_base = nullptr;
 };
 }
 
 bool TupleFrontEnd::Update() {
   m_elements.clear();
-  m_base_sp = m_backend.GetChildMemberWithName(ConstString("__base_"), true);
-  if (! m_base_sp) {
+  m_base = nullptr;
+
+  ValueObjectSP base_sp;
+  base_sp = m_backend.GetChildMemberWithName(ConstString("__base_"), true);
+  if (!base_sp) {
     // Pre r304382 name of the base element.
-    m_base_sp = m_backend.GetChildMemberWithName(ConstString("base_"), true);
+    base_sp = m_backend.GetChildMemberWithName(ConstString("base_"), true);
   }
-  if (! m_base_sp)
+  if (!base_sp)
     return false;
-  m_elements.assign(m_base_sp->GetCompilerType().GetNumDirectBaseClasses(),
-                    ValueObjectSP());
+  m_base = base_sp.get();
+  m_elements.assign(base_sp->GetCompilerType().GetNumDirectBaseClasses(),
+                    nullptr);
   return false;
 }
 
 ValueObjectSP TupleFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx >= m_elements.size())
     return ValueObjectSP();
-  if (!m_base_sp)
+  if (!m_base)
     return ValueObjectSP();
   if (m_elements[idx])
-    return m_elements[idx];
+    return m_elements[idx]->GetSP();
 
   CompilerType holder_type =
-      m_base_sp->GetCompilerType().GetDirectBaseClassAtIndex(idx, nullptr);
+      m_base->GetCompilerType().GetDirectBaseClassAtIndex(idx, nullptr);
   if (!holder_type)
     return ValueObjectSP();
-  ValueObjectSP holder_sp = m_base_sp->GetChildAtIndex(idx, true);
+  ValueObjectSP holder_sp = m_base->GetChildAtIndex(idx, true);
   if (!holder_sp)
     return ValueObjectSP();
 
   ValueObjectSP elem_sp = holder_sp->GetChildAtIndex(0, true);
   if (elem_sp)
     m_elements[idx] =
-        elem_sp->Clone(ConstString(llvm::formatv("[{0}]", idx).str()));
+        elem_sp->Clone(ConstString(llvm::formatv("[{0}]", idx).str())).get();
 
-  return m_elements[idx];
+  return m_elements[idx]->GetSP();
 }
 
 SyntheticChildrenFrontEnd *
 formatters::LibcxxTupleFrontEndCreator(CXXSyntheticChildren *,
                                        lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
     return new TupleFrontEnd(*valobj_sp);
   return nullptr;
 }

lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp

public:
 
   bool MightHaveChildren() override { return true; }
   bool Update() override;
   size_t CalculateNumChildren() override { return m_size; }
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
   size_t m_size = 0;
-  ValueObjectSP m_base_sp;
 };
 } // namespace
 
 bool VariantFrontEnd::Update() {
   m_size = 0;
   ValueObjectSP impl_sp(
       m_backend.GetChildMemberWithName(ConstString("__impl"), true));
   if (!impl_sp)

lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp

public:
 
   bool Update() override;
 
   bool MightHaveChildren() override;
 
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
-  std::vector<ValueObjectSP> m_members;
+  // 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 them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  std::vector<ValueObject*> m_members;
 };
 
 } // end of anonymous namespace
 
 LibStdcppTupleSyntheticFrontEnd::LibStdcppTupleSyntheticFrontEnd(
     lldb::ValueObjectSP valobj_sp)
     : SyntheticChildrenFrontEnd(*valobj_sp) {
   Update();
for (size_t i = 0; i < child_count; ++i) {
       if (name_str.startswith("std::_Tuple_impl<")) {
         next_child_sp = child_sp;
       } else if (name_str.startswith("std::_Head_base<")) {
         ValueObjectSP value_sp =
             child_sp->GetChildMemberWithName(ConstString("_M_head_impl"), true);
         if (value_sp) {
           StreamString name;
           name.Printf("[%zd]", m_members.size());
-          m_members.push_back(value_sp->Clone(ConstString(name.GetString())));
+          m_members.push_back(value_sp->Clone(ConstString(name.GetString())).get());
         }
       }
     }
   }
 
   return false;
 }
 
 bool LibStdcppTupleSyntheticFrontEnd::MightHaveChildren() { return true; }
 
 lldb::ValueObjectSP
 LibStdcppTupleSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx < m_members.size())
-    return m_members[idx];
+    return m_members[idx]->GetSP();
   return lldb::ValueObjectSP();
 }
 
 size_t LibStdcppTupleSyntheticFrontEnd::CalculateNumChildren() {
   return m_members.size();
 }
 
 size_t LibStdcppTupleSyntheticFrontEnd::GetIndexOfChildWithName(

lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp

public:
 
   bool MightHaveChildren() override;
 
   size_t GetIndexOfChildWithName(ConstString name) override;
 
   bool GetSummary(Stream &stream, const TypeSummaryOptions &options);
 
 private:
-  ValueObjectSP m_ptr_obj;
-  ValueObjectSP m_obj_obj;
-  ValueObjectSP m_del_obj;
+  // 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 them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  ValueObject* m_ptr_obj = nullptr;
+  ValueObject* m_obj_obj = nullptr;
+  ValueObject* m_del_obj = nullptr;
 
   ValueObjectSP GetTuple();
 };
 
 } // end of anonymous namespace
 
 LibStdcppUniquePtrSyntheticFrontEnd::LibStdcppUniquePtrSyntheticFrontEnd(
     lldb::ValueObjectSP valobj_sp)
bool LibStdcppUniquePtrSyntheticFrontEnd::Update() {
   if (!tuple_sp)
     return false;
 
   std::unique_ptr<SyntheticChildrenFrontEnd> tuple_frontend(
       LibStdcppTupleSyntheticFrontEndCreator(nullptr, tuple_sp));
 
   ValueObjectSP ptr_obj = tuple_frontend->GetChildAtIndex(0);
   if (ptr_obj)
-    m_ptr_obj = ptr_obj->Clone(ConstString("pointer"));
+    m_ptr_obj = ptr_obj->Clone(ConstString("pointer")).get();
 
   ValueObjectSP del_obj = tuple_frontend->GetChildAtIndex(1);
   if (del_obj)
-    m_del_obj = del_obj->Clone(ConstString("deleter"));
+    m_del_obj = del_obj->Clone(ConstString("deleter")).get();
 
   if (m_ptr_obj) {
     Status error;
     ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
     if (error.Success()) {
-      m_obj_obj = obj_obj->Clone(ConstString("object"));
+      m_obj_obj = obj_obj->Clone(ConstString("object")).get();
     }
   }
 
   return false;
 }
 
 bool LibStdcppUniquePtrSyntheticFrontEnd::MightHaveChildren() { return true; }
 
 lldb::ValueObjectSP
 LibStdcppUniquePtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx == 0)
-    return m_ptr_obj;
+    return m_ptr_obj->GetSP();
   if (idx == 1)
-    return m_del_obj;
+    return m_del_obj->GetSP();
   if (idx == 2)
-    return m_obj_obj;
+    return m_obj_obj->GetSP();
   return lldb::ValueObjectSP();
 }
 
 size_t LibStdcppUniquePtrSyntheticFrontEnd::CalculateNumChildren() {
   if (m_del_obj)
     return 2;
   return 1;
 }