[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:1921
 protected:
   std::vector<FieldDelegateSP> m_fields;
   std::vector<FormAction> m_actions;
----------------
Do these actually need to be shared pointers? They can probably become std::unique_ptr<T> objects and the FieldDelegateSP would then become FieldDelegateUP after defining it as a unique_ptr


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2199
+
   HandleCharResult SelectePrevious(int key) {
     if (m_selection_type == SelectionType::Action) {
----------------
Spelling issue I missed before


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2300
+    types.push_back(std::string("PID"));
+    m_type_field = AddChoicesField("Attach By", 2, types);
+
----------------
I am guessing that the first item in the list is always selected? Might be nice to provide a way for AddChoicesField to select a different item as the default selection with another optional parameter that is the index of the item that is to be selected. Doesn't have to happen in this diff, just thought of it and wanted to mention this.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2301
+    m_type_field = AddChoicesField("Attach By", 2, types);
+
+    m_pid_field = AddIntegerField("PID", 0);
----------------
remove space


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2302
+
+    m_pid_field = AddIntegerField("PID", 0);
+
----------------
I would be nice if we can eventually auto complete the PID by using the platform to get a list of processes. Sometimes we are not attached to a remote platform, but we are always attached to the host platform and should be able to get a list of process IDs. A new form could be popped up with all of the process details and allowing the user to filter processes with a TextField, then a ChoicesField that contains all valid processes the user could attach to with the pid, process name and possibly other details in the text for each choice. This doesn't need to be done now, but it would be a good extra functionality step to complete later if you get everything done on your list.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2303
+    m_pid_field = AddIntegerField("PID", 0);
+
+    m_name_field = AddTextField("Process Name", "");
----------------
remove space


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2304
+
+    m_name_field = AddTextField("Process Name", "");
+
----------------
If we have a target, it might be nice to auto populate this field with the basename of the target's main executable.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2305
+    m_name_field = AddTextField("Process Name", "");
+
+    m_plugin_field = AddTextField("Plugin Name", "");
----------------
remove space


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2306
+
+    m_plugin_field = AddTextField("Plugin Name", "");
+
----------------
Might be nice to iterate over all process plug-ins and make this into a ChoicesField where the first entry is "<default>" and the rest are actual plug-in names. The names can easily be retrieved using:

```
static const char *GetProcessPluginNameAtIndex(uint32_t idx);
```

from PluginManager.h



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2307
+    m_plugin_field = AddTextField("Plugin Name", "");
+
+    m_continue_field = AddBooleanField("Continue once attached.", false);
----------------
remove space


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2346
+      return;
+
+    if (process->GetShouldDetach()) {
----------------
We might want to pop up a dialog here asking the user if they want to kill or detach since we have a GUI. In the "process attach" command we confirm with the user if the terminal is interactive. 
A few ways we can do this:
- popup a form asking the user to kill/detach/cancel when the user hits submit on this form
- pop up the dialog before even showing the this form and asking the user to kill or detach, then show this form after that has been handled
- not allow the Process->Attach to happen by disabling the menu item when a process is running.




================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2367
+        m_debugger, "", "", eLoadDependentsNo, nullptr, new_target_sp);
+
+    target = new_target_sp.get();
----------------
We should select the target in he debugger here so that all GUI commands work correctly after this call if they ask the debugger for its selected target.

```
m_debugger.GetTargetList().SetSelectedTarget(...);
```


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2420-2421
+
+    if (attach_info.GetContinueOnceAttached())
+      process_sp->Resume();
+
----------------
You shouldn't need to do this since you already called ProcessAttachInfo::SetContinueOnceAttached(true) on the attach_info. LLDB should take care of this for you already I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655