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

Pedro Alves
2016-02-17T12:32:33Z

On 02/17/2016 11:46 AM, Luis Machado wrote:
> 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.

Whoops, I meant "already-resumed threads". Fixed locally.

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

Great, thanks!

--
Pedro Alves