Page MenuHomePhabricator

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

Authored by OmarEmaraDev on Apr 10 2021, 4:00 AM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
260 msx64 debian > LLVM.CodeGen/AArch64::dag-combine-insert-subvector.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/dag-combine-insert-subvector.ll -o /dev/null
380 msx64 debian > LLVM.CodeGen/AArch64::inline-asm-constraints-bad-sve.ll
Script: -- : 'RUN: at line 1'; not /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=aarch64-none-linux-gnu -mattr=+sve -o - /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll
280 msx64 debian > LLVM.CodeGen/AArch64::named-vector-shuffles-sve.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -verify-machineinstrs < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll
280 msx64 debian > LLVM.CodeGen/AArch64::spillfill-sve.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=aarch64-none-linux-gnu -mattr=+sve -mattr=+bf16 < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/spillfill-sve.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/spillfill-sve.ll
370 msx64 debian > LLVM.CodeGen/AArch64::split-vector-insert.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/split-vector-insert.ll -debug-only=legalize-types 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/split-vector-insert.ll --check-prefix=CHECK-LEGALIZATION
View Full Test Results (310 Failed)

Event Timeline

OmarEmaraDev requested review of this revision.Apr 10 2021, 4:00 AM
OmarEmaraDev created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2021, 4:00 AM

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
1629–1631

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

2218–2220

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

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

lldb/source/Core/IOHandlerCursesGUI.cpp
2218–2220

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...
  }
}
  • Follow LLVM coding guidelines
OmarEmaraDev marked 2 inline comments as done.Apr 20 2021, 6:43 AM
OmarEmaraDev added inline comments.
lldb/source/Core/IOHandlerCursesGUI.cpp
2218–2220

@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 added inline comments.Apr 21 2021, 11:21 AM
lldb/source/Core/IOHandlerCursesGUI.cpp
2220

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 added inline comments.
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

  • Merge branch 'main' into lldb-gui-expand-threads-tree
  • Implement default selection
OmarEmaraDev added inline comments.Thu, Jun 10, 2:25 PM
lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
39

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

clayborg requested changes to this revision.Fri, Jun 11, 12:02 PM

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

lldb/source/Core/IOHandlerCursesGUI.cpp
1611–1612

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.

1629–1630

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.

1859–1860

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.

2044–2048

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

2130–2134

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

2189–2190

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

2247

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

2249

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

2262

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

This revision now requires changes to proceed.Fri, Jun 11, 12:02 PM
  • Handle review
clayborg accepted this revision.Fri, Jun 11, 2:08 PM

Looks good!

This revision is now accepted and ready to land.Fri, Jun 11, 2:08 PM