detach in all-stop with threads running

A following patch will add a testcase that has a number of threads
constantly stepping over a breakpoint, and then has GDB detach the
process, while threads are running.  If we have more than one inferior
running, and we detach from just one of the inferiors, we expect that
the remaining inferior continues running.  However, in all-stop, if
GDB needs to pause the target for the detach, nothing is re-resuming
the other inferiors after the detach.  "info threads" shows the
threads as running, but they really aren't.  This fixes it.

gdb/ChangeLog:

	* infcmd.c (detach_command): Hold strong reference to target, and
	if all-stop on entry, restart threads on exit.
	* infrun.c (switch_back_to_stepped_thread): Factor out bits to ...
	(restart_stepped_thread): ... this new function.  Also handle
	trap_expected.
	(restart_after_all_stop_detach): New function.
	* infrun.h (restart_after_all_stop_detach): Declare.

4 files changed, 138 insertions, 52 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 365d137..4a02a64 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2021-02-03  Pedro Alves  <pedro@palves.net>
 
+	* infcmd.c (detach_command): Hold strong reference to target, and
+	if all-stop on entry, restart threads on exit.
+	* infrun.c (switch_back_to_stepped_thread): Factor out bits to ...
+	(restart_stepped_thread): ... this new function.  Also handle
+	trap_expected.
+	(restart_after_all_stop_detach): New function.
+	* infrun.h (restart_after_all_stop_detach): Declare.
+
+2021-02-03  Pedro Alves  <pedro@palves.net>
+
 	* infrun.c (struct step_over_info): Initialize fields.
 	(prepare_for_detach): Handle ongoing in-line step over.
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 6f0ed95..ebaf575 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2750,6 +2750,16 @@ detach_command (const char *args, int from_tty)
 
   disconnect_tracing ();
 
+  /* Hold a strong reference to the target while (maybe)
+     detaching the parent.  Otherwise detaching could close the
+     target.  */
+  auto target_ref
+    = target_ops_ref::new_reference (current_inferior ()->process_target ());
+
+  /* Save this before detaching, since detaching may unpush the
+     process_stratum target.  */
+  bool was_non_stop_p = target_is_non_stop_p ();
+
   target_detach (current_inferior (), from_tty);
 
   /* The current inferior process was just detached successfully.  Get
@@ -2766,6 +2776,9 @@ detach_command (const char *args, int from_tty)
 
   if (deprecated_detach_hook)
     deprecated_detach_hook ();
+
+  if (!was_non_stop_p)
+    restart_after_all_stop_detach (as_process_stratum_target (target_ref.get ()));
 }
 
 /* Disconnect from the current target without resuming it (leaving it
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6585ddd..f2abfe4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7110,6 +7110,9 @@ process_event_stop_test (struct execution_control_state *ecs)
   keep_going (ecs);
 }
 
+static bool restart_stepped_thread (process_stratum_target *resume_target,
+				    ptid_t resume_ptid);
+
 /* In all-stop mode, if we're currently stepping but have stopped in
    some other thread, we may need to switch back to the stepped
    thread.  Returns true we set the inferior running, false if we left
@@ -7120,8 +7123,6 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
 {
   if (!target_is_non_stop_p ())
     {
-      struct thread_info *stepping_thread;
-
       /* If any thread is blocked on some internal breakpoint, and we
 	 simply need to step over that breakpoint to get it going
 	 again, do that first.  */
@@ -7184,78 +7185,136 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
       if (!signal_program[ecs->event_thread->suspend.stop_signal])
 	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
 
-      /* Do all pending step-overs before actually proceeding with
-	 step/next/etc.  */
-      if (start_step_over ())
+      if (restart_stepped_thread (ecs->target, ecs->ptid))
 	{
 	  prepare_to_wait (ecs);
 	  return true;
 	}
 
-      /* Look for the stepping/nexting thread.  */
-      stepping_thread = NULL;
+      switch_to_thread (ecs->event_thread);
+    }
 
-      for (thread_info *tp : all_non_exited_threads ())
-	{
-	  switch_to_thread_no_regs (tp);
+  return false;
+}
 
-	  /* Ignore threads of processes the caller is not
-	     resuming.  */
-	  if (!sched_multi
-	      && (tp->inf->process_target () != ecs->target
-		  || tp->inf->pid != ecs->ptid.pid ()))
-	    continue;
+/* Look for the thread that was stepping, and resume it.
+   RESUME_TARGET / RESUME_PTID indicate the set of threads the caller
+   is resuming.  Return true if a thread was started, false
+   otherwise.  */
 
