Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were

[Luis Machado <lgustavo@codesourcery.com>]

On 02/17/2016 12:44 AM, Pedro Alves wrote:
> Currently GDB never sends more than one action per vCont packet, when
> connected in non-stop mode.  A follow up patch will change that, and
> it exposed a gdbserver problem with the vCont handling.
>
> For example, this in non-stop mode:
>
>    => vCont;s:p1.1;c
>    <= OK
>
> Should be equivalent to:
>
>    => vCont;s:p1.1
>    <= OK
>    => vCont;c
>    <= OK
>
> But gdbserver currently doesn't handle this.  In the latter case,
> "vCont;c" makes gdbserver clobber the previous step request.  This
> patch fixes that.
>
> Note the server side must ignore resume actions for the thread that
> has a pending %Stopped notification (and any other threads with events
> pending), until GDB acks the notification with vStopped.  Otherwise,
> e.g., the following case is mishandled:
>
>   #1 => g  (or any other packet)
>   #2 <= [registers]
>   #3 <= %Stopped T05 thread:p1.2
>   #4 => vCont s:p1.1;c
>   #5 <= OK
>
> Above, the server must not resume thread p1.2 when it processes the
> vCont.  GDB can't know that p1.2 stopped until it acks the %Stopped
> notification.  (Otherwise it wouldn't send a default "c" action.)
>
> (The vCont documentation already specifies this.)
>
> Finally, special care must also be given to handling fork/vfork
> events.  A (v)fork event actually tells us that two processes stopped
> -- the parent and the child.  Until we follow the fork, we must not
> resume the child.  Therefore, if we have a pending fork follow, we
> must not send a global wildcard resume action (vCont;c).  We can still
> send process-wide wildcards though.
>
> (The comments above will be added as code comments to gdb in a follow
> up patch.)
>
> gdb/gdbserver/ChangeLog:
> 2016-02-16  Pedro Alves  <palves@redhat.com>
>
> 	* linux-low.c (linux_set_resume_request): Ignore resume requests
> 	for already-resumed threads.
> 	* server.c (in_queued_stop_replies_ptid, in_queued_stop_replies):
> 	New functions.
> 	* server.h (in_queued_stop_replies): New declaration.
> ---
>   gdb/gdbserver/linux-low.c | 27 +++++++++++++++++++++++++++
>   gdb/gdbserver/server.c    | 33 ++++++++++++++++++++++++++++++++-
>   gdb/gdbserver/server.h    |  4 ++++
>   3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 8b025bd..2cac4c0 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4465,6 +4465,33 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg)
>   	      continue;
>   	    }
>
> +	  /* Ignore (wildcard) resume requests for already-resumed
> +	     requests.  */

For already-resumed requests or threads? Looked a little confusing.

If you really meant "requests", then we may need to adjust the wording a 
bit, like "for requests that have already been acknowledged.".

The rest of the series looks good to me.