[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4331-4333
+    if (StateIsRunningState(process->GetState())) {
+      return false;
+    }
----------------
Remove {} for single statement if statements per LLVM guidelines


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4448
+    Process *process = m_debugger.GetSelectedTarget()->GetProcessSP().get();
+    symbol_context.DumpStopContext(&stream, process, address,
+                                   false, /* Show full paths. */
----------------
I would show the address first, then the dump of the symbol context. You can ask the address from the location to dump itself as a load address and fallback to module + file address. Check out the Address::Dump function:

```
  bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, DumpStyle fallback_style, uint32_t addr_byte_size) const;
```

To get the raw address first in the string, you can first call the dump with:

```
// Emit the breakpoint location ID
stream.Format("{0} ", breakpoint_location->GetID())
// Emit the the address as a load address if the process is running, or fallback to <module>[<file-addr>] format if no process is running
address.Dump(&stream, process,  DumpStyleLoadAddress, DumpStyleModuleWithFileAddress, process->GetAddressByteSize());
```

Then you can add a space and then call this again with:

```
stream.PutChar(' ');
// Dump the resolved address description
address.Dump(&stream, process,  DumpStyleResolvedDescription, DumpStyleInvalid, process->GetAddressByteSize());
```

I believe that "DumpStyleResolvedDescription" will dump something verify similar to what you are dumping here with the "symbol_context.DumpStopContext(...)" call.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4454
+                                   true /* Show function name. */);
+    window.PutCStringTruncated(1, stream.GetString().str().c_str());
+  }
----------------
Breakpoint locations have integer IDs. We should show these as the first item in the string.

Take a look at the output of "breakpoint list" or "breakpoint list --verbose" for ideas on how to format the string.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4457
+
+  void TreeDelegateGenerateChildren(TreeItem &item) override {}
+
----------------
We could allow locations to be expanded and then show the details of the resolved address like in the "breakpoint list --verbose" output:
```
(lldb) breakpoint list --verbose
Current breakpoints:
1: name = 'main'
    1.1: 
      module = /Users/gclayton/Documents/src/args/build/a.out
      compile unit = main.cpp
      function = main
      location = /Users/gclayton/Documents/src/args/main.cpp:5:3
      address = a.out[0x0000000100003f3a]
      resolved = false
      hardware = false
      hit count = 0   
```
We could make a child for each of the lines in this output. I believe the about output is from calling Address::Dump(...) with a DumpStyle of "DumpStyleDetailedSymbolContext" (see above details on how to call Address::Dump(...)). You can dump this to a "StringStream" and then get the std::string back from it and split the string using newlines if you like that idea. I like the idea of getting full details on a breakpoint location by being able to expand it.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:4481
+    BreakpointSP breakpoint = GetBreakpoint(item);
+    StreamString stream;
+    breakpoint->GetResolverDescription(&stream);
----------------
Breakpoints have integer IDs. We should show these as the first item in the string. Not sure on the formatting of this number, maybe just the raw number followed by a space or possibly with square brackets and a space.

Take a look at the output of "breakpoint list" or "breakpoint list --verbose" for ideas on how to format the string.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:6635
     view_menu_sp->AddSubmenu(
         MenuSP(new Menu("Backtrace", nullptr, 'b',
                         ApplicationDelegate::eMenuID_ViewBacktrace)));
----------------
We would switch this one to 't' for backTrace and then let the breakpoint view take the 'b' key?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:6646
+    view_menu_sp->AddSubmenu(
+        MenuSP(new Menu("Breakpoints", nullptr, 'k',
+                        ApplicationDelegate::eMenuID_ViewBreakpoints)));
----------------
switch to 'b' and mentioned above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107386/new/

https://reviews.llvm.org/D107386