[4/5] gdbserver: Leave already-vCont-resumed threads as they were
Commit Message
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(-)
Comments
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.
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!
@@ -4465,6 +4465,33 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg)
continue;
}
+ /* Ignore (wildcard) resume requests for already-resumed
+ requests. */
+ if (r->resume[ndx].kind != resume_stop
+ && thread->last_resume_kind != resume_stop)
+ {
+ if (debug_threads)
+ debug_printf ("already %s LWP %ld at GDB's request\n",
+ (thread->last_resume_kind
+ == resume_step)
+ ? "stepping"
+ : "continuing",
+ lwpid_of (thread));
+ continue;
+ }
+
+ /* If the thread has a pending event that has already been
+ reported to GDBserver core, but GDB has not pulled the
+ event out of the vStopped queue yet, likewise, ignore the
+ (wildcard) resume request. */
+ if (in_queued_stop_replies (entry->id))
+ {
+ if (debug_threads)
+ debug_printf ("not resuming LWP %ld: has queued stop reply\n",
+ lwpid_of (thread));
+ continue;
+ }
+
lwp->resume = &r->resume[ndx];
thread->last_resume_kind = lwp->resume->kind;
@@ -193,6 +193,38 @@ vstop_notif_reply (struct notif_event *event, char *own_buf)
prepare_resume_reply (own_buf, vstop->ptid, &vstop->status);
}
+/* QUEUE_iterate callback helper for in_queued_stop_replies. */
+
+static int
+in_queued_stop_replies_ptid (QUEUE (notif_event_p) *q,
+ QUEUE_ITER (notif_event_p) *iter,
+ struct notif_event *event,
+ void *data)
+{
+ ptid_t filter_ptid = *(ptid_t *) data;
+ struct vstop_notif *vstop_event = (struct vstop_notif *) event;
+
+ if (ptid_match (vstop_event->ptid, filter_ptid))
+ return 0;
+
+ /* Don't resume fork children that GDB does not know about yet. */
+ if ((vstop_event->status.kind == TARGET_WAITKIND_FORKED
+ || vstop_event->status.kind == TARGET_WAITKIND_VFORKED)
+ && ptid_match (vstop_event->status.value.related_pid, filter_ptid))
+ return 0;
+
+ return 1;
+}
+
+/* See server.h. */
+
+int
+in_queued_stop_replies (ptid_t ptid)
+{
+ return !QUEUE_iterate (notif_event_p, notif_stop.queue,
+ in_queued_stop_replies_ptid, &ptid);
+}
+
struct notif_server notif_stop =
{
"vStopped", "Stop", NULL, vstop_notif_reply,
@@ -2949,7 +2981,6 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
if (startswith (own_buf, "vCont;"))
{
- require_running (own_buf);
handle_v_cont (own_buf);
return;
}
@@ -119,6 +119,10 @@ extern int handle_target_event (int err, gdb_client_data client_data);
/* Get rid of the currently pending stop replies that match PTID. */
extern void discard_queued_stop_replies (ptid_t ptid);
+/* Returns true if there's a pending stop reply that matches PTID in
+ the vStopped notifications queue. */
+extern int in_queued_stop_replies (ptid_t ptid);
+
#include "remote-utils.h"
#include "utils.h"