remove somewhat dangerous 'd'(etach) and 'k'(ill) shortcuts

[llunak]

Summary

'd' would be much better used for up/down shortcuts, and this also removes the possibility of ruining the whole debugging session by accidentally hitting 'd' or 'k'. Also change menu to have both 'detach and resume' and 'detach suspended' to make it clear which one is which. See discussion at https://reviews.llvm.org/D68541 .

[clayborg]

So it seems like we should be either moving everything to the process menu, or keeping everything and introducing a confirmation dialog for any dangerous commands (kill, both detaches). This patch seems to keep some dangerous commands (detach suspended), remove one (detach), and add another (kill).

lldb/source/Core/IOHandler.cpp

3766 ↗

(On Diff #224734)

Do we really want to remove 'd' and not 'D' as well? We now handle this in the process menu right?

4331 ↗

(On Diff #224734)

Do we really want to remove 'd' and not 'D' as well? We now handle this in the process menu right?

4340–4349 ↗

(On Diff #224734)

Is this not handled in the process menu? 'k' seems dangerous. Either that or we can introduce a confirmation dialog?

[clayborg]

If removing 'd' was only to make it available for "up" and "down" in https://reviews.llvm.org/D68541, then maybe we should switch 'D' to "detach and let run? Or is that what we did, but the comment on line 4331 is now out of date?

[llunak]

In D68908#2174222, @clayborg wrote:

If removing 'd' was only to make it available for "up" and "down" in https://reviews.llvm.org/D68541, then maybe we should switch 'D' to "detach and let run? Or is that what we did, but the comment on line 4331 is now out of date?

Making 'd' available for "down" was the original motivation, but not having single-key shortcuts with "dangerous" seemed like a good thing on its own (or maybe I thought I needed strong arguments for reassigning 'd', I don't quite remember). I don't have a strong opinion on 'D'. On the one hand shift+d is harder to hit by accident. On the other hand that's still somewhat possible, the action is still available as 'F3,d' in the menu, and gdb tui does not seem to have a single "dangerous" shortcut (according to https://sourceware.org/gdb/onlinedocs/gdb/TUI-Single-Key-Mode.html). And I've never used the detach action, so I'd prefer if somebody else decided this.

[llunak]

Updated for current git, and clang-format-ed.

[llunak]

lldb/source/Core/IOHandler.cpp

4340–4349 ↗

(On Diff #224734)

I think you got this backwards, it doesn't add a shortcut for kill, it removes it.

lldb/source/Core/IOHandlerCursesGUI.cpp

enum {
     eMenuID_LLDBExit,
 
     eMenuID_Target,
     eMenuID_TargetCreate,
     eMenuID_TargetDelete,
 
     eMenuID_Process,
     eMenuID_ProcessAttach,
-    eMenuID_ProcessDetach,
+    eMenuID_ProcessDetachResume,
+    eMenuID_ProcessDetachSuspended,
     eMenuID_ProcessLaunch,
     eMenuID_ProcessContinue,
     eMenuID_ProcessHalt,
     eMenuID_ProcessKill,
 
     eMenuID_Thread,
     eMenuID_ThreadStepIn,
     eMenuID_ThreadStepOver,
case eMenuID_ProcessHalt: {
       if (exe_ctx.HasProcessScope()) {
         Process *process = exe_ctx.GetProcessPtr();
         if (process && process->IsAlive())
           process->Halt();
       }
     }
       return MenuActionResult::Handled;
 
-    case eMenuID_ProcessDetach: {
+    case eMenuID_ProcessDetachResume:
+    case eMenuID_ProcessDetachSuspended: {
       ExecutionContext exe_ctx =
           m_debugger.GetCommandInterpreter().GetExecutionContext();
       if (exe_ctx.HasProcessScope()) {
         Process *process = exe_ctx.GetProcessPtr();
         if (process && process->IsAlive())
-          process->Detach(false);
+          process->Detach(menu.GetIdentifier() ==
+                          eMenuID_ProcessDetachSuspended);
       }
     }
       return MenuActionResult::Handled;
 
     case eMenuID_Process: {
       // Populate the menu with all of the threads if the process is stopped
       // when the Process menu gets selected and is about to display its
       // submenu.
KeyHelp *WindowDelegateGetKeyHelp() override {
     static curses::KeyHelp g_source_view_key_help[] = {
         {KEY_RETURN, "Run to selected line with one shot breakpoint"},
         {KEY_UP, "Select previous source line"},
         {KEY_DOWN, "Select next source line"},
         {KEY_PPAGE, "Page up"},
         {KEY_NPAGE, "Page down"},
         {'b', "Set breakpoint on selected source/disassembly line"},
         {'c', "Continue process"},
-        {'d', "Detach and resume process"},
         {'D', "Detach with process suspended"},
         {'h', "Show help dialog"},
-        {'k', "Kill process"},
         {'n', "Step over (source line)"},
         {'N', "Step over (single instruction)"},
         {'o', "Step out"},
         {'s', "Step in (source line)"},
         {'S', "Step in (single instruction)"},
         {',', "Page up"},
         {'.', "Page down"},
         {'\0', nullptr}};
case 'b': // 'b' == toggle breakpoint on currently selected line
           BreakpointSP bp_sp = exe_ctx.GetTargetRef().CreateBreakpoint(
               addr,   // lldb_private::Address
               false,  // internal
               false); // request_hardware
         }
       }
       return eKeyHandled;
 
-    case 'd': // 'd' == detach and let run
     case 'D': // 'D' == detach and keep stopped
     {
       ExecutionContext exe_ctx =
           m_debugger.GetCommandInterpreter().GetExecutionContext();
       if (exe_ctx.HasProcessScope())
-        exe_ctx.GetProcessRef().Detach(c == 'D');
-    }
-      return eKeyHandled;
-
-    case 'k':
-      // 'k' == kill
-      {
-        ExecutionContext exe_ctx =
-            m_debugger.GetCommandInterpreter().GetExecutionContext();
-        if (exe_ctx.HasProcessScope())
-          exe_ctx.GetProcessRef().Destroy(false);
-      }
+        exe_ctx.GetProcessRef().Detach(true);
       return eKeyHandled;
 
     case 'c':
       // 'c' == continue
       {
         ExecutionContext exe_ctx =
             m_debugger.GetCommandInterpreter().GetExecutionContext();
         if (exe_ctx.HasProcessScope())
target_menu_sp->AddSubmenu(MenuSP(new Menu(
         "Create", nullptr, 'c', ApplicationDelegate::eMenuID_TargetCreate)));
     target_menu_sp->AddSubmenu(MenuSP(new Menu(
         "Delete", nullptr, 'd', ApplicationDelegate::eMenuID_TargetDelete)));
 
     MenuSP process_menu_sp(new Menu("Process", "F3", KEY_F(3),
                                     ApplicationDelegate::eMenuID_Process));
     process_menu_sp->AddSubmenu(MenuSP(new Menu(
         "Attach", nullptr, 'a', ApplicationDelegate::eMenuID_ProcessAttach)));
-    process_menu_sp->AddSubmenu(MenuSP(new Menu(
-        "Detach", nullptr, 'd', ApplicationDelegate::eMenuID_ProcessDetach)));
+    process_menu_sp->AddSubmenu(
+        MenuSP(new Menu("Detach and resume", nullptr, 'd',
+                        ApplicationDelegate::eMenuID_ProcessDetachResume)));
+    process_menu_sp->AddSubmenu(
+        MenuSP(new Menu("Detach suspended", nullptr, 's',
+                        ApplicationDelegate::eMenuID_ProcessDetachSuspended)));
     process_menu_sp->AddSubmenu(MenuSP(new Menu(
         "Launch", nullptr, 'l', ApplicationDelegate::eMenuID_ProcessLaunch)));
     process_menu_sp->AddSubmenu(MenuSP(new Menu(Menu::Type::Separator)));
     process_menu_sp->AddSubmenu(
         MenuSP(new Menu("Continue", nullptr, 'c',
                         ApplicationDelegate::eMenuID_ProcessContinue)));
     process_menu_sp->AddSubmenu(MenuSP(new Menu(
         "Halt", nullptr, 'h', ApplicationDelegate::eMenuID_ProcessHalt)));