From 3726be8b71ceb1092e912b0a278c743a342f496a Mon Sep 17 00:00:00 2001 From: Pavel Kosov Date: Wed, 16 Aug 2023 11:04:46 +0300 Subject: [PATCH] [lldb][gui] Update TreeItem children's m_parent on move Before this patch, any time TreeItem is copied in Resize method, its parent is not updated, which can cause crashes when, for example, thread window with multiple hierarchy levels is updated. Makes TreeItem move-only, removes TreeItem's m_delegate extra self-assignment by making it a pointer, adds code to fix up children's parent on move constructor and operator= Patch prepared by NH5pml30 ~~~ Huawei RRI, OS Lab Reviewed By: clayborg Differential Revision: https://reviews.llvm.org/D157960 Signed-off-by: Nikolai Kholiavin --- lldb/source/Core/IOHandlerCursesGUI.cpp | 100 +++++++++++------- .../API/commands/gui/spawn-threads/Makefile | 3 + .../gui/spawn-threads/TestGuiSpawnThreads.py | 47 ++++++++ .../API/commands/gui/spawn-threads/main.cpp | 25 +++++ 4 files changed, 135 insertions(+), 40 deletions(-) create mode 100644 lldb/test/API/commands/gui/spawn-threads/Makefile create mode 100644 lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py create mode 100644 lldb/test/API/commands/gui/spawn-threads/main.cpp diff --git a/lldb/source/Core/IOHandlerCursesGUI.cpp b/lldb/source/Core/IOHandlerCursesGUI.cpp index 0151255631bf..dcbd7e5c9b37 100644 --- a/lldb/source/Core/IOHandlerCursesGUI.cpp +++ b/lldb/source/Core/IOHandlerCursesGUI.cpp @@ -4613,30 +4613,48 @@ public: typedef std::shared_ptr TreeDelegateSP; -class TreeItem { +struct TreeItemData { + TreeItemData(TreeItem *parent, TreeDelegate &delegate, + bool might_have_children, bool is_expanded) + : m_parent(parent), m_delegate(&delegate), + m_might_have_children(might_have_children), m_is_expanded(is_expanded) { + } + +protected: + TreeItem *m_parent; + TreeDelegate *m_delegate; + void *m_user_data = nullptr; + uint64_t m_identifier = 0; + std::string m_text; + int m_row_idx = -1; // Zero based visible row index, -1 if not visible or for + // the root item + bool m_might_have_children; + bool m_is_expanded = false; +}; + +class TreeItem : public TreeItemData { public: TreeItem(TreeItem *parent, TreeDelegate &delegate, bool might_have_children) - : m_parent(parent), m_delegate(delegate), m_children(), - m_might_have_children(might_have_children) { - if (m_parent == nullptr) - m_is_expanded = m_delegate.TreeDelegateExpandRootByDefault(); - } + : TreeItemData(parent, delegate, might_have_children, + parent == nullptr + ? delegate.TreeDelegateExpandRootByDefault() + : false), + m_children() {} + + TreeItem(const TreeItem &) = delete; + TreeItem &operator=(const TreeItem &rhs) = delete; - TreeItem &operator=(const TreeItem &rhs) { + TreeItem &operator=(TreeItem &&rhs) { if (this != &rhs) { - m_parent = rhs.m_parent; - m_delegate = rhs.m_delegate; - m_user_data = rhs.m_user_data; - m_identifier = rhs.m_identifier; - m_row_idx = rhs.m_row_idx; - m_children = rhs.m_children; - m_might_have_children = rhs.m_might_have_children; - m_is_expanded = rhs.m_is_expanded; + TreeItemData::operator=(std::move(rhs)); + AdoptChildren(rhs.m_children); } return *this; } - TreeItem(const TreeItem &) = default; + TreeItem(TreeItem &&rhs) : TreeItemData(std::move(rhs)) { + AdoptChildren(rhs.m_children); + } size_t GetDepth() const { if (m_parent) @@ -4648,18 +4666,28 @@ public: void ClearChildren() { m_children.clear(); } - void Resize(size_t n, const TreeItem &t) { m_children.resize(n, t); } + void Resize(size_t n, TreeDelegate &delegate, bool might_have_children) { + if (m_children.size() >= n) { + m_children.erase(m_children.begin() + n, m_children.end()); + return; + } + m_children.reserve(n); + std::generate_n(std::back_inserter(m_children), n - m_children.size(), + [&, parent = this]() { + return TreeItem(parent, delegate, might_have_children); + }); + } TreeItem &operator[](size_t i) { return m_children[i]; } void SetRowIndex(int row_idx) { m_row_idx = row_idx; } size_t GetNumChildren() { - m_delegate.TreeDelegateGenerateChildren(*this); + m_delegate->TreeDelegateGenerateChildren(*this); return m_children.size(); } - void ItemWasSelected() { m_delegate.TreeDelegateItemSelected(*this); } + void ItemWasSelected() { m_delegate->TreeDelegateItemSelected(*this); } void CalculateRowIndexes(int &row_idx) { SetRowIndex(row_idx); @@ -4726,7 +4754,7 @@ public: if (highlight) window.AttributeOn(A_REVERSE); - m_delegate.TreeDelegateDrawTreeItem(*this, window); + m_delegate->TreeDelegateDrawTreeItem(*this, window); if (highlight) window.AttributeOff(A_REVERSE); @@ -4810,16 +4838,13 @@ public: void SetMightHaveChildren(bool b) { m_might_have_children = b; } protected: - TreeItem *m_parent; - TreeDelegate &m_delegate; - void *m_user_data = nullptr; - uint64_t m_identifier = 0; - std::string m_text; - int m_row_idx = -1; // Zero based visible row index, -1 if not visible or for - // the root item + void AdoptChildren(std::vector &children) { + m_children = std::move(children); + for (auto &child : m_children) + child.m_parent = this; + } + std::vector m_children; - bool m_might_have_children; - bool m_is_expanded = false; }; class TreeWindowDelegate : public WindowDelegate { @@ -5116,9 +5141,8 @@ public: m_stop_id = process_sp->GetStopID(); m_tid = thread_sp->GetID(); - TreeItem t(&item, *m_frame_delegate_sp, false); size_t num_frames = thread_sp->GetStackFrameCount(); - item.Resize(num_frames, t); + item.Resize(num_frames, *m_frame_delegate_sp, false); for (size_t i = 0; i < num_frames; ++i) { item[i].SetUserData(thread_sp.get()); item[i].SetIdentifier(i); @@ -5218,12 +5242,11 @@ public: std::make_shared(m_debugger); } - TreeItem t(&item, *m_thread_delegate_sp, false); ThreadList &threads = process_sp->GetThreadList(); std::lock_guard guard(threads.GetMutex()); ThreadSP selected_thread = threads.GetSelectedThread(); size_t num_threads = threads.GetSize(); - item.Resize(num_threads, t); + item.Resize(num_threads, *m_thread_delegate_sp, false); for (size_t i = 0; i < num_threads; ++i) { ThreadSP thread = threads.GetThreadAtIndex(i); item[i].SetIdentifier(thread->GetID()); @@ -5402,9 +5425,8 @@ public: if (!m_string_delegate_sp) m_string_delegate_sp = std::make_shared(); - TreeItem details_tree_item(&item, *m_string_delegate_sp, false); - item.Resize(details.GetSize(), details_tree_item); + item.Resize(details.GetSize(), *m_string_delegate_sp, false); for (size_t i = 0; i < details.GetSize(); i++) { item[i].SetText(details.GetStringAtIndex(i)); } @@ -5446,10 +5468,9 @@ public: if (!m_breakpoint_location_delegate_sp) m_breakpoint_location_delegate_sp = std::make_shared(m_debugger); - TreeItem breakpoint_location_tree_item( - &item, *m_breakpoint_location_delegate_sp, true); - item.Resize(breakpoint->GetNumLocations(), breakpoint_location_tree_item); + item.Resize(breakpoint->GetNumLocations(), + *m_breakpoint_location_delegate_sp, true); for (size_t i = 0; i < breakpoint->GetNumLocations(); i++) { item[i].SetIdentifier(i); item[i].SetUserData(breakpoint.get()); @@ -5493,9 +5514,8 @@ public: if (!m_breakpoint_delegate_sp) m_breakpoint_delegate_sp = std::make_shared(m_debugger); - TreeItem breakpoint_tree_item(&item, *m_breakpoint_delegate_sp, true); - item.Resize(breakpoints.GetSize(), breakpoint_tree_item); + item.Resize(breakpoints.GetSize(), *m_breakpoint_delegate_sp, true); for (size_t i = 0; i < breakpoints.GetSize(); i++) { item[i].SetIdentifier(i); } diff --git a/lldb/test/API/commands/gui/spawn-threads/Makefile b/lldb/test/API/commands/gui/spawn-threads/Makefile new file mode 100644 index 000000000000..566938ca0cc4 --- /dev/null +++ b/lldb/test/API/commands/gui/spawn-threads/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp +ENABLE_THREADS := YES +include Makefile.rules diff --git a/lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py b/lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py new file mode 100644 index 000000000000..54726de22b10 --- /dev/null +++ b/lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py @@ -0,0 +1,47 @@ +""" +Test that 'gui' does not crash when adding new threads, which +populate TreeItem's children and may be reallocated elsewhere. +""" + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test.lldbpexpect import PExpectTest + +import sys + +class TestGuiSpawnThreadsTest(PExpectTest): + # PExpect uses many timeouts internally and doesn't play well + # under ASAN on a loaded machine.. + @skipIfAsan + @skipIfCursesSupportMissing + def test_gui(self): + self.build() + + self.launch(executable=self.getBuildArtifact('a.out'), dimensions=(100, 500)) + self.expect( + 'breakpoint set -f main.cpp -p "break here"', substrs=['Breakpoint 1', 'address ='] + ) + self.expect( + 'breakpoint set -f main.cpp -p "before join"', substrs=['Breakpoint 2', 'address ='] + ) + self.expect("run", substrs=["stop reason ="]) + + escape_key = chr(27).encode() + + # Start the GUI + self.child.sendline("gui") + self.child.expect_exact("Threads") + self.child.expect_exact(f"thread #1: tid =") + + for i in range(5): + # Stopped at the breakpoint, continue over the thread creation + self.child.send("c") + # Check the newly created thread + self.child.expect_exact(f"thread #{i + 2}: tid =") + + # Exit GUI. + self.child.send(escape_key) + self.expect_prompt() + + self.quit() diff --git a/lldb/test/API/commands/gui/spawn-threads/main.cpp b/lldb/test/API/commands/gui/spawn-threads/main.cpp new file mode 100644 index 000000000000..c34e057bc715 --- /dev/null +++ b/lldb/test/API/commands/gui/spawn-threads/main.cpp @@ -0,0 +1,25 @@ +#include +#include +#include + +#include "pseudo_barrier.h" + +pseudo_barrier_t barrier_inside; + +void thread_func() { pseudo_barrier_wait(barrier_inside); } + +void test_thread() { + std::vector thrs; + for (int i = 0; i < 5; i++) + thrs.push_back(std::thread(thread_func)); // break here + + pseudo_barrier_wait(barrier_inside); // break before join + for (auto &t : thrs) + t.join(); +} + +int main() { + pseudo_barrier_init(barrier_inside, 6); + test_thread(); + return 0; +} -- Gitee