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

Pedro Alves
2016-10-26T15:39:20Z

On 02/17/2016 12:32 PM, Pedro Alves wrote:
> On 02/17/2016 11:46 AM, Luis Machado wrote:

>> The rest of the series looks good to me.
>
> Great, thanks!
>

This is the version that I pushed. The fork parent/child handling needed an
extra tweak, to consider the case of the fork event not having migrated out
of linux-low.c yet. This was exposed by sporadic failures in
the gdb.threads/forking-threads-plus-breakpoint.exp test.

From 5a04c4cf5df6d13596e79e7b84520cbe245a5a4d Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 26 Oct 2016 16:17:25 +0100
Subject: [PATCH] gdbserver: Leave already-vCont-resumed threads as they were

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-10-26 Pedro Alves <palves@redhat.com>

* linux-low.c (handle_extended_wait): Link parent/child fork
threads.
(linux_wait_1): Unlink them.
(linux_set_resume_request): Ignore resume requests for
already-resumed and unhandled fork child threads.
* linux-low.h (struct lwp_info) <fork_relative>: New field.
* server.c (in_queued_stop_replies_ptid, in_queued_stop_replies):
New functions.
(handle_v_requests) <vCont>: Don't call require_running.
* server.h (in_queued_stop_replies): New declaration.
---
gdb/gdbserver/ChangeLog | 13 +++++++++++
gdb/gdbserver/linux-low.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++
gdb/gdbserver/linux-low.h | 6 +++++
gdb/gdbserver/server.c | 33 +++++++++++++++++++++++++-
gdb/gdbserver/server.h | 4 ++++
5 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 437bb4c..1a9c4e5 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,16 @@
+2016-10-26 Pedro Alves <palves@redhat.com>
+
+ * linux-low.c (handle_extended_wait): Link parent/child fork
+ threads.
+ (linux_wait_1): Unlink them.
+ (linux_set_resume_request): Ignore resume requests for
+ already-resumed and unhandled fork child threads.
+ * linux-low.h (struct lwp_info) <fork_relative>: New field.
+ * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies):
+ New functions.
+ (handle_v_requests) <vCont>: Don't call require_running.
+ * server.h (in_queued_stop_replies): New declaration.
+
2016-10-24 Yao Qi <yao.qi@linaro.org>

PR server/20733
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 94c5bbe..f43ce7e 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -589,6 +589,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
event_lwp->status_pending_p = 1;
event_lwp->status_pending = wstat;

+ /* Link the threads until the parent event is passed on to
+ higher layers. */
+ event_lwp->fork_relative = child_lwp;
+ child_lwp->fork_relative = event_lwp;
+
/* If the parent thread is doing step-over with single-step
breakpoints, the list of single-step breakpoints are cloned
from the parent's. Remove them from the child process.
@@ -3853,6 +3858,15 @@ linux_wait_1 (ptid_t ptid,
{
/* If the reported event is an exit, fork, vfork or exec, let
GDB know. */
+
+ /* Break the unreported fork relationship chain. */
+ if (event_child->waitstatus.kind == TARGET_WAITKIND_FORKED
+ || event_child->waitstatus.kind == TARGET_WAITKIND_VFORKED)
+ {
+ event_child->fork_relative->fork_relative = NULL;
+ event_child->fork_relative = NULL;
+ }
+
*ourstatus = event_child->waitstatus;
/* Clear the event lwp's waitstatus since we handled it already. */
event_child->waitstatus.kind = TARGET_WAITKIND_IGNORE;
@@ -4654,6 +4668,51 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg)
continue;
}

+ /* Ignore (wildcard) resume requests for already-resumed
+ threads. */
+ 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;
+ }
+
+ /* Don't let wildcard resumes resume fork children that GDB
+ does not yet know are new fork children. */
+ if (lwp->fork_relative != NULL)
+ {
+ struct inferior_list_entry *inf, *tmp;
+ struct lwp_info *rel = lwp->fork_relative;
+
+ if (rel->status_pending_p
+ && (rel->waitstatus.kind == TARGET_WAITKIND_FORKED
+ || rel->waitstatus.kind == TARGET_WAITKIND_VFORKED))
+ {
+ if (debug_threads)
+ debug_printf ("not resuming LWP %ld: has queued stop reply\n",
+ 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;

diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 5057e66..476816d 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -301,6 +301,12 @@ struct lwp_info
information or exit status until it can be reported to GDB. */
struct target_waitstatus waitstatus;

+ /* A pointer to the fork child/parent relative. Valid only while
+ the parent fork event is not reported to higher layers. Used to
+ avoid wildcard vCont actions resuming a fork child before GDB is
+ notified about the parent's fork event. */
+ struct lwp_info *fork_relative;
+
/* When stopped is set, this is where the lwp last stopped, with
decr_pc_after_break already accounted for. If the LWP is
running, this is the address at which the lwp was resumed. */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 2996e19..3f9ff2b 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -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,
@@ -2947,7 +2979,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;
}
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 51b2191..f56c0f5 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -123,6 +123,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"
--
2.5.5