[PATCH] Slighly tweak and clarify target_resume's interface

[PATCH] Slighly tweak and clarify target_resume's interface

[Pedro Alves]

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:

  - 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
    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
implementation a little bit.  Other targets could do the same, but
they don't have to.

Change-Id: I02a2ec2ab3a3e9b191de1e9a84f55c17cab7daaf
---
 gdb/linux-nat.c | 28 +++++++++-------------------
 gdb/linux-nat.h |  4 ++--
 gdb/remote.c    | 46 +++++++++++++++++++---------------------------
 gdb/target.c    | 15 ++++++++++-----
 gdb/target.h    | 31 ++++++++++++++++++++-----------
 5 files changed, 60 insertions(+), 64 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index dff8d07d3f7..233d229cdb7 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1595,32 +1595,22 @@ resume_set_callback (struct lwp_info *lp)
 }
 
 void
-linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
+linux_nat_target::resume (ptid_t scope_ptid, int step, enum gdb_signal signo)
 {
   struct lwp_info *lp;
-  int resume_many;
 
   linux_nat_debug_printf ("Preparing to %s %s, %s, inferior_ptid %s",
 			  step ? "step" : "resume",
-			  ptid.to_string ().c_str (),
+			  scope_ptid.to_string ().c_str (),
 			  (signo != GDB_SIGNAL_0
 			   ? strsignal (gdb_signal_to_host (signo)) : "0"),
 			  inferior_ptid.to_string ().c_str ());
 
-  /* A specific PTID means `step only this process id'.  */
-  resume_many = (minus_one_ptid == ptid
-		 || ptid.is_pid ());
-
   /* Mark the lwps we're resuming as resumed and update their
      last_resume_kind to resume_continue.  */
-  iterate_over_lwps (ptid, resume_set_callback);
+  iterate_over_lwps (scope_ptid, resume_set_callback);
 
-  /* See if it's the current inferior that should be handled
-     specially.  */
-  if (resume_many)
-    lp = find_lwp_pid (inferior_ptid);
-  else
-    lp = find_lwp_pid (ptid);
+  lp = find_lwp_pid (inferior_ptid);
   gdb_assert (lp != NULL);
 
   /* Remember if we're stepping.  */
@@ -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 ())
+    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); }
 
   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)
 {
   struct remote_state *rs = get_remote_state ();
@@ -6468,7 +6464,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 
   p += xsnprintf (p, endp - p, "vCont");
 
-  if (ptid == magic_null_ptid)
+  if (scope_ptid == magic_null_ptid)
     {
       /* MAGIC_NULL_PTID means that we don't have any active threads,
 	 so we don't have any TID numbers the inferior will
@@ -6476,7 +6472,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 	 a TID.  */
       append_resumption (p, endp, minus_one_ptid, step, siggnal);
     }
-  else if (ptid == minus_one_ptid || ptid.is_pid ())
+  else if (scope_ptid == minus_one_ptid || scope_ptid.is_pid ())
     {
       /* Resume all threads (of all processes, or of a single
 	 process), with preference for INFERIOR_PTID.  This assumes
@@ -6490,15 +6486,15 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 
       /* Also pass down any pending signaled resumption for other
 	 threads not the current.  */
-      p = append_pending_thread_resumptions (p, endp, ptid);
+      p = append_pending_thread_resumptions (p, endp, scope_ptid);
 
       /* And continue others without a signal.  */
-      append_resumption (p, endp, ptid, /*step=*/ 0, GDB_SIGNAL_0);
+      append_resumption (p, endp, scope_ptid, /*step=*/ 0, GDB_SIGNAL_0);
     }
   else
     {
-      /* Scheduler locking; resume only PTID.  */
-      append_resumption (p, endp, ptid, step, siggnal);
+      /* Scheduler locking; resume only SCOPE_PTID.  */
+      append_resumption (p, endp, scope_ptid, step, siggnal);
     }
 
   gdb_assert (strlen (rs->buf.data ()) < get_remote_packet_size ());
@@ -6521,7 +6517,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 /* Tell the remote machine to resume.  */
 
 void
-remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
+remote_target::resume (ptid_t scope_ptid, int step, enum gdb_signal siggnal)
 {
   struct remote_state *rs = get_remote_state ();
 
@@ -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 ());
 
       /* 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);
+  if (scope_ptid != minus_one_ptid && !scope_ptid.is_pid ())
+    gdb_assert (scope_ptid == inferior_ptid);
+  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)
+   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.  Note neither STEP nor SIGNAL apply to any thread
+   other than the current.
 
    In order to efficiently handle batches of resumption requests,
    targets may implement this method such that it records the
    resumption request, but defers the actual resumption to the
    target_commit_resume method implementation.  See
    target_commit_resume below.  */
-extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
+extern void target_resume (ptid_t scope_ptid,
+			   int step, enum gdb_signal signal);
 
 /* Ensure that all resumed threads are committed to the target.
 

base-commit: 5355776935f1a69d897c4770209b6a3978f07ce1
-- 
2.26.2

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

[Simon Marchi]

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

[PATCH v2] Slightly tweak and clarify target_resume's interface

[Pedro Alves]

On 2022-04-21 17:22, Simon Marchi wrote:
> 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)?

Both.  The comment in the code mentions that.  I'd done it here too now.

> 
>>     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.

Fixed.

> 
>> @@ -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.

I thought the intent was clear as it was (hence why I inlined the resume_many variable),
but I'm fine with that too.  A comment helps regardless.  Here's what I have now:

  /* No use iterating unless we're resuming other threads.  */
  if (scope_ptid != lp->ptid)
    iterate_over_lwps (scope_ptid, [=] (struct lwp_info *info)
      {
	return linux_nat_resume_callback (info, lp);
      });

I also thought of making iterate_over_lwps itself not iterate if the
ptid is an lwp, but this seems like the only spot where that would make
a difference so I didn't bother.  Plus, we'd call the callback only
to have it do nothing (the second parameter is an "expect this lwp" parameter).

> 
>> +    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.

Oh yes, I didn't think that one through.  Reverted that change.

>>
>> -/* 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.

Yes.

> 
>> @@ -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?

Yeah, the code assumes that, and there's even a comment above this above,
but I don't see why couldn't the core pass in a wildcard in non-stop.  I think
gdbserver would handle it correctly, for instance.

I've added an assertion for now:

      /* There's actually nothing that says that the core can't
	 request a wildcard resume in non-stop mode, though.  It's
	 just that we know it doesn't currently, so we don't bother
	 with it.  */
      gdb_assert (scope_ptid == inferior_ptid);

>> 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));
> 

