Re: [PATCH] Slighly tweak and clarify target_resume's interface

[Simon Marchi <simark@simark.ca>]

On 2022-04-21 11:49, Pedro Alves wrote:
> The current target_resume interface is a bit odd & non-intuitive.
> I've found myself explaining it a couple times the recent past, while
> reviewing patches that assumed STEP/SIGNAL always applied to the
> passed in PTID.  It goes like this today:

Thanks, this is a nice cleanup.

>
>   - if the passed in PTID is a thread, then the step/signal request is
>     for that thread.
>
>   - otherwise, if PTID is a wildcard, the step/signal request is for

Can wildcard here means either a "full" wildcard (minus_one_ptid) or an
inferior wildcard, (pid, 0, 0)?

>     inferior_ptid, and PTID indicates which set of threads run free.
>
> Because GDB always switches the current thread to "leader" thread
> being resumed/stepped/signalled, we can simplify this a bit to:
>
>   - step/signal are always for inferior_ptid.
>
>   - PTID indicates the set of threads that run free.
>
> Still not ideal, but it's a minimal change and at least there are no
> special cases this way.
>
> That's what this patch does.  It renames the PTID parameter to
> SCOPE_PTID, adds some assertions to target_resume, and tweaks
> target_resume's description.  In addition, it also renames PTID to
> SCOPED_PTID in the remote and linux-nat targets, and simplifies their

SCOPED_PTID -> SCOPE_PTID.

> @@ -1669,11 +1659,11 @@ linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
>        return;
>      }
>
> -  if (resume_many)
> -    iterate_over_lwps (ptid, [=] (struct lwp_info *info)
> -			     {
> -			       return linux_nat_resume_callback (info, lp);
> -			     });
> +  if (minus_one_ptid == scope_ptid || scope_ptid.is_pid ())

This could also be

  if (scope_ptid != inferior_ptid)

of

  if (scope_ptid != lp->ptid)

if you think the intent is clearer this way.