-	  /* When stepping over a breakpoint, we lock all threads
-	     except the one that needs to move past the breakpoint.
-	     If a non-event thread has this set, the "incomplete
-	     step-over" check above should have caught it earlier.  */
-	  if (tp->control.trap_expected)
-	    {
-	      internal_error (__FILE__, __LINE__,
-			      "[%s] has inconsistent state: "
-			      "trap_expected=%d\n",
-			      target_pid_to_str (tp->ptid).c_str (),
-			      tp->control.trap_expected);
-	    }
+static bool
+restart_stepped_thread (process_stratum_target *resume_target,
+			ptid_t resume_ptid)
+{
+  /* Do all pending step-overs before actually proceeding with
+     step/next/etc.  */
+  if (start_step_over ())
+    return true;
 
-	  /* Did we find the stepping thread?  */
-	  if (tp->control.step_range_end)
-	    {
-	      /* Yep.  There should only one though.  */
-	      gdb_assert (stepping_thread == NULL);
+  for (thread_info *tp : all_threads_safe ())
+    {
+      if (tp->state == THREAD_EXITED)
+	continue;
+
+      if (tp->suspend.waitstatus_pending_p)
+	continue;
 
-	      /* The event thread is handled at the top, before we
-		 enter this loop.  */
-	      gdb_assert (tp != ecs->event_thread);
+      /* Ignore threads of processes the caller is not
+	 resuming.  */
+      if (!sched_multi
+	  && (tp->inf->process_target () != resume_target
+	      || tp->inf->pid != resume_ptid.pid ()))
+	continue;
 
-	      /* If some thread other than the event thread is
-		 stepping, then scheduler locking can't be in effect,
-		 otherwise we wouldn't have resumed the current event
-		 thread in the first place.  */
-	      gdb_assert (!schedlock_applies (tp));
+      if (tp->control.trap_expected)
+	{
+	  infrun_debug_printf ("switching back to stepped thread (step-over)");
 
-	      stepping_thread = tp;
-	    }
+	  if (keep_going_stepped_thread (tp))
+	    return true;
 	}
+    }
+
+  for (thread_info *tp : all_threads_safe ())
+    {
+      if (tp->state == THREAD_EXITED)
+	continue;
+
+      if (tp->suspend.waitstatus_pending_p)
+	continue;
 
-      if (stepping_thread != NULL)
+      /* Ignore threads of processes the caller is not
+	 resuming.  */
+      if (!sched_multi
+	  && (tp->inf->process_target () != resume_target
+	      || tp->inf->pid != resume_ptid.pid ()))
+	continue;
+
+      /* Did we find the stepping thread?  */
+      if (tp->control.step_range_end)
 	{
-	  infrun_debug_printf ("switching back to stepped thread");
+	  infrun_debug_printf ("switching back to stepped thread (stepping)");
 
-	  if (keep_going_stepped_thread (stepping_thread))
-	    {
-	      prepare_to_wait (ecs);
-	      return true;
-	    }
+	  if (keep_going_stepped_thread (tp))
+	    return true;
 	}
-
-      switch_to_thread (ecs->event_thread);
     }
 
   return false;
 }
 
+/* See infrun.h.  */
+
+void
+restart_after_all_stop_detach (process_stratum_target *proc_target)
+{
+  /* Note we don't check target_is_non_stop_p() here, because the
+     current inferior may no longer have a process_stratum target
+     pushed, as we just detached.  */
+
+  /* See if we have a THREAD_RUNNING thread that need to be
+     re-resumed.  If we have any thread that is already executing,
+     then we don't need to resume the target -- it is already been
+     resumed.  With the remote target (in all-stop), it's even
+     impossible to issue another resumption if the target is already
+     resumed, until the target reports a stop.  */
+  for (thread_info *thr : all_threads (proc_target))
+    {
+      if (thr->state != THREAD_RUNNING)
+	continue;
+
+      /* If we have any thread that is already executing, then we
+	 don't need to resume the target -- it is already been
+	 resumed.  */
+      if (thr->executing)
+	return;
+
+      /* If we have a pending event to process, skip resuming the
+	 target and go straight to processing it.  */
+      if (thr->resumed && thr->suspend.waitstatus_pending_p)
+	return;
+    }
+
+  /* Alright, we need to re-resume the target.  If a thread was
+     stepping, we need to restart it stepping.  */
+  if (restart_stepped_thread (proc_target, minus_one_ptid))
+    return;
+
+  /* Otherwise, find the first THREAD_RUNNING thread and resume
+     it.  */
+  for (thread_info *thr : all_threads (proc_target))
+    {
+      if (thr->state != THREAD_RUNNING)
+	continue;
+
+      execution_control_state ecs;
+      reset_ecs (&ecs, thr);
+      switch_to_thread (thr);
+      keep_going (&ecs);
+      return;
+    }
+}
+
 /* Set a previously stepped thread back to stepping.  Returns true on
    success, false if the resume is not possible (e.g., the thread
    vanished).  */
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 7160b60..e643c84 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -269,4 +269,8 @@ extern void all_uis_check_sync_execution_done (void);
    started or re-started).  */
 extern void all_uis_on_sync_execution_starting (void);
 
+/* In all-stop, restart the target if it had to be stopped to
+   detach.  */
+extern void restart_after_all_stop_detach (process_stratum_target *proc_target);
+
 #endif /* INFRUN_H */