[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 
----------------
You don't want to return a unique_ptr here, if you do something like:
```
FieldDelegateUP field_up = form->GetField(1);
```
You have now given away the one reference to this field to the local variable and it will destruct the item when the local goes out of scope.

Easy solution: just return a pointer, not a unique_ptr. The FormDelegate owns all of its fields, so it is ok to hand out a pointer as others should temporarily use the FieldDelegate. Also we should bounds check "field_index".


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1985
 
-    FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+    FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
     ScrollContext context = field->FieldDelegateGetScrollContext();
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2152-2153
+      if (m_selection_type == SelectionType::Field) {
+        FieldDelegateUP &next_field =
+            m_delegate_sp->GetField(m_selection_index);
+        next_field->FieldDelegateSelectFirstElement();
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2159
 
-    FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+    FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
     if (!field->FieldDelegateOnLastOrOnlyElement()) {
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2176
+    if (m_selection_type == SelectionType::Field) {
+      FieldDelegateUP &next_field = m_delegate_sp->GetField(m_selection_index);
+      next_field->FieldDelegateSelectFirstElement();
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2208-2209
+      if (m_selection_type == SelectionType::Field) {
+        FieldDelegateUP &previous_field =
+            m_delegate_sp->GetField(m_selection_index);
+        previous_field->FieldDelegateSelectLastElement();
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2215
 
-    FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+    FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
     if (!field->FieldDelegateOnFirstOrOnlyElement()) {
----------------
No need to use a std::unique_ptr here, and GetField should return a pointer as mentioned above.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2232-2233
+    if (m_selection_type == SelectionType::Field) {
+      FieldDelegateUP &previous_field =
+          m_delegate_sp->GetField(m_selection_index);
+      previous_field->FieldDelegateSelectLastElement();
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2274
     if (m_selection_type == SelectionType::Field) {
-      FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+      FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
       return field->FieldDelegateHandleChar(key);
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2298
+  DetachOrKillProcessFormDelegate(Process *process) : m_process(process) {
+    if (process->GetShouldDetach()) {
+      SetError("There is a running process, detach from it?");
----------------
When we have interaction, we should allow the user to select kill or detach. The GetShouldDetach is only for when the user doesn't interact. Can we use a ChoicesField for this with the two options? We can even go a step further and add three options: "Detach", "Detach (stopped)", and "Kill". This is easy since lldb_private::Process has this option on detach:

```
  Status Process::Detach(bool keep_stopped);
```

When you detach with "keep_stopped" set to true the process will not resume. This allows another debugger to be attached, or someone to run another tool on the process in its suspended state.



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2324
+    }
+    window.GetParent()->RemoveSubWindow(&window);
+  }
----------------
Does anyone remove the sub window if the user cancels this dialog?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2342-2343
+        AddTextField("Process Name", GetDefaultProcessName().c_str());
+    m_plugin_field =
+        AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+    m_continue_field = AddBooleanField("Continue once attached.", false);
----------------
most people when attaching won't ever specify the plug-in names. I wonder if we should include an extra boolean field that enables this? Maybe:
```
m_show_plugin_field = AddBooleanField("Show advanced settings.", false);
```
And then show/hide any settings that aren't commonly used like the "Plugin Name" field?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2389-2395
+    const char *plugin_name;
+    for (int i = 0;
+         (plugin_name = PluginManager::GetProcessPluginNameAtIndex(i)) !=
+         nullptr;
+         i++) {
+      names.push_back(plugin_name);
+    }
----------------
While loop might be easier here?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2418
+    form_window_sp->SetDelegate(window_delegate_sp);
+
+    return true;
----------------
Is the form for the kill/detach done by the time it reaches this code? If so, what if the user cancels the Kill/Detach dialog? Can we return false here if the user cancels?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655