> +    iterate_over_lwps (scope_ptid, [=] (struct lwp_info *info)
> +      {
> +	return linux_nat_resume_callback (info, lp);
> +      });
>
>    linux_nat_debug_printf ("%s %s, %s (resume event thread)",
>  			  step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index 12a90eccb28..6033c406ec5 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -135,8 +135,8 @@ class linux_nat_target : public inf_ptrace_target
>    /* Methods that are meant to overridden by the concrete
>       arch-specific target instance.  */
>
> -  virtual void low_resume (ptid_t ptid, int step, enum gdb_signal sig)
> -  { inf_ptrace_target::resume (ptid, step, sig); }
> +  virtual void low_resume (ptid_t scope_ptid, int step, enum gdb_signal sig)
> +  { inf_ptrace_target::resume (scope_ptid, step, sig); }

Does it make sense to rename the parameter in low_resume?  AFAIU, this
is always a specific LWP ptid.

Maybe you meant to change the resume method, that one is above but
doesn't have parameter names.

>
>    virtual bool low_stopped_by_watchpoint ()
>    { return false; }
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 75d6bf3919d..484c1b88897 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -737,7 +737,7 @@ class remote_target : public process_stratum_target
>
>    char *append_resumption (char *p, char *endp,
>  			   ptid_t ptid, int step, gdb_signal siggnal);
> -  int remote_resume_with_vcont (ptid_t ptid, int step,
> +  int remote_resume_with_vcont (ptid_t scope_ptid, int step,
>  				gdb_signal siggnal);
>
>    thread_info *add_current_inferior_and_thread (const char *wait_status);
> @@ -6272,9 +6272,8 @@ remote_target::remote_vcont_probe ()
>     thread to be resumed is PTID; STEP and SIGGNAL indicate whether the
>     resumed thread should be single-stepped and/or signalled.  If PTID
>     equals minus_one_ptid, then all threads are resumed; if PTID
> -   represents a process, then all threads of the process are resumed;
> -   the thread to be stepped and/or signalled is given in the global
> -   INFERIOR_PTID.  */
> +   represents a process, then all threads of the process are
> +   resumed.  */
>
>  char *
>  remote_target::append_resumption (char *p, char *endp,
> @@ -6431,18 +6430,15 @@ remote_target::remote_resume_with_hc (ptid_t ptid, int step,
>    putpkt (buf);
>  }
>
> -/* Resume the remote inferior by using a "vCont" packet.  The thread
> -   to be resumed is PTID; STEP and SIGGNAL indicate whether the
> -   resumed thread should be single-stepped and/or signalled.  If PTID
> -   equals minus_one_ptid, then all threads are resumed; the thread to
> -   be stepped and/or signalled is given in the global INFERIOR_PTID.
> -   This function returns non-zero iff it resumes the inferior.
> +/* Resume the remote inferior by using a "vCont" packet.  SCOPE_PTID,
> +   STEP, and SIGGNAL have the same meaning as in target_resume.  This
> +   function returns non-zero iff it resumes the inferior.
>
>     This function issues a strict subset of all possible vCont commands
>     at the moment.  */
>
>  int
> -remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
> +remote_target::remote_resume_with_vcont (ptid_t scope_ptid, int step,
>  					 enum gdb_signal siggnal)

inferior_ptid is used in this method if step or siggnal are set.  I
think that use could easily be "pushed up" by adding another ptid_t
parameter here, conceptually "thread_concerned_by_step_or_signal".  The
caller would pass inferior_ptid.  I imagine that the same change could
eventually be done to the target_resume interface (which would be a lot
more work), but it would be a step towards that.  To be clear, I think
it could be done as a subsequent patch.

> @@ -6534,12 +6530,8 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
>       able to do vCont action coalescing.  */
>    if (target_is_non_stop_p () && ::execution_direction != EXEC_REVERSE)
>      {
> -      remote_thread_info *remote_thr;
> -
> -      if (minus_one_ptid == ptid || ptid.is_pid ())
> -	remote_thr = get_remote_thread_info (this, inferior_ptid);
> -      else
> -	remote_thr = get_remote_thread_info (this, ptid);
> +      remote_thread_info *remote_thr
> +	= get_remote_thread_info (inferior_thread ());

Not specific to your patch (it's pre-existing behavior), but just to
understand better: this code seems to ignore scope_ptid when in
non-stop.  So it assumes that the core will never use a wildcard on a
non-stop target.  Could the core technically (according to the
target_resume interface "contract") pass a wildcard to target_resume for
a non-stop target?

>
>        /* We don't expect the core to ask to resume an already resumed (from
>           its point of view) thread.  */
> @@ -6561,11 +6553,11 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
>    rs->last_resume_exec_dir = ::execution_direction;
>
>    /* Prefer vCont, and fallback to s/c/S/C, which use Hc.  */
> -  if (!remote_resume_with_vcont (ptid, step, siggnal))
> -    remote_resume_with_hc (ptid, step, siggnal);
> +  if (!remote_resume_with_vcont (scope_ptid, step, siggnal))
> +    remote_resume_with_hc (scope_ptid, step, siggnal);
>
>    /* Update resumed state tracked by the remote target.  */
> -  for (thread_info *tp : all_non_exited_threads (this, ptid))
> +  for (thread_info *tp : all_non_exited_threads (this, scope_ptid))
>      get_remote_thread_info (tp)->set_resumed ();
>
>    /* We've just told the target to resume.  The remote server will
> diff --git a/gdb/target.c b/gdb/target.c
> index 7d291fd4d0d..67e883851e9 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2655,21 +2655,26 @@ target_thread_info_to_thread_handle (struct thread_info *tip)
>  }
>
>  void
> -target_resume (ptid_t ptid, int step, enum gdb_signal signal)
> +target_resume (ptid_t scope_ptid, int step, enum gdb_signal signal)
>  {
>    process_stratum_target *curr_target = current_inferior ()->process_target ();
>    gdb_assert (!curr_target->commit_resumed_state);
>
> +  gdb_assert (inferior_ptid != null_ptid && scope_ptid != null_ptid);

I would suggest splitting this in two gdb_asserts.

> +  if (scope_ptid != minus_one_ptid && !scope_ptid.is_pid ())
> +    gdb_assert (scope_ptid == inferior_ptid);

Isn't what the gdb_assert just above here checks covered by the
"matches" assert just below?  ptid_t::matches is:

  constexpr bool matches (const ptid_t &filter) const
  {
    return (/* If filter represents any ptid, it's always a match.  */
	    filter == make_minus_one ()
	    /* If filter is only a pid, any ptid with that pid
	       matches.  */
	    || (filter.is_pid () && m_pid == filter.pid ())

	    /* Otherwise, this ptid only matches if it's exactly equal
	       to filter.  */
	    || *this == filter);
  }

This says: if the filter (scope_ptid) isn't minus_one_ptid and isn't a
pid-only ptid (same as what you check in your "if"), then the ptid
(inferior_ptid) must be exactly equal to the filter (scope_ptid) (same
as what you check in the assert above).

> +  gdb_assert (inferior_ptid.matches (scope_ptid));


> +
>    target_dcache_invalidate ();
>
> -  current_inferior ()->top_target ()->resume (ptid, step, signal);
> +  current_inferior ()->top_target ()->resume (scope_ptid, step, signal);
>
> -  registers_changed_ptid (curr_target, ptid);
> +  registers_changed_ptid (curr_target, scope_ptid);
>    /* We only set the internal executing state here.  The user/frontend
>       running state is set at a higher level.  This also clears the
>       thread's stop_pc as side effect.  */
> -  set_executing (curr_target, ptid, true);
> -  clear_inline_frame_state (curr_target, ptid);
> +  set_executing (curr_target, scope_ptid, true);
> +  clear_inline_frame_state (curr_target, scope_ptid);
>
>    if (target_can_async_p ())
>      target_async (1);
> diff --git a/gdb/target.h b/gdb/target.h
> index 093178af0bc..13753e5e8b9 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1471,23 +1471,32 @@ extern void target_detach (inferior *inf, int from_tty);
>
>  extern void target_disconnect (const char *, int);
>
> -/* Resume execution (or prepare for execution) of a target thread,
> -   process or all processes.  STEP says whether to hardware
> -   single-step or to run free; SIGGNAL is the signal to be given to
> -   the target, or GDB_SIGNAL_0 for no signal.  The caller may not pass
> -   GDB_SIGNAL_DEFAULT.  A specific PTID means `step/resume only this
> -   process id'.  A wildcard PTID (all threads, or all threads of
> -   process) means `step/resume INFERIOR_PTID, and let other threads
> -   (for which the wildcard PTID matches) resume with their
> -   'thread->suspend.stop_signal' signal (usually GDB_SIGNAL_0) if it
> -   is in "pass" state, or with no signal if in "no pass" state.
> +/* Resume execution (or prepare for execution) of the current thread
> +   (INFERIOR_PTID), while optionally letting other threads of the
> +   current process or all processes run free.
> +
> +   STEP says whether to hardware single-step the current thread or to
> +   let it run free; SIGNAL is the signal to be given to the current
> +   thread, or GDB_SIGNAL_0 for no signal.  The caller may not pass
> +   GDB_SIGNAL_DEFAULT.
> +
> +   SCOPE_PTID indicates the resumption scope.  I.e., which threads
> +   (other than the current) run free.  If resuming a single thread,
> +   SCOPE_PTID is the same thread as the current thread.  A wildcard
> +   SCOPE_PTID (all threads, or all threads of process) lets threads
> +   other then the current (for which the wildcard SCOPE_PTID matches)

then -> than

Simon