You're right.  I was just following the restrictions mentioned in
the new description comment more directly.  We can actually simplify further, like so:

  gdb_assert (inferior_ptid != null_ptid);
  gdb_assert (inferior_ptid.matches (scope_ptid));

if scope_ptid is null_ptid then then second assert fails too.

I've made the change.


>> +   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
> 

Fixed.

Here's the updated patch.  Retested on native and native-extended-gdbserver x86-64 GNU/Linux.

WDYT?

From 54eb4d6240454250cb1ed4471e8762cbb014f913 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 21 Apr 2022 14:20:36 +0100
Subject: [PATCH v2] Slightly tweak and clarify target_resume's interface

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:

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

  - otherwise, if PTID is a wildcard (all threads or all threads of
    process), the step/signal request is for 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
SCOPE_PTID in the remote and linux-nat targets, and simplifies their
implementation a little bit.  Other targets could do the same, but
they don't have to.

Change-Id: I02a2ec2ab3a3e9b191de1e9a84f55c17cab7daaf
---
 gdb/linux-nat.c | 29 ++++++++++-----------------
 gdb/remote.c    | 52 ++++++++++++++++++++++++-------------------------
 gdb/target.c    | 13 ++++++++-----
 gdb/target.h    | 31 ++++++++++++++++++-----------
 4 files changed, 63 insertions(+), 62 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index dff8d07d3f7..740cc0ddfc0 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1595,32 +1595,22 @@ resume_set_callback (struct lwp_info *lp)
 }
 
 void
-linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
+linux_nat_target::resume (ptid_t scope_ptid, int step, enum gdb_signal signo)
 {
   struct lwp_info *lp;
-  int resume_many;
 
   linux_nat_debug_printf ("Preparing to %s %s, %s, inferior_ptid %s",
 			  step ? "step" : "resume",
-			  ptid.to_string ().c_str (),
+			  scope_ptid.to_string ().c_str (),
 			  (signo != GDB_SIGNAL_0
 			   ? strsignal (gdb_signal_to_host (signo)) : "0"),
 			  inferior_ptid.to_string ().c_str ());
 
-  /* A specific PTID means `step only this process id'.  */
-  resume_many = (minus_one_ptid == ptid
-		 || ptid.is_pid ());
-
   /* Mark the lwps we're resuming as resumed and update their
      last_resume_kind to resume_continue.  */
-  iterate_over_lwps (ptid, resume_set_callback);
+  iterate_over_lwps (scope_ptid, resume_set_callback);
 
-  /* See if it's the current inferior that should be handled
-     specially.  */
-  if (resume_many)
-    lp = find_lwp_pid (inferior_ptid);
-  else
-    lp = find_lwp_pid (ptid);
+  lp = find_lwp_pid (inferior_ptid);
   gdb_assert (lp != NULL);
 
   /* Remember if we're stepping.  */
@@ -1669,11 +1659,12 @@ 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);
-			     });
+  /* No use iterating unless we're resuming other threads.  */
+  if (scope_ptid != lp->ptid)
+    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/remote.c b/gdb/remote.c
index 75d6bf3919d..ebbb7a8cd44 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)
 {
   struct remote_state *rs = get_remote_state ();
@@ -6468,7 +6464,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 
   p += xsnprintf (p, endp - p, "vCont");
 
-  if (ptid == magic_null_ptid)
+  if (scope_ptid == magic_null_ptid)
     {
       /* MAGIC_NULL_PTID means that we don't have any active threads,
 	 so we don't have any TID numbers the inferior will
@@ -6476,7 +6472,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 	 a TID.  */
       append_resumption (p, endp, minus_one_ptid, step, siggnal);
     }
-  else if (ptid == minus_one_ptid || ptid.is_pid ())
+  else if (scope_ptid == minus_one_ptid || scope_ptid.is_pid ())
     {
       /* Resume all threads (of all processes, or of a single
 	 process), with preference for INFERIOR_PTID.  This assumes
@@ -6490,15 +6486,15 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 
       /* Also pass down any pending signaled resumption for other
 	 threads not the current.  */
-      p = append_pending_thread_resumptions (p, endp, ptid);
+      p = append_pending_thread_resumptions (p, endp, scope_ptid);
 
       /* And continue others without a signal.  */
-      append_resumption (p, endp, ptid, /*step=*/ 0, GDB_SIGNAL_0);
+      append_resumption (p, endp, scope_ptid, /*step=*/ 0, GDB_SIGNAL_0);
     }
   else
     {
-      /* Scheduler locking; resume only PTID.  */
-      append_resumption (p, endp, ptid, step, siggnal);
+      /* Scheduler locking; resume only SCOPE_PTID.  */
+      append_resumption (p, endp, scope_ptid, step, siggnal);
     }
 
   gdb_assert (strlen (rs->buf.data ()) < get_remote_packet_size ());
@@ -6521,7 +6517,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid, int step,
 /* Tell the remote machine to resume.  */
 
 void
-remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
+remote_target::resume (ptid_t scope_ptid, int step, enum gdb_signal siggnal)
 {
   struct remote_state *rs = get_remote_state ();
 
@@ -6534,18 +6530,20 @@ 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 ());
 
       /* We don't expect the core to ask to resume an already resumed (from
          its point of view) thread.  */
       gdb_assert (remote_thr->get_resume_state () == resume_state::NOT_RESUMED);
 
       remote_thr->set_resumed_pending_vcont (step, siggnal);
+
+      /* There's actually nothing that says that the core can't
+	 request a wildcard resume in non-stop mode, though.  It's
+	 just that we know it doesn't currently, so we don't bother
+	 with it.  */
+      gdb_assert (scope_ptid == inferior_ptid);
       return;
     }
 
@@ -6561,11 +6559,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..1063f8086bc 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2655,21 +2655,24 @@ 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);
+  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..f77dbf05113 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 than the current (for which the wildcard SCOPE_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.  Note neither STEP nor SIGNAL apply to any thread
+   other than the current.
 
    In order to efficiently handle batches of resumption requests,
    targets may implement this method such that it records the
    resumption request, but defers the actual resumption to the
    target_commit_resume method implementation.  See
    target_commit_resume below.  */
-extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
+extern void target_resume (ptid_t scope_ptid,
+			   int step, enum gdb_signal signal);
 
 /* Ensure that all resumed threads are committed to the target.
 

base-commit: 5355776935f1a69d897c4770209b6a3978f07ce1
-- 
2.26.2

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

[Simon Marchi]

>>> @@ -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.
> 
> I thought the intent was clear as it was (hence why I inlined the resume_many variable),
> but I'm fine with that too.  A comment helps regardless.  Here's what I have now:
> 
>   /* No use iterating unless we're resuming other threads.  */
>   if (scope_ptid != lp->ptid)
>     iterate_over_lwps (scope_ptid, [=] (struct lwp_info *info)
>       {
> 	return linux_nat_resume_callback (info, lp);
>       });

That's fine with me (either way is fine, this is a subjective issue).

> I also thought of making iterate_over_lwps itself not iterate if the
> ptid is an lwp, but this seems like the only spot where that would make
> a difference so I didn't bother.  Plus, we'd call the callback only
> to have it do nothing (the second parameter is an "expect this lwp" parameter).

What do you mean not iterate?  We still want iterate_over_lwps to call
the callback on that specific LWP, don't we?  And since lwps are stored
in a linked list, it has to iterate.  I suppose it could break early
once it has found the lwp though.

> Here's the updated patch.  Retested on native and native-extended-gdbserver x86-64 GNU/Linux.
> 
> WDYT?

LGTM, thanks!

Simon

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

[Pedro Alves]

On 2022-04-28 21:38, Simon Marchi wrote:

>> I also thought of making iterate_over_lwps itself not iterate if the
>> ptid is an lwp, but this seems like the only spot where that would make
>> a difference so I didn't bother.  Plus, we'd call the callback only
>> to have it do nothing (the second parameter is an "expect this lwp" parameter).
> 
> What do you mean not iterate?  We still want iterate_over_lwps to call
> the callback on that specific LWP, don't we?  And since lwps are stored
> in a linked list, it has to iterate.  I suppose it could break early
> once it has found the lwp though.

Yeah, I was thinking of something like done in
0618ae41497998792701b72f34ea4859cf95aafc ("gdb: optimize all_matching_threads_iterator").
To avoid iterating to find the lwp, we'd add a hash map like done for threads, with
inferior::ptid_thread_map.

> 
>> Here's the updated patch.  Retested on native and native-extended-gdbserver x86-64 GNU/Linux.
>>
>> WDYT?
> 
> LGTM, thanks!

I'll check it in, then, thanks.