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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Two actions with an option:

F17916215: 20210713-214929.png <https://reviews.llvm.org/F17916215>



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 
----------------
clayborg wrote:
> 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".
I see, I though returning it as a reference wouldn't case such issues.


================
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?");
----------------
clayborg wrote:
> 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.
> 
I think this might be a good place to add two actions with possible parameters, let me know if you think this would be a good idea.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2324
+    }
+    window.GetParent()->RemoveSubWindow(&window);
+  }
----------------
clayborg wrote:
> Does anyone remove the sub window if the user cancels this dialog?
Yes. This is done in `WindowDelegateHandleChar` of the `FormWindowDelegate`.


================
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);
----------------
clayborg wrote:
> 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?
This what I do for the launch and target creation forms. In this case, I didn't think it was worth it because it was just one field. But now that it is a choice field that takes a bit more space, maybe it would be a good idea to do this here as well.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2418
+    form_window_sp->SetDelegate(window_delegate_sp);
+
+    return true;
----------------
clayborg wrote:
> 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?
No, the return boolean here essentially denotes that a form for killing/detaching was spawned and that the attach form shouldn't continue executing. Once the user executes an action or cancels, the user will end up in the same attach from, where the user can execute attach again where it will succeed without the kill/detach dialog.

I can't think of a way to make the form block execution of another form until it is done without resorting to complicated trickery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655