Re: [PATCH 11/12] detach in all-stop with threads running

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-01-12 8:15 p.m., Pedro Alves wrote:
> 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 then one inferior

"more than"

> 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.
> ---
>  gdb/infcmd.c |  13 ++++
>  gdb/infrun.c | 163 +++++++++++++++++++++++++++++++++++----------------
>  gdb/infrun.h |   4 ++
>  3 files changed, 128 insertions(+), 52 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 6f0ed952de6..ebaf57592ef 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 4a17410fcd6..4f1a16d2c60 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7098,6 +7098,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
> @@ -7108,8 +7111,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.  */
> @@ -7172,78 +7173,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;
> +    }

Hmm instead of that loop, could you do an early check in the function
instead, like

  if (proc_target->threads_executing)
    return;

? When a thread gets target-resumed, the thr->executing flag gets set
at the same time as the proc_target->threads_executing flag.  And when
resume_1-ing a thread with a pending status,
proc_target->threads_executing also gets set.  So maybe (just maybe)
that whole loop could be replaced with that check.

But otherwise, that LGTM.

Simon