kpdev42 created this revision.
kpdev42 added reviewers: clayborg, davide, k8stone.
kpdev42 added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
kpdev42 requested review of this revision.
Herald added a subscriber: lldb-commits.
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=
~~~
Huawei RRI, OS Lab
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157960
Files:
lldb/source/Core/IOHandlerCursesGUI.cpp
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
Index: lldb/test/API/commands/gui/spawn-threads/main.cpp
===================================================================
--- /dev/null
+++ 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;
+}
Index: lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py
===================================================================
--- /dev/null
+++ 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 breakpoit, 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()
Index: lldb/test/API/commands/gui/spawn-threads/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/commands/gui/spawn-threads/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+ENABLE_THREADS := YES
+include Makefile.rules
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===================================================================
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -4614,30 +4614,48 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
