[lldb][gui] Update TreeItem children's m_parent on move

[kpdev42]

Summary

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

[clayborg]

lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py

39

lldb/source/Core/IOHandlerCursesGUI.cpp

public:
   // This is mostly useful for root tree delegates. If false is returned,
   // drawing will be skipped completely. This is needed, for instance, in
   // skipping drawing of the threads tree if there is no running process.
   virtual bool TreeDelegateShouldDraw() { return true; }
 };
 
 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)
       return 1 + m_parent->GetDepth();
     return 0;
   }
 
   int GetRowIndex() const { return m_row_idx; }
 
   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);
     ++row_idx;
 
     const bool expanded = IsExpanded();
 
     // The root item must calculate its children, or we must calculate the
if (m_row_idx >= first_visible_row) {
         window.PutChar(ACS_HLINE);
       }
       bool highlight = (selected_row_idx == static_cast<size_t>(m_row_idx)) &&
                        window.IsActive();
 
       if (highlight)
         window.AttributeOn(A_REVERSE);
 
-      m_delegate.TreeDelegateDrawTreeItem(*this, window);
+      m_delegate->TreeDelegateDrawTreeItem(*this, window);
 
       if (highlight)
         window.AttributeOff(A_REVERSE);
       ++row_idx;
       --num_rows_left;
     }
 
     if (num_rows_left <= 0)
if (text == nullptr) {
       return;
     }
     m_text = text;
   }
 
   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 {
 public:
   TreeWindowDelegate(Debugger &debugger, const TreeDelegateSP &delegate_sp)
       : m_debugger(debugger), m_delegate_sp(delegate_sp),
         m_root(nullptr, *delegate_sp, true) {}
 
if (process_sp && process_sp->IsAlive()) {
           if (!m_frame_delegate_sp) {
             // Always expand the thread item the first time we show it
             m_frame_delegate_sp = std::make_shared<FrameTreeDelegate>();
           }
 
           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);
           }
         }
         return;
       }
     }
if (process_sp && process_sp->IsAlive()) {
 
         if (!m_thread_delegate_sp) {
           // Always expand the thread item the first time we show it
           // item.Expand();
           m_thread_delegate_sp =
               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());
           item[i].SetMightHaveChildren(true);
           if (selected_thread->GetID() == thread->GetID())
             item[i].Expand();
         }
         return;
public:
   }
 
   void TreeDelegateGenerateChildren(TreeItem &item) override {
     BreakpointLocationSP breakpoint_location = GetBreakpointLocation(item);
     StringList details = ComputeDetailsList(breakpoint_location);
 
     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));
     }
   }
 
   bool TreeDelegateItemSelected(TreeItem &item) override { return false; }
 
 protected:
public:
   }
 
   void TreeDelegateGenerateChildren(TreeItem &item) override {
     BreakpointSP breakpoint = GetBreakpoint(item);
 
     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());
     }
   }
 
   bool TreeDelegateItemSelected(TreeItem &item) override { return false; }
 
void TreeDelegateGenerateChildren(TreeItem &item) override {
 
     BreakpointList &breakpoints = target->GetBreakpointList(false);
     std::unique_lock<std::recursive_mutex> lock;
     breakpoints.GetListMutex(lock);
 
     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);
     }
   }
 
   bool TreeDelegateItemSelected(TreeItem &item) override { return false; }
 
   bool TreeDelegateExpandRootByDefault() override { return true; }

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

+CXX_SOURCES := main.cpp
+ENABLE_THREADS := YES
+include Makefile.rules

lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py

+"""
+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

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