[LLDB][GUI] Expand selected thread tree item by default

[OmarEmaraDev]

Summary

This patch expands the tree item that corresponds to the selected thread
by default in the Threads window. Additionally, the tree root item is
always expanded, which is the process in the Threads window.

[clayborg]

I have been meaning to get to this for a while, thanks for taking this on! It would be great if we can actually select the selected stack frame as well when this the process view is initialized. Same kind of code as with the selected thread. See inline comments.

lldb/source/Core/IOHandlerCursesGUI.cpp

3382–3384

Remove parens for single statement if due to LLVM coding guidelines.

3971–3973

Remove parens for single statement if due to LLVM coding guidelines.

[clayborg]

Including the inlined comments for showing and selecting the selected frame.

lldb/source/Core/IOHandlerCursesGUI.cpp

3971–3973

It would be great if we can also select the currently selected stack frame here. Something like:

if (selected_thread && selected_thread->GetID() == thread->GetID()) {
  item[i].Expand();
  StackFrameSP selected_frame = thread->GetSelectedFrame();
  if (selected_frame) {
    // Add all frames and select the right one...
  }
}

[OmarEmaraDev]

• Follow LLVM coding guidelines

[OmarEmaraDev]

lldb/source/Core/IOHandlerCursesGUI.cpp

3971–3973

@clayborg I didn't implement selection in this patch because my implementation didn't look great and I thought selection could be refactored to to make handling easier.

I haven't looked at this in more details yet, but I was thinking that maybe instead of having linear selection, we would have it hierarchical.
Each tree item declares which of its children is selected, so the process item will declare which thread item is selected and the selected thread item will declare which frame item is selected and so on.
It could be more conceptually correct because when selecting a frame, you are also selecting its parent thread. And selection can be checked by traversing parents and highlighting can be done by checking for leaf items. Something like that.

[clayborg]

lldb/source/Core/IOHandlerCursesGUI.cpp

3973

yeah, we need one frame item to be selected at all times so it indicates the current thread/frame that is selected within LLDB.

So a great solution would be to expand the selected thread and show all frames within the selected thread. Add code to select the current frame after as we can re-use this code as we step along in our program. If we add a command line window that allows LLDB commands, if the user types "frame select 12", the view should update to indicate which frame is selected. Also if the user types "thread select 5", the view should update accordingly.

[wallace]

lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py

39

create a method self.exit_gui to dedup some code, as you'll end up doing a lot of them as you write your tests

def exit_gui(self):
  self.child.send(chr(27).encode())
  self.expect_prompt()

you can also create a method start_gui

def start_gui(self):
  self.child.sendline("gui")
  self.child.send(chr(27).encode())

you could also later create a base class lldbgui_testcase similar to lldbvscode_testcase, where you can put all needed common code

[OmarEmaraDev]

• Merge branch 'main' into lldb-gui-expand-threads-tree
• Implement default selection

[OmarEmaraDev]

lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py

39

Okay. I will do for all tests in a separate patch.

[clayborg]

Many minor things, but looking good! I look forward to seeing this get checked in

lldb/source/Core/IOHandlerCursesGUI.cpp

3364–3365

Don't make this pure virtual unless all subclasses must implement this. I see a few implementations below that do nothing, so just make a default implementation so classes that don't need this don't have to implement it. Also renamed to TreeDelegateUpdateSelection to stay consistent with the other delegate methods.

3382–3383

We might want to opt into this? We don't want all variables in a variable view to be expanded by default. But we do want the process tree to always expand, so we need to be able to opt in in the tree delegates.

3612–3613

I assume we want this method to update both m_selected_row_idx and m_selected_item right? If so, make sure we switch to references on line 1611 above so both do get updated.

3797–3801

Remove and make a default implementation in the base class and don't make it pure virtual

3883–3887

Remove and make a default implementation in the base class and don't make it pure virtual

3942–3943

See comment below where you used to set m_need_to_set_default_selection to false.

4000

This just sets the local variable "selected_item" to a value. Make sure we switch to using references on 1611 as mentioned before.

4002

You can remove this and just always set this in the TreeDelegateGenerateChildren

4015

Maybe renamed to m_update_selection so this variable is a bit shorter?

[OmarEmaraDev]

• Handle review

[clayborg]

Looks good!

[omjavaid]

This change has caused various GUI related tests to fail randomly.
https://lab.llvm.org/buildbot/#/builders/96/builds/10100

