[Lldb-commits] [lldb] 83695d4 - [lldb][gui] Update TreeItem children's m_parent on move
Pavel Kosov via lldb-commits
lldb-commits at lists.llvm.org
Wed Aug 16 01:10:53 PDT 2023
Author: Pavel Kosov
Date: 2023-08-16T11:10:00+03:00
New Revision: 83695d45d62121ab306d0dc108b549d9056a2f28
URL: https://github.com/llvm/llvm-project/commit/83695d45d62121ab306d0dc108b549d9056a2f28
DIFF: https://github.com/llvm/llvm-project/commit/83695d45d62121ab306d0dc108b549d9056a2f28.diff
LOG: [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
Added:
lldb/test/API/commands/gui/spawn-threads/Makefile
lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py
lldb/test/API/commands/gui/spawn-threads/main.cpp
Modified:
lldb/source/Core/IOHandlerCursesGUI.cpp
Removed:
################################################################################
diff --git a/lldb/source/Core/IOHandlerCursesGUI.cpp b/lldb/source/Core/IOHandlerCursesGUI.cpp
index 92f62b725ce385..22b8cc3582eae7 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -4614,30 +4614,48 @@ class TreeDelegate {
typedef std::shared_ptr<TreeDelegate> 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)
@@ -4649,18 +4667,28 @@ class TreeItem {
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);
@@ -4727,7 +4755,7 @@ class TreeItem {
if (highlight)
window.AttributeOn(A_REVERSE);
- m_delegate.TreeDelegateDrawTreeItem(*this, window);
+ m_delegate->TreeDelegateDrawTreeItem(*this, window);
if (highlight)
window.AttributeOff(A_REVERSE);
@@ -4811,16 +4839,13 @@ class TreeItem {
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<TreeItem> &children) {
+ m_children = std::move(children);
+ for (auto &child : m_children)
+ child.m_parent = this;
+ }
+
std::vector<TreeItem> m_children;
- bool m_might_have_children;
- bool m_is_expanded = false;
};
class TreeWindowDelegate : public WindowDelegate {
@@ -5117,9 +5142,8 @@ class ThreadTreeDelegate : public TreeDelegate {
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);
@@ -5219,12 +5243,11 @@ class ThreadsTreeDelegate : public TreeDelegate {
std::make_shared<ThreadTreeDelegate>(m_debugger);
}
- TreeItem t(&item, *m_thread_delegate_sp, false);
ThreadList &threads = process_sp->GetThreadList();
std::lock_guard<std::recursive_mutex> 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());
@@ -5404,9 +5427,8 @@ class BreakpointLocationTreeDelegate : public TreeDelegate {
if (!m_string_delegate_sp)
m_string_delegate_sp = std::make_shared<TextTreeDelegate>();
- 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));
}
@@ -5448,10 +5470,9 @@ class BreakpointTreeDelegate : public TreeDelegate {
if (!m_breakpoint_location_delegate_sp)
m_breakpoint_location_delegate_sp =
std::make_shared<BreakpointLocationTreeDelegate>(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());
@@ -5495,9 +5516,8 @@ class BreakpointsTreeDelegate : public TreeDelegate {
if (!m_breakpoint_delegate_sp)
m_breakpoint_delegate_sp =
std::make_shared<BreakpointTreeDelegate>(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 00000000000000..566938ca0cc4e1
--- /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 00000000000000..54726de22b100e
--- /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 00000000000000..c34e057bc71571
--- /dev/null
+++ b/lldb/test/API/commands/gui/spawn-threads/main.cpp
@@ -0,0 +1,25 @@
+#include <iostream>
+#include <thread>
+#include <vector>
+
+#include "pseudo_barrier.h"
+
+pseudo_barrier_t barrier_inside;
+
+void thread_func() { pseudo_barrier_wait(barrier_inside); }
+
+void test_thread() {
+ std::vector<std::thread> 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;
+}
More information about the lldb-commits
mailing list