[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

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);
     }

lldb/test/API/commands/gui/spawn-threads/Makefile

@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+ENABLE_THREADS := YES
+include Makefile.rules

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

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;
+}