From 89386daa9571add3bc30311dc0902f82a1148a4c Mon Sep 17 00:00:00 2001 From: Cameron Desrochers Date: Wed, 9 Oct 2019 18:27:33 +0000 Subject: [PATCH] [LLDB] Fix for synthetic children memory leak 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. Differential Revision: https://reviews.llvm.org/D68641 llvm-svn: 374195 --- .../Plugins/Language/CPlusPlus/LibCxxBitset.cpp | 15 ++++++--- .../Plugins/Language/CPlusPlus/LibCxxOptional.cpp | 1 - .../Plugins/Language/CPlusPlus/LibCxxQueue.cpp | 11 +++++-- .../Plugins/Language/CPlusPlus/LibCxxTuple.cpp | 37 ++++++++++++++-------- .../Plugins/Language/CPlusPlus/LibCxxVariant.cpp | 1 - .../Plugins/Language/CPlusPlus/LibStdcppTuple.cpp | 11 +++++-- .../Language/CPlusPlus/LibStdcppUniquePointer.cpp | 23 ++++++++------ 7 files changed, 64 insertions(+), 35 deletions(-) diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp index 815dafb..78c453c 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp @@ -30,8 +30,15 @@ public: 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 m_elements; - ValueObjectSP m_first; + ValueObject* m_first = nullptr; CompilerType m_bool_type; ByteOrder m_byte_order = eByteOrderInvalid; uint8_t m_byte_size = 0; @@ -50,7 +57,7 @@ BitsetFrontEnd::BitsetFrontEnd(ValueObject &valobj) bool BitsetFrontEnd::Update() { m_elements.clear(); - m_first.reset(); + m_first = nullptr; TargetSP target_sp = m_backend.GetTargetSP(); if (!target_sp) @@ -63,7 +70,7 @@ bool BitsetFrontEnd::Update() { m_elements.assign(size, ValueObjectSP()); - m_first = m_backend.GetChildMemberWithName(ConstString("__first_"), true); + m_first = m_backend.GetChildMemberWithName(ConstString("__first_"), true).get(); return false; } @@ -86,7 +93,7 @@ ValueObjectSP BitsetFrontEnd::GetChildAtIndex(size_t idx) { chunk = m_first->GetChildAtIndex(idx / *bit_size, true); } else { type = m_first->GetCompilerType(); - chunk = m_first; + chunk = m_first->GetSP(); } if (!type || !chunk) return {}; diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp index 1160215..b1ad171 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp @@ -31,7 +31,6 @@ public: private: size_t m_size = 0; - ValueObjectSP m_base_sp; }; } // namespace diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp index 4b72089..2f06d68 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp @@ -38,16 +38,21 @@ public: } 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; } diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp index 8da7460..c86ab6b 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp @@ -30,47 +30,56 @@ public: ValueObjectSP GetChildAtIndex(size_t idx) override; private: - std::vector 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 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 * diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp index 491cf04..62945bd 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp @@ -184,7 +184,6 @@ public: private: size_t m_size = 0; - ValueObjectSP m_base_sp; }; } // namespace diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp index 66624e5..15bf7f8 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp @@ -37,7 +37,12 @@ public: size_t GetIndexOfChildWithName(ConstString name) override; private: - std::vector 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 m_members; }; } // end of anonymous namespace @@ -72,7 +77,7 @@ bool LibStdcppTupleSyntheticFrontEnd::Update() { 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()); } } } @@ -86,7 +91,7 @@ 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(); } diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp index 3860f96..84f9e57 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp @@ -39,9 +39,14 @@ public: 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(); }; @@ -92,17 +97,17 @@ bool LibStdcppUniquePtrSyntheticFrontEnd::Update() { 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(); } } @@ -114,11 +119,11 @@ 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(); } -- 2.7.4