This also causes random failures of one of the most resource sufficient buildbot here:
https://lab.llvm.org/buildbot/#/builders/68/builds/16275
https://lab.llvm.org/buildbot/#/builders/68/builds/16300

@teemperor
Referring to your comment on skipping TestGuiBasicDebug.py this may have caused it.

[omjavaid]

I suspect this change as culprit behind failure of various GUI tests randomly on various LLDB buildbots. I have reverted this change to see if its the real culprit.

lldb/source/Core/IOHandlerCursesGUI.cpp

 
 class TreeDelegate {
 public:
   TreeDelegate() = default;
   virtual ~TreeDelegate() = default;
 
   virtual void TreeDelegateDrawTreeItem(TreeItem &item, Window &window) = 0;
   virtual void TreeDelegateGenerateChildren(TreeItem &item) = 0;
+  virtual void TreeDelegateUpdateSelection(TreeItem &root, int &selection_index,
+                                           TreeItem *&selected_item) {
+    return;
+  }
   virtual bool TreeDelegateItemSelected(
       TreeItem &item) = 0; // Return true if we need to update views
+  virtual bool TreeDelegateExpandRootByDefault() { return false; }
 };
 
 typedef std::shared_ptr<TreeDelegate> TreeDelegateSP;
 
 class TreeItem {
 public:
   TreeItem(TreeItem *parent, TreeDelegate &delegate, bool might_have_children)
       : m_parent(parent), m_delegate(delegate), m_user_data(nullptr),
         m_identifier(0), m_row_idx(-1), m_children(),
-        m_might_have_children(might_have_children), m_is_expanded(false) {}
+        m_might_have_children(might_have_children), m_is_expanded(false) {
+    if (m_parent == nullptr)
+      m_is_expanded = m_delegate.TreeDelegateExpandRootByDefault();
+  }
 
   TreeItem &operator=(const 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;
bool WindowDelegateDraw(Window &window, bool force) override {
 
     window.Erase();
     window.DrawTitleBox(window.GetName());
 
     if (display_content) {
       const int num_visible_rows = NumVisibleRows();
       m_num_rows = 0;
       m_root.CalculateRowIndexes(m_num_rows);
+      m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
+                                                 m_selected_item);
 
       // If we unexpanded while having something selected our total number of
       // rows is less than the num visible rows, then make sure we show all the
       // rows by setting the first visible row accordingly.
       if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
         m_first_visible_row = 0;
 
       // Make sure the selected row is always visible
if (thread) {
       }
     }
   }
 
   void TreeDelegateGenerateChildren(TreeItem &item) override {
     // No children for frames yet...
   }
 
   bool TreeDelegateItemSelected(TreeItem &item) override {
     Thread *thread = (Thread *)item.GetUserData();
     if (thread) {
       thread->GetProcess()->GetThreadList().SetSelectedThreadByID(
           thread->GetID());
       const uint64_t frame_idx = item.GetIdentifier();
       thread->SetSelectedFrameByIndex(frame_idx);
       return true;
     }
     return false;
   }
 
 protected:
if (process_sp && process_sp->IsAlive()) {
           }
         }
         return;
       }
     }
     item.ClearChildren();
   }
 
   bool TreeDelegateItemSelected(TreeItem &item) override {
     ProcessSP process_sp = GetProcess();
     if (process_sp && process_sp->IsAlive()) {
       StateType state = process_sp->GetState();
       if (StateIsStoppedState(state, true)) {
         ThreadSP thread_sp = GetThread(item);
         if (thread_sp) {
           ThreadList &thread_list = thread_sp->GetProcess()->GetThreadList();
           std::lock_guard<std::recursive_mutex> guard(thread_list.GetMutex());
           ThreadSP selected_thread_sp = thread_list.GetSelectedThread();
           if (selected_thread_sp->GetID() != thread_sp->GetID()) {
             thread_list.SetSelectedThreadByID(thread_sp->GetID());
             return true;
protected:
   uint32_t m_stop_id;
   FormatEntity::Entry m_format;
 };
 
 class ThreadsTreeDelegate : public TreeDelegate {
 public:
   ThreadsTreeDelegate(Debugger &debugger)
       : TreeDelegate(), m_thread_delegate_sp(), m_debugger(debugger),
-        m_stop_id(UINT32_MAX) {
+        m_stop_id(UINT32_MAX), m_update_selection(false) {
     FormatEntity::Parse("process ${process.id}{, name = ${process.name}}",
                         m_format);
   }
 
   ~ThreadsTreeDelegate() override = default;
 
   ProcessSP GetProcess() {
     return m_debugger.GetCommandInterpreter()
if (process_sp && process_sp->IsAlive()) {
                                nullptr, false, false)) {
         int right_pad = 1;
         window.PutCStringTruncated(right_pad, strm.GetString().str().c_str());
       }
     }
   }
 
   void TreeDelegateGenerateChildren(TreeItem &item) override {
     ProcessSP process_sp = GetProcess();
+    m_update_selection = false;
     if (process_sp && process_sp->IsAlive()) {
       StateType state = process_sp->GetState();
       if (StateIsStoppedState(state, true)) {
         const uint32_t stop_id = process_sp->GetStopID();
         if (m_stop_id == stop_id)
           return; // Children are already up to date
 
         m_stop_id = stop_id;
+        m_update_selection = true;
 
         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);
         for (size_t i = 0; i < num_threads; ++i) {
-          item[i].SetIdentifier(threads.GetThreadAtIndex(i)->GetID());
+          ThreadSP thread = threads.GetThreadAtIndex(i);
+          item[i].SetIdentifier(thread->GetID());
           item[i].SetMightHaveChildren(true);
+          if (selected_thread->GetID() == thread->GetID())
+            item[i].Expand();
         }
         return;
       }
     }
     item.ClearChildren();
   }
 
+  void TreeDelegateUpdateSelection(TreeItem &root, int &selection_index,
+                                   TreeItem *&selected_item) override {
+    if (!m_update_selection)
+      return;
+
+    ProcessSP process_sp = GetProcess();
+    if (!(process_sp && process_sp->IsAlive()))
+      return;
+
+    StateType state = process_sp->GetState();
+    if (!StateIsStoppedState(state, true))
+      return;
+
+    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();
+    for (size_t i = 0; i < num_threads; ++i) {
+      ThreadSP thread = threads.GetThreadAtIndex(i);
+      if (selected_thread->GetID() == thread->GetID()) {
+        selected_item = &root[i][thread->GetSelectedFrameIndex()];
+        selection_index = selected_item->GetRowIndex();
+        return;
+      }
+    }
+  }
+
   bool TreeDelegateItemSelected(TreeItem &item) override { return false; }
 
+  bool TreeDelegateExpandRootByDefault() override { return true; }
+
 protected:
   std::shared_ptr<ThreadTreeDelegate> m_thread_delegate_sp;
   Debugger &m_debugger;
   uint32_t m_stop_id;
+  bool m_update_selection;
   FormatEntity::Entry m_format;
 };
 
 class ValueObjectListDelegate : public WindowDelegate {
 public:
   ValueObjectListDelegate() : m_rows() {}
 
   ValueObjectListDelegate(ValueObjectList &valobj_list)

lldb/test/API/commands/gui/expand-threads-tree/Makefile

+C_SOURCES := main.c
+ENABLE_THREADS := YES
+include Makefile.rules

lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py

+"""
+Test the 'gui' default thread tree expansion.
+The root process tree item and the tree item corresponding to the selected
+thread should be expanded by default.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestGuiExpandThreadsTree(PExpectTest):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    # 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 -r thread_start_routine", substrs=["Breakpoint 1", "address ="])
+        self.expect("run", substrs=["stop reason ="])
+
+        escape_key = chr(27).encode()
+
+        # Start the GUI and close the welcome window.
+        self.child.sendline("gui")
+        self.child.send(escape_key)
+        self.child.expect_exact("Threads")
+
+        # The thread running thread_start_routine should be expanded.
+        self.child.expect_exact("frame #0: thread_start_routine")
+
+        # Exit GUI.
+        self.child.send(escape_key)
+        self.expect_prompt()
+
+        # Select the main thread.
+        self.child.sendline("thread select 1")
+
+        # Start the GUI.
+        self.child.sendline("gui")
+        self.child.expect_exact("Threads")
+
+        # The main thread should be expanded.
+        self.child.expect("frame #\d+: main")
+
+        # Quit the GUI
+        self.child.send(escape_key)
+
+        self.expect_prompt()
+        self.quit()

lldb/test/API/commands/gui/expand-threads-tree/main.c

+#include <pthread.h>
+
+void *thread_start_routine(void *arg) { return NULL; }
+
+int main() {
+  pthread_t thread;
+  pthread_create(&thread, NULL, thread_start_routine, NULL);
+  pthread_join(thread, NULL);
+  return 0;
+}