* [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions
@ 2016-02-17 2:44 Pedro Alves
2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw)
To: gdb-patches
Since:
[PATCH 00/18] Remote all-stop on top of non-stop
https://www.sourceware.org/ml/gdb-patches/2015-10/msg00213.html
The remote target supports "maint set target-non-stop on". However
that's still not the default.
I thought I'd first try to aggregate vCont packets, because with
"maint set target-non-stop on", we go from:
- All-stop / "maint set target-non-stop off"
(gdb) c
Continuing.
(...)
Sending packet: $vCont;c#a8..
to:
- All-stop / "maint set target-non-stop on"
(gdb) c
Continuing.
(...)
Sending packet: $vCont;c:p2c45.2c45#7c...Packet received: OK
Sending packet: $vCont;c:p2c45.2c4f#ad...Packet received: OK
Sending packet: $vCont;c:p2c45.2c50#78...Packet received: OK
... one packet per thread ...
After the series, we'll get back:
(gdb) c
Continuing.
(...)
Sending packet: $vCont;c#a8...Packet received: OK
(Note the "OK", showing that that was indeed an async vCont resume.)
along with other "wildcard" vCont packets like, "vCont;s:p1.1;c".
Also pushed to the users/palves/vcont-coalesce-actions branch.
Pedro Alves (5):
gdb: Clean up remote.c:remote_resume
gdb: Free inferior->priv when inferior exits
gdb/doc: Clarify vCont packet description
gdbserver: Leave already-vCont-resumed threads as they were
gdb: Coalesce/aggregate (async) vCont packets/actions
gdb/doc/gdb.texinfo | 32 ++-
gdb/gdbserver/linux-low.c | 27 +++
gdb/gdbserver/server.c | 33 ++-
gdb/gdbserver/server.h | 4 +
gdb/inferior.c | 3 +
gdb/inferior.h | 6 +
gdb/infrun.c | 8 +
gdb/record-btrace.c | 11 +
gdb/record-full.c | 11 +
gdb/remote.c | 566 ++++++++++++++++++++++++++++++++++++++++------
gdb/target-delegates.c | 26 +++
gdb/target.c | 29 +++
gdb/target.h | 46 +++-
13 files changed, 705 insertions(+), 97 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/5] gdb: Clean up remote.c:remote_resume
2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves
2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves
2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves
@ 2016-02-17 2:44 ` Pedro Alves
2016-02-17 11:45 ` Luis Machado
2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves
2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves
4 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw)
To: gdb-patches
Just some refactoring / TLC. Mainly split the old c/s/C/S packet
handling to a separate function.
gdb/ChangeLog:
2016-02-09 Pedro Alves <palves@redhat.com>
* remote.c (remote_resume_with_hc): New function, factored out
from ...
(remote_resume): ... this. Always try vCont first.
(remote_vcont_resume): Rename to ...
(remote_resume_with_vcont): ... this. Bail out if execution
direction is reverse.
---
gdb/remote.c | 113 ++++++++++++++++++++++++++++++++---------------------------
1 file changed, 62 insertions(+), 51 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index fa97e1e..60e2dda 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
return p;
}
+/* Set the target running, using the packets that use Hc
+ (c/s/C/S). */
+
+static void
+remote_resume_with_hc (struct target_ops *ops,
+ ptid_t ptid, int step, enum gdb_signal siggnal)
+{
+ struct remote_state *rs = get_remote_state ();
+ struct thread_info *thread;
+ char *buf;
+
+ rs->last_sent_signal = siggnal;
+ rs->last_sent_step = step;
+
+ /* The c/s/C/S resume packets use Hc, so set the continue
+ thread. */
+ if (ptid_equal (ptid, minus_one_ptid))
+ set_continue_thread (any_thread_ptid);
+ else
+ set_continue_thread (ptid);
+
+ ALL_NON_EXITED_THREADS (thread)
+ resume_clear_thread_private_info (thread);
+
+ buf = rs->buf;
+ if (execution_direction == EXEC_REVERSE)
+ {
+ /* We don't pass signals to the target in reverse exec mode. */
+ if (info_verbose && siggnal != GDB_SIGNAL_0)
+ warning (_(" - Can't pass signal %d to target in reverse: ignored."),
+ siggnal);
+
+ if (step && packet_support (PACKET_bs) == PACKET_DISABLE)
+ error (_("Remote reverse-step not supported."));
+ if (!step && packet_support (PACKET_bc) == PACKET_DISABLE)
+ error (_("Remote reverse-continue not supported."));
+
+ strcpy (buf, step ? "bs" : "bc");
+ }
+ else if (siggnal != GDB_SIGNAL_0)
+ {
+ buf[0] = step ? 'S' : 'C';
+ buf[1] = tohex (((int) siggnal >> 4) & 0xf);
+ buf[2] = tohex (((int) siggnal) & 0xf);
+ buf[3] = '\0';
+ }
+ else
+ strcpy (buf, step ? "s" : "c");
+
+ 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
@@ -5467,16 +5519,20 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
be stepped and/or signalled is given in the global INFERIOR_PTID.
This function returns non-zero iff it resumes the inferior.
- This function issues a strict subset of all possible vCont commands at the
- moment. */
+ This function issues a strict subset of all possible vCont commands
+ at the moment. */
static int
-remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
+remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal)
{
struct remote_state *rs = get_remote_state ();
char *p;
char *endp;
+ /* No reverse support (yet) for vCont. */
+ if (execution_direction == EXEC_REVERSE)
+ return 0;
+
if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN)
remote_vcont_probe (rs);
@@ -5548,8 +5604,6 @@ remote_resume (struct target_ops *ops,
ptid_t ptid, int step, enum gdb_signal siggnal)
{
struct remote_state *rs = get_remote_state ();
- char *buf;
- struct thread_info *thread;
/* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
(explained in remote-notif.c:handle_notification) so
@@ -5560,53 +5614,10 @@ remote_resume (struct target_ops *ops,
if (!target_is_non_stop_p ())
remote_notif_process (rs->notif_state, ¬if_client_stop);
- rs->last_sent_signal = siggnal;
- rs->last_sent_step = step;
-
- /* The vCont packet doesn't need to specify threads via Hc. */
- /* No reverse support (yet) for vCont. */
- if (execution_direction != EXEC_REVERSE)
- if (remote_vcont_resume (ptid, step, siggnal))
- goto done;
-
- /* All other supported resume packets do use Hc, so set the continue
- thread. */
- if (ptid_equal (ptid, minus_one_ptid))
- set_continue_thread (any_thread_ptid);
- else
- set_continue_thread (ptid);
-
- ALL_NON_EXITED_THREADS (thread)
- resume_clear_thread_private_info (thread);
-
- buf = rs->buf;
- if (execution_direction == EXEC_REVERSE)
- {
- /* We don't pass signals to the target in reverse exec mode. */
- if (info_verbose && siggnal != GDB_SIGNAL_0)
- warning (_(" - Can't pass signal %d to target in reverse: ignored."),
- siggnal);
-
- if (step && packet_support (PACKET_bs) == PACKET_DISABLE)
- error (_("Remote reverse-step not supported."));
- if (!step && packet_support (PACKET_bc) == PACKET_DISABLE)
- error (_("Remote reverse-continue not supported."));
-
- strcpy (buf, step ? "bs" : "bc");
- }
- else if (siggnal != GDB_SIGNAL_0)
- {
- buf[0] = step ? 'S' : 'C';
- buf[1] = tohex (((int) siggnal >> 4) & 0xf);
- buf[2] = tohex (((int) siggnal) & 0xf);
- buf[3] = '\0';
- }
- else
- strcpy (buf, step ? "s" : "c");
-
- putpkt (buf);
+ /* Prefer vCont, and fallback to s/c/S/C, which use Hc. */
+ if (!remote_resume_with_vcont (ptid, step, siggnal))
+ remote_resume_with_hc (ops, ptid, step, siggnal);
- done:
/* We are about to start executing the inferior, let's register it
with the event loop. NOTE: this is the one place where all the
execution commands end up. We could alternatively do this in each
--
1.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] gdb: Free inferior->priv when inferior exits
2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves
@ 2016-02-17 2:44 ` Pedro Alves
2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw)
To: gdb-patches
(Where "exits" includes being killed or detached.)
Nothing is clearing inferior->priv currently. This is a problem if we
change the inferior's process_stratum targets in a single debug
session. This field is currently only used by darwin-nat.c, but a
follow up patch will make remote.c use it too. Without the fix,
remote.c might end up mistaking the priv object allocated by
darwin-nat.c with its own.
(Found by inspection.)
gdb/ChangeLog:
2016-02-16 Pedro Alves <palves@redhat.com>
* inferior.c (exit_inferior_1): Free 'priv'.
---
gdb/inferior.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 45b3141..083dc63 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -255,6 +255,9 @@ exit_inferior_1 (struct inferior *inftoex, int silent)
inf->pid = 0;
inf->fake_pid_p = 0;
+ xfree (inf->priv);
+ inf->priv = NULL;
+
if (inf->vfork_parent != NULL)
{
inf->vfork_parent->vfork_child = NULL;
--
1.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/5] gdb/doc: Clarify vCont packet description
2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves
2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves
@ 2016-02-17 2:44 ` Pedro Alves
2016-02-17 15:56 ` Eli Zaretskii
2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw)
To: gdb-patches
Specifically, what happens with multiple actions that could match a
thread, and what happens when we get a vCont action that matches a
thread that was already running. E.g., what does:
"vCont;s:2"
"vCont;s:1;c"
mean for thread 2.
(Thread 2 continues stepping.)
gdb/doc/ChangeLog:
2016-02-16 Pedro Alves <palves@redhat.com>
* gdb.texinfo (Packets): Clarify vCont packets with multiple
actions that match a thread, and what happens when an action
matches a thread that is already running.
---
gdb/doc/gdb.texinfo | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9db234e..d936176 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -35321,13 +35321,17 @@ for success in non-stop mode (@pxref{Remote Non-Stop})
@cindex @samp{vCont} packet
@anchor{vCont packet}
Resume the inferior, specifying different actions for each thread.
-If an action is specified with no @var{thread-id}, then it is applied to any
-threads that don't have a specific action specified; if no default action is
-specified then other threads should remain stopped in all-stop mode and
-in their current state in non-stop mode.
-Specifying multiple
-default actions is an error; specifying no actions is also an error.
-Thread IDs are specified using the syntax described in @ref{thread-id syntax}.
+
+For each inferior thread, the leftmost action with a matching
+@var{thread-id} is applied. Threads that don't match any action
+remain in their current state. Thread IDs are specified using the
+syntax described in @ref{thread-id syntax}. If multiprocess
+extensions (@pxref{multiprocess extensions}) are supported, actions
+can be specified to match all threads in a process by using the
+@samp{p@var{pid}.-1} form of the @var{thread-id}. An action with no
+@var{thread-id} is called the default action and matches all threads.
+Specifying multiple default actions is an error; specifying no actions
+is also an error.
Currently supported actions are:
@@ -35372,11 +35376,17 @@ the corresponding stop reply should indicate that the thread has stopped with
signal @samp{0}, regardless of whether the target uses some other signal
as an implementation detail.
+The server must ignore @samp{c}, @samp{C}, @samp{s}, @samp{S}, and
+@samp{r} actions for threads that are already running. Conversely,
+the server must ignore @samp{t} actions for threads that are already
+stopped.
+
+@emph{Note:} In non-stop mode, a thread is considered running until
+@value{GDBN} acknowleges an asynchronous stop notification for it with
+the @samp{vStopped} packet (@pxref{Remote Non-Stop}).
+
The stub must support @samp{vCont} if it reports support for
-multiprocess extensions (@pxref{multiprocess extensions}). Note that in
-this case @samp{vCont} actions can be specified to apply to all threads
-in a process by using the @samp{p@var{pid}.-1} form of the
-@var{thread-id}.
+multiprocess extensions (@pxref{multiprocess extensions}).
Reply:
@xref{Stop Reply Packets}, for the reply specifications.
--
1.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions
2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves
` (2 preceding siblings ...)
2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves
@ 2016-02-17 2:51 ` Pedro Alves
2016-02-17 13:42 ` Luis Machado
2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves
4 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 2:51 UTC (permalink / raw)
To: gdb-patches
Currently, with "maint set target-non-stop on", that is, when gdb
connects with the non-stop/asynchronous variant of the remote
protocol, even with "set non-stop off", GDB always sends one vCont
packet per thread resumed. This patch makes GDB aggregate and
coalesce vCont packets, so we send vCont packets like "vCont;s:p1.1;c"
in non-stop mode too.
Basically, this is done by:
- Adding a new target method target_do_resume that is called after
calling target_resume one or more times. When resuming a batch of
threads, we'll only call target_do_resume once after calling
target_resume for all threads.
- Making the remote target defer sending the actual vCont packet to
target_do_resume.
Special care must be taken to avoid sending a vCont action with a
"wildcard" thread-id (all threads of process / all threads) when that
would resume threads/processes that should not be resumed. See
remote_do_resume comments for details.
Unlike all-stop's remote_resume implementation, this handles the case
of too many actions resulting in a too-big vCont packet, by flushing
the vCont packet and starting a new one.
E.g., imagining that the "c" action in:
vCont;s:1;c
overflows the packet buffer, we split the actions like:
vCont;s:1
vCont;c
Tested on x86_64 Fedora 20, with and without "maint set
target-non-stop on".
Also tested with a hack that makes remote_do_resume flush the vCont
packet after every action appended (which caught a few bugs).
gdb/ChangeLog:
2016-02-16 Pedro Alves <palves@redhat.com>
* inferior.h (ALL_NON_EXITED_INFERIORS): New macro.
* infrun.c (do_target_resume): Call target_do_resume.
(proceed): Defer target_do_resume while looping over threads,
resuming them. Call target_do_resume at the end.
* record-btrace.c (record_btrace_do_resume): New function.
(init_record_btrace_ops): Install it as to_do_resume method.
* record-full.c (record_full_do_resume): New function.
(record_full_wait_1): Call the beneath target's to_do_resume
method.
(init_record_full_ops): Install record_full_do_resume as
to_do_resume method.
* remote.c (struct private_thread_info) <last_resume_step,
last_resume_sig, vcont_resumed>: New fields.
(remote_add_thread): Set the new thread's vcont_resumed flag.
(demand_private_info): Delete.
(get_private_info_thread, get_private_info_ptid): New functions.
(remote_update_thread_list): Adjust.
(process_initial_stop_replies): Clear the thread's vcont_resumed
flag.
(remote_resume): If connected in non-stop mode, record the resume
request and return early.
(struct private_inferior): New.
(struct vcont_builder): New.
(vcont_builder_restart, vcont_builder_flush)
(vcont_builder_push_action): New functions.
(MAX_ACTION_SIZE): New macro.
(remote_do_resume): New function.
(thread_pending_fork_status, is_pending_fork_parent_thread): New
functions.
(check_pending_event_prevents_wildcard_vcont_callback)
(check_pending_events_prevent_wildcard_vcont): New functions.
(process_stop_reply): Adjust. Clear the thread's vcont_resumed
flag.
(init_remote_ops): Install remote_do_resume.
* target-delegates.c: Regenerate.
* target.c (defer_target_do_resume): New global.
(target_do_resume, make_cleanup_defer_target_do_resume): New
functions.
* target.h (struct target_ops) <to_do_resume>: New field.
(target_resume): Update comments.
(target_do_resume): New declaration.
---
gdb/inferior.h | 6 +
gdb/infrun.c | 8 +
gdb/record-btrace.c | 11 ++
gdb/record-full.c | 11 ++
gdb/remote.c | 453 ++++++++++++++++++++++++++++++++++++++++++++++---
gdb/target-delegates.c | 26 +++
gdb/target.c | 29 ++++
gdb/target.h | 46 +++--
8 files changed, 556 insertions(+), 34 deletions(-)
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 571d26a..42f6f6a 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -513,6 +513,12 @@ extern struct cleanup *save_current_inferior (void);
#define ALL_INFERIORS(I) \
for ((I) = inferior_list; (I); (I) = (I)->next)
+/* Traverse all non-exited inferiors. */
+
+#define ALL_NON_EXITED_INFERIORS(I) \
+ ALL_INFERIORS (I) \
+ if ((I)->pid != 0)
+
extern struct inferior *inferior_list;
/* Prune away automatically added inferiors that aren't required
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 15210c9..f72937c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
target_resume (resume_ptid, step, sig);
+
+ target_do_resume ();
}
/* Resume the inferior, but allow a QUIT. This is useful if the user
@@ -2977,6 +2979,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
struct execution_control_state ecss;
struct execution_control_state *ecs = &ecss;
struct cleanup *old_chain;
+ struct cleanup *defer_resume_cleanup;
int started;
/* If we're stopped at a fork/vfork, follow the branch set by the
@@ -3126,6 +3129,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
until the target stops again. */
tp->prev_pc = regcache_read_pc (regcache);
+ defer_resume_cleanup = make_cleanup_defer_target_do_resume ();
+
started = start_step_over ();
if (step_over_info_valid_p ())
@@ -3190,6 +3195,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
error (_("Command aborted."));
}
+ do_cleanups (defer_resume_cleanup);
+ target_do_resume ();
+
discard_cleanups (old_chain);
/* Tell the event loop to wait for it to stop. If the target
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 6a5ffa4..91d329b 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2122,6 +2122,16 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step,
}
}
+/* The to_do_resume method of target record-btrace. */
+
+static void
+record_btrace_do_resume (struct target_ops *ops)
+{
+ if ((execution_direction != EXEC_REVERSE)
+ && !record_btrace_is_replaying (ops, minus_one_ptid))
+ ops->beneath->to_do_resume (ops->beneath);
+}
+
/* Cancel resuming TP. */
static void
@@ -2847,6 +2857,7 @@ init_record_btrace_ops (void)
ops->to_get_unwinder = &record_btrace_to_get_unwinder;
ops->to_get_tailcall_unwinder = &record_btrace_to_get_tailcall_unwinder;
ops->to_resume = record_btrace_resume;
+ ops->to_do_resume = record_btrace_do_resume;
ops->to_wait = record_btrace_wait;
ops->to_stop = record_btrace_stop;
ops->to_update_thread_list = record_btrace_update_thread_list;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index e325869..b0955ef 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1010,6 +1010,15 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step,
target_async (1);
}
+/* "to_do_resume" method for process record target. */
+
+static void
+record_full_do_resume (struct target_ops *ops)
+{
+ if (!RECORD_FULL_IS_REPLAY)
+ ops->beneath->to_do_resume (ops->beneath);
+}
+
static int record_full_get_sig = 0;
/* SIGINT signal handler, registered by "to_wait" method. */
@@ -1180,6 +1189,7 @@ record_full_wait_1 (struct target_ops *ops,
"target beneath\n");
ops->beneath->to_resume (ops->beneath, ptid, step,
GDB_SIGNAL_0);
+ ops->beneath->to_do_resume (ops->beneath);
continue;
}
}
@@ -1956,6 +1966,7 @@ init_record_full_ops (void)
record_full_ops.to_close = record_full_close;
record_full_ops.to_async = record_full_async;
record_full_ops.to_resume = record_full_resume;
+ record_full_ops.to_do_resume = record_full_do_resume;
record_full_ops.to_wait = record_full_wait;
record_full_ops.to_disconnect = record_disconnect;
record_full_ops.to_detach = record_detach;
diff --git a/gdb/remote.c b/gdb/remote.c
index 60e2dda..453ce0f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -441,6 +441,24 @@ struct private_thread_info
/* This is set to the data address of the access causing the target
to stop for a watchpoint. */
CORE_ADDR watch_data_address;
+
+ /* Fields used by the vCont action coalescing implemented in
+ remote_resume / remote_do_resume. remote_resume stores each
+ thread's last resume request in these fields, so that a later
+ remote_do_resume knows which is the proper action for this thread
+ to include in the vCont packet. */
+
+ /* True if the last target_resume call for this thread was a step
+ request, false if a continue request. */
+ int last_resume_step;
+
+ /* The signal specified in the last target_resume call for this
+ thread. */
+ enum gdb_signal last_resume_sig;
+
+ /* Whether this thread was already vCont-resumed on the remote
+ side. */
+ int vcont_resumed;
};
static void
@@ -1797,6 +1815,9 @@ remote_add_inferior (int fake_pid_p, int pid, int attached,
return inf;
}
+static struct private_thread_info *
+ get_private_info_thread (struct thread_info *info);
+
/* Add thread PTID to GDB's thread list. Tag it as executing/running
according to RUNNING. */
@@ -1804,6 +1825,7 @@ static void
remote_add_thread (ptid_t ptid, int running)
{
struct remote_state *rs = get_remote_state ();
+ struct thread_info *thread;
/* GDB historically didn't pull threads in the initial connection
setup. If the remote target doesn't even have a concept of
@@ -1812,10 +1834,11 @@ remote_add_thread (ptid_t ptid, int running)
might be confusing to the user. Be silent then, preserving the
age old behavior. */
if (rs->starting_up)
- add_thread_silent (ptid);
+ thread = add_thread_silent (ptid);
else
- add_thread (ptid);
+ thread = add_thread (ptid);
+ get_private_info_thread (thread)->vcont_resumed = running;
set_executing (ptid, running);
set_running (ptid, running);
}
@@ -1904,25 +1927,40 @@ remote_notice_new_inferior (ptid_t currthread, int running)
}
}
-/* Return the private thread data, creating it if necessary. */
+/* Return THREAD's private thread data, creating it if necessary. */
static struct private_thread_info *
-demand_private_info (ptid_t ptid)
+get_private_info_thread (struct thread_info *thread)
{
- struct thread_info *info = find_thread_ptid (ptid);
-
- gdb_assert (info);
+ gdb_assert (thread != NULL);
- if (!info->priv)
+ if (thread->priv == NULL)
{
- info->priv = XNEW (struct private_thread_info);
- info->private_dtor = free_private_thread_info;
- info->priv->core = -1;
- info->priv->extra = NULL;
- info->priv->name = NULL;
+ struct private_thread_info *priv = XNEW (struct private_thread_info);
+
+ thread->private_dtor = free_private_thread_info;
+ thread->priv = priv;
+
+ priv->core = -1;
+ priv->extra = NULL;
+ priv->name = NULL;
+ priv->name = NULL;
+ priv->last_resume_step = 0;
+ priv->last_resume_sig = GDB_SIGNAL_0;
+ priv->vcont_resumed = 0;
}
- return info->priv;
+ return thread->priv;
+}
+
+/* Return PTID's private thread data, creating it if necessary. */
+
+static struct private_thread_info *
+get_private_info_ptid (ptid_t ptid)
+{
+ struct thread_info *info = find_thread_ptid (ptid);
+
+ return get_private_info_thread (info);
}
/* Call this function as a result of
@@ -3266,7 +3304,7 @@ remote_update_thread_list (struct target_ops *ops)
remote_notice_new_inferior (item->ptid, running);
- info = demand_private_info (item->ptid);
+ info = get_private_info_ptid (item->ptid);
info->core = item->core;
info->extra = item->extra;
item->extra = NULL;
@@ -3900,6 +3938,7 @@ process_initial_stop_replies (int from_tty)
set_executing (event_ptid, 0);
set_running (event_ptid, 0);
+ thread->priv->vcont_resumed = 0;
}
/* "Notice" the new inferiors before anything related to
@@ -5605,6 +5644,26 @@ remote_resume (struct target_ops *ops,
{
struct remote_state *rs = get_remote_state ();
+ /* When connected in non-stop mode, the core resumes threads
+ individually. Resuming remote threads directly in target_resume
+ would thus result in sending one packet per thread. Instead, to
+ minimize roundtrip latency, here we just store the resume
+ request; the actual remote resumption will be done in
+ target_do_resume / remote_do_resume, where we'll be able to do
+ vCont action coalescing. */
+ if (target_is_non_stop_p () && execution_direction != EXEC_REVERSE)
+ {
+ struct private_thread_info *remote_thr;
+
+ if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid))
+ remote_thr = get_private_info_ptid (inferior_ptid);
+ else
+ remote_thr = get_private_info_ptid (ptid);
+ remote_thr->last_resume_step = step;
+ remote_thr->last_resume_sig = siggnal;
+ return;
+ }
+
/* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
(explained in remote-notif.c:handle_notification) so
remote_notif_process is not called. We need find a place where
@@ -5638,6 +5697,284 @@ remote_resume (struct target_ops *ops,
if (!target_is_non_stop_p ())
rs->waiting_for_stop_reply = 1;
}
+
+static void check_pending_events_prevent_wildcard_vcont
+ (int *may_global_wildcard_vcont);
+static int is_pending_fork_parent_thread (struct thread_info *thread);
+
+/* Private per-inferior info for target remote processes. */
+
+struct private_inferior
+{
+ /* Whether we can send a wildcard vCont for this process. */
+ int may_wildcard_vcont;
+};
+
+/* Structure used to track the construction of a vCont packet in the
+ outgoing packet buffer. This is used to send multiple vCont
+ packets if we have more resumptions than would fit a single
+ packet. */
+
+struct vcont_builder
+{
+ /* Pointer to the first action. P points here if no resumption has
+ been appended yet. */
+ char *first_action;
+
+ /* Where the next resumption will be appended. */
+ char *p;
+
+ /* The end of the buffer. Must never write past this. */
+ char *endp;
+};
+
+/* Prepare the outgoing buffer for a new vCont packet. */
+
+static void
+vcont_builder_restart (struct vcont_builder *builder)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ builder->p = rs->buf;
+ builder->endp = rs->buf + get_remote_packet_size ();
+ builder->p += xsnprintf (builder->p, builder->endp - builder->p, "vCont");
+ builder->first_action = builder->p;
+}
+
+/* If the vCont packet being built has any action, send it to the
+ remote end. */
+
+static void
+vcont_builder_flush (struct vcont_builder *builder)
+{
+ struct remote_state *rs;
+
+ if (builder->p == builder->first_action)
+ return;
+
+ rs = get_remote_state ();
+ putpkt (rs->buf);
+ getpkt (&rs->buf, &rs->buf_size, 0);
+ if (strcmp (rs->buf, "OK") != 0)
+ error (_("Unexpected vCont reply in non-stop mode: %s"), rs->buf);
+}
+
+/* The largest resumption is range-stepping, with its two addresses.
+ This is more than sufficient. If a new, bigger resumption is
+ created, it'll quickly trigger a failed assertion in
+ append_resumption (and we'll just bump this). */
+#define MAX_ACTION_SIZE 200
+
+/* Append a new vCont action in the outgoing packet being built. If
+ the action doesn't fit the packet along with previous actions, push
+ what we've got so far to the remote end and start over a new vCont
+ packet (with the new action). */
+
+static void
+vcont_builder_push_action (struct vcont_builder *builder,
+ ptid_t ptid, int step, enum gdb_signal siggnal)
+{
+ char buf[MAX_ACTION_SIZE + 1];
+ char *endp;
+ size_t rsize;
+
+ endp = append_resumption (buf, buf + sizeof (buf),
+ ptid, step, siggnal);
+
+ /* Check whether this new action would fit in the vCont packet along
+ with previous actions. If not, send what we've got so far and
+ start a new vCont packet. */
+ rsize = endp - buf;
+ if (rsize > builder->endp - builder->p)
+ {
+ vcont_builder_flush (builder);
+ vcont_builder_restart (builder);
+
+ /* Should now fit. */
+ gdb_assert (rsize <= builder->endp - builder->p);
+ }
+
+ memcpy (builder->p, buf, rsize);
+ builder->p += rsize;
+ *builder->p = '\0';
+}
+
+/* to_do_resume implementation. */
+
+static void
+remote_do_resume (struct target_ops *ops)
+{
+ struct remote_state *rs = get_remote_state ();
+ struct inferior *inf;
+ struct thread_info *tp;
+ int any_process_wildcard;
+ int may_global_wildcard_vcont;
+ struct vcont_builder vcont_builder;
+
+ /* If connected in all-stop mode, we'd send the remote resume
+ request directly from remote_resume. Likewise if
+ reverse-debugging, as there are no defined vCont actions for
+ reverse execution. */
+ if (!target_is_non_stop_p () || execution_direction == EXEC_REVERSE)
+ return;
+
+ /* Try to send wildcard actions ("vCont;c" or "vCont;c:pPID.-1")
+ instead of resuming all threads of each process individually.
+ However, if any thread of a process must remain halted, we can't
+ send wildcard resumes and must send one action per thread.
+
+ Care must be taken to not resume threads/processes the server
+ side already told us are stopped, but the core doesn't know about
+ yet, because the events are still in the vStopped notification
+ queue. For example:
+
+ #1 => vCont s:p1.1;c
+ #2 <= OK
+ #3 <= %Stopped T05 p1.1
+ #4 => vStopped
+ #5 <= T05 p1.2
+ #6 => vStopped
+ #7 <= OK
+ #8 (infrun handles the stop for p1.1 and continues stepping)
+ #9 => vCont s:p1.1;c
+
+ The last vCont above would resume thread p1.2 by mistake, because
+ the server has no idea that the event for p1.2 had not been
+ handled yet.
+
+ The server side must similarly 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 p1.2
+ #4 => vCont s:p1.1;c
+ #5 <= OK
+
+ Above, the server must not resume thread p1.2. GDB can't know
+ that p1.2 stopped until it acks the %Stopped notification, and
+ since from GDB's perspective all threads should be running, it
+ sends a "c" action.
+
+ 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. */
+
+ /* Start by assuming a global wildcard (vCont;c) is possible. */
+ may_global_wildcard_vcont = 1;
+
+ /* And assume every process is individually wildcard-able too. */
+ ALL_NON_EXITED_INFERIORS (inf)
+ {
+ if (inf->priv == NULL)
+ inf->priv = XNEW (struct private_inferior);
+ inf->priv->may_wildcard_vcont = 1;
+ }
+
+ /* Check for any pending events (not reported or processed yet) and
+ disable process and global wildcard resumes appropriately. */
+ check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont);
+
+ ALL_NON_EXITED_THREADS (tp)
+ {
+ /* If a thread of a process is not meant to be resumed, then we
+ can't wildcard that process. */
+ if (!tp->executing)
+ {
+ tp->inf->priv->may_wildcard_vcont = 0;
+
+ /* And if we can't wildcard a process, we can't wildcard
+ everything either. */
+ may_global_wildcard_vcont = 0;
+ continue;
+ }
+
+ /* If a thread is the parent of an unfollowed fork, then we
+ can't do a global wildcard, as that would resume the fork
+ child. */
+ if (is_pending_fork_parent_thread (tp))
+ may_global_wildcard_vcont = 0;
+ }
+
+ /* Now let's build the vCont packet(s). Actions must be appended
+ from narrower to wider scopes (thread -> process -> global). If
+ we end up with too many actions for a single packet vcont_builder
+ flushes the current vCont packet to the remote side and starts a
+ new one. */
+ vcont_builder_restart (&vcont_builder);
+
+ /* Threads first. */
+ ALL_NON_EXITED_THREADS (tp)
+ {
+ struct private_thread_info *remote_thr = tp->priv;
+
+ if (!tp->executing || remote_thr->vcont_resumed)
+ continue;
+
+ gdb_assert (!thread_is_in_step_over_chain (tp));
+
+ if (!remote_thr->last_resume_step
+ && remote_thr->last_resume_sig == GDB_SIGNAL_0
+ && tp->inf->priv->may_wildcard_vcont)
+ {
+ /* We'll send a wildcard resume instead. */
+ remote_thr->vcont_resumed = 1;
+ continue;
+ }
+
+ vcont_builder_push_action (&vcont_builder, tp->ptid,
+ remote_thr->last_resume_step,
+ remote_thr->last_resume_sig);
+ remote_thr->vcont_resumed = 1;
+ }
+
+ /* Now check whether we can send any process-wide wildcard. This is
+ to avoid sending a global wildcard in the case nothing is
+ supposed to be resumed. */
+ any_process_wildcard = 0;
+
+ ALL_NON_EXITED_INFERIORS (inf)
+ {
+ if (inf->priv->may_wildcard_vcont)
+ {
+ any_process_wildcard = 1;
+ break;
+ }
+ }
+
+ if (any_process_wildcard)
+ {
+ /* If all processes are wildcard-able, then send a single "c"
+ action, otherwise, send an "all (-1) threads of process"
+ continue action for each running process, if any. */
+ if (may_global_wildcard_vcont)
+ {
+ vcont_builder_push_action (&vcont_builder, minus_one_ptid,
+ 0, GDB_SIGNAL_0);
+ }
+ else
+ {
+ ALL_NON_EXITED_INFERIORS (inf)
+ {
+ if (inf->priv->may_wildcard_vcont)
+ {
+ vcont_builder_push_action (&vcont_builder,
+ pid_to_ptid (inf->pid),
+ 0, GDB_SIGNAL_0);
+ }
+ }
+ }
+ }
+
+ vcont_builder_flush (&vcont_builder);
+}
+
\f
/* Set up the signal handler for SIGINT, while the target is
@@ -6120,7 +6457,7 @@ struct queue_iter_param
struct stop_reply *output;
};
-/* Determine if THREAD is a pending fork parent thread. ARG contains
+/* Determine if THREAD_PTID is a pending fork parent thread. ARG contains
the pid of the process that owns the threads we want to check, or
-1 if we want to check all threads. */
@@ -6138,6 +6475,29 @@ is_pending_fork_parent (struct target_waitstatus *ws, int event_pid,
return 0;
}
+/* Return the thread's pending status used to determine whether the
+ thread is a fork parent stopped at a fork event. */
+
+static struct target_waitstatus *
+thread_pending_fork_status (struct thread_info *thread)
+{
+ if (thread->suspend.waitstatus_pending_p)
+ return &thread->suspend.waitstatus;
+ else
+ return &thread->pending_follow;
+}
+
+/* Determine if THREAD is a pending fork parent thread. */
+
+static int
+is_pending_fork_parent_thread (struct thread_info *thread)
+{
+ struct target_waitstatus *ws = thread_pending_fork_status (thread);
+ int pid = -1;
+
+ return is_pending_fork_parent (ws, pid, thread->ptid);
+}
+
/* Check whether EVENT is a fork event, and if it is, remove the
fork child from the context list passed in DATA. */
@@ -6177,12 +6537,7 @@ remove_new_fork_children (struct threads_listing_context *context)
fork child threads from the CONTEXT list. */
ALL_NON_EXITED_THREADS (thread)
{
- struct target_waitstatus *ws;
-
- if (thread->suspend.waitstatus_pending_p)
- ws = &thread->suspend.waitstatus;
- else
- ws = &thread->pending_follow;
+ struct target_waitstatus *ws = thread_pending_fork_status (thread);
if (is_pending_fork_parent (ws, pid, thread->ptid))
{
@@ -6200,6 +6555,56 @@ remove_new_fork_children (struct threads_listing_context *context)
remove_child_of_pending_fork, ¶m);
}
+/* Check whether EVENT would prevent a global or process wildcard
+ vCont action. */
+
+static int
+check_pending_event_prevents_wildcard_vcont_callback
+ (QUEUE (stop_reply_p) *q,
+ QUEUE_ITER (stop_reply_p) *iter,
+ stop_reply_p event,
+ void *data)
+{
+ struct inferior *inf;
+ int *may_global_wildcard_vcont = (int *) data;
+
+ if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED
+ || event->ws.kind == TARGET_WAITKIND_NO_HISTORY)
+ return 1;
+
+ if (event->ws.kind == TARGET_WAITKIND_FORKED
+ || event->ws.kind == TARGET_WAITKIND_VFORKED)
+ *may_global_wildcard_vcont = 0;
+
+ inf = find_inferior_ptid (event->ptid);
+
+ /* This may be the first time we heard about this process.
+ Regardless, we must not do a global wildcard resume, otherwise
+ we'd resume this process too. */
+ *may_global_wildcard_vcont = 0;
+ if (inf != NULL)
+ inf->priv->may_wildcard_vcont = 0;
+
+ return 1;
+}
+
+/* Check whether any event pending in the vStopped queue would prevent
+ a global or process wildcard vCont action. Clear
+ *may_global_wildcard if we can't do a global wildcard (vCont;c),
+ and clear the event inferior's may_wildcard_vcont flag if we can do
+ a process-wide wildcard resume (vCont;c:pPID.-1). */
+
+static void
+check_pending_events_prevent_wildcard_vcont (int *may_global_wildcard)
+{
+ struct notif_client *notif = ¬if_client_stop;
+
+ remote_notif_get_pending_events (notif);
+ QUEUE_iterate (stop_reply_p, stop_reply_queue,
+ check_pending_event_prevents_wildcard_vcont_callback,
+ may_global_wildcard);
+}
+
/* Remove stop replies in the queue if its pid is equal to the given
inferior's pid. */
@@ -6830,10 +7235,11 @@ process_stop_reply (struct stop_reply *stop_reply,
}
remote_notice_new_inferior (ptid, 0);
- remote_thr = demand_private_info (ptid);
+ remote_thr = get_private_info_ptid (ptid);
remote_thr->core = stop_reply->core;
remote_thr->stop_reason = stop_reply->stop_reason;
remote_thr->watch_data_address = stop_reply->watch_data_address;
+ remote_thr->vcont_resumed = 0;
}
stop_reply_xfree (stop_reply);
@@ -13044,6 +13450,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_detach = remote_detach;
remote_ops.to_disconnect = remote_disconnect;
remote_ops.to_resume = remote_resume;
+ remote_ops.to_do_resume = remote_do_resume;
remote_ops.to_wait = remote_wait;
remote_ops.to_fetch_registers = remote_fetch_registers;
remote_ops.to_store_registers = remote_store_registers;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index e21746f..67a7281 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -109,6 +109,28 @@ debug_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal ar
fputs_unfiltered (")\n", gdb_stdlog);
}
+static void
+delegate_do_resume (struct target_ops *self)
+{
+ self = self->beneath;
+ self->to_do_resume (self);
+}
+
+static void
+tdefault_do_resume (struct target_ops *self)
+{
+}
+
+static void
+debug_do_resume (struct target_ops *self)
+{
+ fprintf_unfiltered (gdb_stdlog, "-> %s->to_do_resume (...)\n", debug_target.to_shortname);
+ debug_target.to_do_resume (&debug_target);
+ fprintf_unfiltered (gdb_stdlog, "<- %s->to_do_resume (", debug_target.to_shortname);
+ target_debug_print_struct_target_ops_p (&debug_target);
+ fputs_unfiltered (")\n", gdb_stdlog);
+}
+
static ptid_t
delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3)
{
@@ -4086,6 +4108,8 @@ install_delegators (struct target_ops *ops)
ops->to_disconnect = delegate_disconnect;
if (ops->to_resume == NULL)
ops->to_resume = delegate_resume;
+ if (ops->to_do_resume == NULL)
+ ops->to_do_resume = delegate_do_resume;
if (ops->to_wait == NULL)
ops->to_wait = delegate_wait;
if (ops->to_fetch_registers == NULL)
@@ -4389,6 +4413,7 @@ install_dummy_methods (struct target_ops *ops)
ops->to_detach = tdefault_detach;
ops->to_disconnect = tdefault_disconnect;
ops->to_resume = tdefault_resume;
+ ops->to_do_resume = tdefault_do_resume;
ops->to_wait = default_target_wait;
ops->to_fetch_registers = tdefault_fetch_registers;
ops->to_store_registers = tdefault_store_registers;
@@ -4545,6 +4570,7 @@ init_debug_target (struct target_ops *ops)
ops->to_detach = debug_detach;
ops->to_disconnect = debug_disconnect;
ops->to_resume = debug_resume;
+ ops->to_do_resume = debug_do_resume;
ops->to_wait = debug_wait;
ops->to_fetch_registers = debug_fetch_registers;
ops->to_store_registers = debug_store_registers;
diff --git a/gdb/target.c b/gdb/target.c
index a0d5937..3de6e10 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2298,6 +2298,35 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
clear_inline_frame_state (ptid);
}
+/* If true, target_do_resume is a nop. */
+
+static int defer_target_do_resume;
+
+/* See target.h. */
+
+void
+target_do_resume (void)
+{
+ struct target_ops *t;
+
+ if (defer_target_do_resume)
+ return;
+
+ current_target.to_do_resume (¤t_target);
+}
+
+/* See target.h. */
+
+struct cleanup *
+make_cleanup_defer_target_do_resume (void)
+{
+ struct cleanup *old_chain;
+
+ old_chain = make_cleanup_restore_integer (&defer_target_do_resume);
+ defer_target_do_resume = 1;
+ return old_chain;
+}
+
void
target_pass_signals (int numsigs, unsigned char *pass_signals)
{
diff --git a/gdb/target.h b/gdb/target.h
index 7c8513a..c40d1c7 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -461,6 +461,8 @@ struct target_ops
int TARGET_DEBUG_PRINTER (target_debug_print_step),
enum gdb_signal)
TARGET_DEFAULT_NORETURN (noprocess ());
+ void (*to_do_resume) (struct target_ops *)
+ TARGET_DEFAULT_IGNORE ();
ptid_t (*to_wait) (struct target_ops *,
ptid_t, struct target_waitstatus *,
int TARGET_DEBUG_PRINTER (target_debug_print_options))
@@ -1317,19 +1319,41 @@ extern void target_detach (const char *, int);
extern void target_disconnect (const char *, int);
-/* Resume execution of the target process PTID (or a group of
- threads). 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 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.
+
+ 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_do_resume method implementation. See target_do_resume
+ below. */
extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
+/* Commit a series of resumption requests previously prepared with
+ target_resume calls.
+
+ GDB always call target_do_resume after calling target_resume or
+ more times. A target may thus use this method in coordination with
+ the target_resume method to batch target-side resumption requests.
+ In that case, the target doesn't actually resume in its
+ target_resume implementation. Instead, it prepares the resumption
+ in target_resume, and defers the actual resumption to
+ target_do_resume. E.g., the remote target uses this to coalesce
+ multiple resumption requests in a single vCont packet. */
+extern void target_do_resume (void);
+
+/* Setup to defer target_do_resume calls, and return a cleanup that
+ reactivates target_do_resume, if it was previously active. */
+struct cleanup *make_cleanup_defer_target_do_resume (void);
+
/* Wait for process pid to do something. PTID = -1 to wait for any
pid to do something. Return pid of child, or -1 in case of error;
store status through argument pointer STATUS. Note that it is
--
1.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were
2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves
` (3 preceding siblings ...)
2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves
@ 2016-02-17 2:51 ` Pedro Alves
2016-02-17 11:46 ` Luis Machado
4 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 2:51 UTC (permalink / raw)
To: gdb-patches
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. */
+ 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;
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index ef715e7..660ee5b 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,
@@ -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;
}
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 3d78fb3..68a3670 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -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"
--
1.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume
2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves
@ 2016-02-17 11:45 ` Luis Machado
2016-02-17 12:32 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Luis Machado @ 2016-02-17 11:45 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
Just nits.
On 02/17/2016 12:44 AM, Pedro Alves wrote:
> Just some refactoring / TLC. Mainly split the old c/s/C/S packet
> handling to a separate function.
>
> gdb/ChangeLog:
> 2016-02-09 Pedro Alves <palves@redhat.com>
>
> * remote.c (remote_resume_with_hc): New function, factored out
> from ...
> (remote_resume): ... this. Always try vCont first.
> (remote_vcont_resume): Rename to ...
> (remote_resume_with_vcont): ... this. Bail out if execution
> direction is reverse.
> ---
> gdb/remote.c | 113 ++++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 62 insertions(+), 51 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index fa97e1e..60e2dda 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
> return p;
> }
>
> +/* Set the target running, using the packets that use Hc
> + (c/s/C/S). */
> +
> +static void
> +remote_resume_with_hc (struct target_ops *ops,
> + ptid_t ptid, int step, enum gdb_signal siggnal)
> +{
> + struct remote_state *rs = get_remote_state ();
> + struct thread_info *thread;
> + char *buf;
> +
> + rs->last_sent_signal = siggnal;
> + rs->last_sent_step = step;
> +
> + /* The c/s/C/S resume packets use Hc, so set the continue
> + thread. */
> + if (ptid_equal (ptid, minus_one_ptid))
> + set_continue_thread (any_thread_ptid);
> + else
> + set_continue_thread (ptid);
> +
> + ALL_NON_EXITED_THREADS (thread)
> + resume_clear_thread_private_info (thread);
> +
> + buf = rs->buf;
> + if (execution_direction == EXEC_REVERSE)
> + {
> + /* We don't pass signals to the target in reverse exec mode. */
> + if (info_verbose && siggnal != GDB_SIGNAL_0)
> + warning (_(" - Can't pass signal %d to target in reverse: ignored."),
> + siggnal);
> +
Even though it is existing code, this reads a bit odd.
Should we update it to "... in reverse execution: ..." maybe?
> + if (step && packet_support (PACKET_bs) == PACKET_DISABLE)
> + error (_("Remote reverse-step not supported."));
> + if (!step && packet_support (PACKET_bc) == PACKET_DISABLE)
> + error (_("Remote reverse-continue not supported."));
> +
> + strcpy (buf, step ? "bs" : "bc");
> + }
> + else if (siggnal != GDB_SIGNAL_0)
> + {
> + buf[0] = step ? 'S' : 'C';
> + buf[1] = tohex (((int) siggnal >> 4) & 0xf);
> + buf[2] = tohex (((int) siggnal) & 0xf);
> + buf[3] = '\0';
> + }
> + else
> + strcpy (buf, step ? "s" : "c");
> +
> + 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
> @@ -5467,16 +5519,20 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
> be stepped and/or signalled is given in the global INFERIOR_PTID.
> This function returns non-zero iff it resumes the inferior.
>
> - This function issues a strict subset of all possible vCont commands at the
> - moment. */
> + This function issues a strict subset of all possible vCont commands
> + at the moment. */
>
> static int
> -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
> +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal)
> {
> struct remote_state *rs = get_remote_state ();
> char *p;
> char *endp;
>
> + /* No reverse support (yet) for vCont. */
> + if (execution_direction == EXEC_REVERSE)
> + return 0;
> +
Same case as above. Also, do we need "(yet)"?
> if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN)
> remote_vcont_probe (rs);
>
> @@ -5548,8 +5604,6 @@ remote_resume (struct target_ops *ops,
> ptid_t ptid, int step, enum gdb_signal siggnal)
> {
> struct remote_state *rs = get_remote_state ();
> - char *buf;
> - struct thread_info *thread;
>
> /* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
> (explained in remote-notif.c:handle_notification) so
> @@ -5560,53 +5614,10 @@ remote_resume (struct target_ops *ops,
> if (!target_is_non_stop_p ())
> remote_notif_process (rs->notif_state, ¬if_client_stop);
>
> - rs->last_sent_signal = siggnal;
> - rs->last_sent_step = step;
> -
> - /* The vCont packet doesn't need to specify threads via Hc. */
> - /* No reverse support (yet) for vCont. */
> - if (execution_direction != EXEC_REVERSE)
> - if (remote_vcont_resume (ptid, step, siggnal))
> - goto done;
> -
> - /* All other supported resume packets do use Hc, so set the continue
> - thread. */
> - if (ptid_equal (ptid, minus_one_ptid))
> - set_continue_thread (any_thread_ptid);
> - else
> - set_continue_thread (ptid);
> -
> - ALL_NON_EXITED_THREADS (thread)
> - resume_clear_thread_private_info (thread);
> -
> - buf = rs->buf;
> - if (execution_direction == EXEC_REVERSE)
> - {
> - /* We don't pass signals to the target in reverse exec mode. */
> - if (info_verbose && siggnal != GDB_SIGNAL_0)
> - warning (_(" - Can't pass signal %d to target in reverse: ignored."),
> - siggnal);
> -
> - if (step && packet_support (PACKET_bs) == PACKET_DISABLE)
> - error (_("Remote reverse-step not supported."));
> - if (!step && packet_support (PACKET_bc) == PACKET_DISABLE)
> - error (_("Remote reverse-continue not supported."));
> -
> - strcpy (buf, step ? "bs" : "bc");
> - }
> - else if (siggnal != GDB_SIGNAL_0)
> - {
> - buf[0] = step ? 'S' : 'C';
> - buf[1] = tohex (((int) siggnal >> 4) & 0xf);
> - buf[2] = tohex (((int) siggnal) & 0xf);
> - buf[3] = '\0';
> - }
> - else
> - strcpy (buf, step ? "s" : "c");
> -
> - putpkt (buf);
> + /* Prefer vCont, and fallback to s/c/S/C, which use Hc. */
> + if (!remote_resume_with_vcont (ptid, step, siggnal))
> + remote_resume_with_hc (ops, ptid, step, siggnal);
>
> - done:
> /* We are about to start executing the inferior, let's register it
> with the event loop. NOTE: this is the one place where all the
> execution commands end up. We could alternatively do this in each
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were
2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves
@ 2016-02-17 11:46 ` Luis Machado
2016-02-17 12:32 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Luis Machado @ 2016-02-17 11:46 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were
2016-02-17 11:46 ` Luis Machado
@ 2016-02-17 12:32 ` Pedro Alves
2016-10-26 15:39 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 12:32 UTC (permalink / raw)
To: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume
2016-02-17 11:45 ` Luis Machado
@ 2016-02-17 12:32 ` Pedro Alves
2016-02-17 13:44 ` Luis Machado
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2016-02-17 12:32 UTC (permalink / raw)
To: Luis Machado, gdb-patches
On 02/17/2016 11:45 AM, Luis Machado wrote:
> Just nits.
>
> On 02/17/2016 12:44 AM, Pedro Alves wrote:
>> Just some refactoring / TLC. Mainly split the old c/s/C/S packet
>> handling to a separate function.
>>
>> gdb/ChangeLog:
>> 2016-02-09 Pedro Alves <palves@redhat.com>
>>
>> * remote.c (remote_resume_with_hc): New function, factored out
>> from ...
>> (remote_resume): ... this. Always try vCont first.
>> (remote_vcont_resume): Rename to ...
>> (remote_resume_with_vcont): ... this. Bail out if execution
>> direction is reverse.
>> ---
>> gdb/remote.c | 113 ++++++++++++++++++++++++++++++++---------------------------
>> 1 file changed, 62 insertions(+), 51 deletions(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index fa97e1e..60e2dda 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
>> return p;
>> }
>>
>> +/* Set the target running, using the packets that use Hc
>> + (c/s/C/S). */
>> +
>> +static void
>> +remote_resume_with_hc (struct target_ops *ops,
>> + ptid_t ptid, int step, enum gdb_signal siggnal)
>> +{
>> + struct remote_state *rs = get_remote_state ();
>> + struct thread_info *thread;
>> + char *buf;
>> +
>> + rs->last_sent_signal = siggnal;
>> + rs->last_sent_step = step;
>> +
>> + /* The c/s/C/S resume packets use Hc, so set the continue
>> + thread. */
>> + if (ptid_equal (ptid, minus_one_ptid))
>> + set_continue_thread (any_thread_ptid);
>> + else
>> + set_continue_thread (ptid);
>> +
>> + ALL_NON_EXITED_THREADS (thread)
>> + resume_clear_thread_private_info (thread);
>> +
>> + buf = rs->buf;
>> + if (execution_direction == EXEC_REVERSE)
>> + {
>> + /* We don't pass signals to the target in reverse exec mode. */
>> + if (info_verbose && siggnal != GDB_SIGNAL_0)
>> + warning (_(" - Can't pass signal %d to target in reverse: ignored."),
>> + siggnal);
>> +
>
> Even though it is existing code, this reads a bit odd.
(Also, I have no idea what that unusual leading " - " is there.)
>
> Should we update it to "... in reverse execution: ..." maybe?
Hmm, it'd still sound like a word is missing after execution,
to me.
I did 'grep reverse * | grep "\""' and found:
reverse.c: error (_("Already in reverse mode. Use '%s' or 'set exec-dir forward'."),
infcall.c: error (_("Cannot call functions in reverse mode."));
So maybe
"... in reverse mode: ..."
"... in reverse execution mode: ..."
?
I'd rather leave it be in this patch though, since it's
just a refactor with no UI change intended.
>> static int
>> -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
>> +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal)
>> {
>> struct remote_state *rs = get_remote_state ();
>> char *p;
>> char *endp;
>>
>> + /* No reverse support (yet) for vCont. */
>> + if (execution_direction == EXEC_REVERSE)
>> + return 0;
>> +
>
> Same case as above. Also, do we need "(yet)"?
How about:
/* There are no vCont reverse-execution actions defined. */
if (execution_direction == EXEC_REVERSE)
return 0;
?
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions
2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves
@ 2016-02-17 13:42 ` Luis Machado
2016-10-26 15:35 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Luis Machado @ 2016-02-17 13:42 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
I missed 5/5... Mostly nits, overall it looks sane to me.
On 02/17/2016 12:44 AM, Pedro Alves wrote:
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 15210c9..f72937c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
> target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
>
> target_resume (resume_ptid, step, sig);
> +
> + target_do_resume ();
> }
>
I'm looking at the sequence of function names and they don't look too clear.
do_target_resume/target_resume/target_do_resume.
Should we have better names for these functions? Ones that make it more
explicit what each function is doing and the fact that we are
potentially defering resumptions? Like "queue_resume_actions" for
target_resume or "commit_resumption_actions" for target_do_resume?
> @@ -6200,6 +6555,56 @@ remove_new_fork_children (struct threads_listing_context *context)
> remove_child_of_pending_fork, ¶m);
> }
>
> +/* Check whether EVENT would prevent a global or process wildcard
> + vCont action. */
> +
> +static int
> +check_pending_event_prevents_wildcard_vcont_callback
> + (QUEUE (stop_reply_p) *q,
> + QUEUE_ITER (stop_reply_p) *iter,
> + stop_reply_p event,
> + void *data)
> +{
> + struct inferior *inf;
> + int *may_global_wildcard_vcont = (int *) data;
> +
> + if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED
> + || event->ws.kind == TARGET_WAITKIND_NO_HISTORY)
> + return 1;
> +
> + if (event->ws.kind == TARGET_WAITKIND_FORKED
> + || event->ws.kind == TARGET_WAITKIND_VFORKED)
> + *may_global_wildcard_vcont = 0;
> +
> + inf = find_inferior_ptid (event->ptid);
> +
> + /* This may be the first time we heard about this process.
> + Regardless, we must not do a global wildcard resume, otherwise
> + we'd resume this process too. */
> + *may_global_wildcard_vcont = 0;
> + if (inf != NULL)
> + inf->priv->may_wildcard_vcont = 0;
> +
> + return 1;
> +}
> +
> +/* Check whether any event pending in the vStopped queue would prevent
> + a global or process wildcard vCont action. Clear
> + *may_global_wildcard if we can't do a global wildcard (vCont;c),
> + and clear the event inferior's may_wildcard_vcont flag if we can do
> + a process-wide wildcard resume (vCont;c:pPID.-1). */
> +
... inferior's may_wildcard_vcont flag if we can do ...
Can do or can't do?
> diff --git a/gdb/target.h b/gdb/target.h
> index 7c8513a..c40d1c7 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -461,6 +461,8 @@ struct target_ops
> int TARGET_DEBUG_PRINTER (target_debug_print_step),
> enum gdb_signal)
> TARGET_DEFAULT_NORETURN (noprocess ());
> + void (*to_do_resume) (struct target_ops *)
> + TARGET_DEFAULT_IGNORE ();
> ptid_t (*to_wait) (struct target_ops *,
> ptid_t, struct target_waitstatus *,
> int TARGET_DEBUG_PRINTER (target_debug_print_options))
> @@ -1317,19 +1319,41 @@ extern void target_detach (const char *, int);
>
> extern void target_disconnect (const char *, int);
>
> -/* Resume execution of the target process PTID (or a group of
> - threads). 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 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.
> +
> + 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_do_resume method implementation. See target_do_resume
> + below. */
> extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
>
> +/* Commit a series of resumption requests previously prepared with
> + target_resume calls.
> +
> + GDB always call target_do_resume after calling target_resume or
> + more times. A target may thus use this method in coordination with
"GDB always calls ... target_resume one or more times."?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume
2016-02-17 12:32 ` Pedro Alves
@ 2016-02-17 13:44 ` Luis Machado
0 siblings, 0 replies; 15+ messages in thread
From: Luis Machado @ 2016-02-17 13:44 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 02/17/2016 10:32 AM, Pedro Alves wrote:
> On 02/17/2016 11:45 AM, Luis Machado wrote:
>> Just nits.
>>
>> On 02/17/2016 12:44 AM, Pedro Alves wrote:
>>> Just some refactoring / TLC. Mainly split the old c/s/C/S packet
>>> handling to a separate function.
>>>
>>> gdb/ChangeLog:
>>> 2016-02-09 Pedro Alves <palves@redhat.com>
>>>
>>> * remote.c (remote_resume_with_hc): New function, factored out
>>> from ...
>>> (remote_resume): ... this. Always try vCont first.
>>> (remote_vcont_resume): Rename to ...
>>> (remote_resume_with_vcont): ... this. Bail out if execution
>>> direction is reverse.
>>> ---
>>> gdb/remote.c | 113 ++++++++++++++++++++++++++++++++---------------------------
>>> 1 file changed, 62 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index fa97e1e..60e2dda 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
>>> return p;
>>> }
>>>
>>> +/* Set the target running, using the packets that use Hc
>>> + (c/s/C/S). */
>>> +
>>> +static void
>>> +remote_resume_with_hc (struct target_ops *ops,
>>> + ptid_t ptid, int step, enum gdb_signal siggnal)
>>> +{
>>> + struct remote_state *rs = get_remote_state ();
>>> + struct thread_info *thread;
>>> + char *buf;
>>> +
>>> + rs->last_sent_signal = siggnal;
>>> + rs->last_sent_step = step;
>>> +
>>> + /* The c/s/C/S resume packets use Hc, so set the continue
>>> + thread. */
>>> + if (ptid_equal (ptid, minus_one_ptid))
>>> + set_continue_thread (any_thread_ptid);
>>> + else
>>> + set_continue_thread (ptid);
>>> +
>>> + ALL_NON_EXITED_THREADS (thread)
>>> + resume_clear_thread_private_info (thread);
>>> +
>>> + buf = rs->buf;
>>> + if (execution_direction == EXEC_REVERSE)
>>> + {
>>> + /* We don't pass signals to the target in reverse exec mode. */
>>> + if (info_verbose && siggnal != GDB_SIGNAL_0)
>>> + warning (_(" - Can't pass signal %d to target in reverse: ignored."),
>>> + siggnal);
>>> +
>>
>> Even though it is existing code, this reads a bit odd.
>
> (Also, I have no idea what that unusual leading " - " is there.)
>
>>
>> Should we update it to "... in reverse execution: ..." maybe?
>
> Hmm, it'd still sound like a word is missing after execution,
> to me.
>
> I did 'grep reverse * | grep "\""' and found:
>
> reverse.c: error (_("Already in reverse mode. Use '%s' or 'set exec-dir forward'."),
> infcall.c: error (_("Cannot call functions in reverse mode."));
>
> So maybe
>
> "... in reverse mode: ..."
> "... in reverse execution mode: ..."
>
> ?
>
> I'd rather leave it be in this patch though, since it's
> just a refactor with no UI change intended.
>
"... in reverse mode: ..." sounds good. I'm fine with leaving this be
though.
>>> static int
>>> -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
>>> +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal)
>>> {
>>> struct remote_state *rs = get_remote_state ();
>>> char *p;
>>> char *endp;
>>>
>>> + /* No reverse support (yet) for vCont. */
>>> + if (execution_direction == EXEC_REVERSE)
>>> + return 0;
>>> +
>>
>> Same case as above. Also, do we need "(yet)"?
>
> How about:
>
> /* There are no vCont reverse-execution actions defined. */
> if (execution_direction == EXEC_REVERSE)
> return 0;
>
> ?
That's good.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] gdb/doc: Clarify vCont packet description
2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves
@ 2016-02-17 15:56 ` Eli Zaretskii
0 siblings, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2016-02-17 15:56 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 17 Feb 2016 02:44:49 +0000
>
> gdb/doc/ChangeLog:
> 2016-02-16 Pedro Alves <palves@redhat.com>
>
> * gdb.texinfo (Packets): Clarify vCont packets with multiple
> actions that match a thread, and what happens when an action
> matches a thread that is already running.
OK, thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions
2016-02-17 13:42 ` Luis Machado
@ 2016-10-26 15:35 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2016-10-26 15:35 UTC (permalink / raw)
To: Luis Machado, gdb-patches
Hi there. I've finally got back to looking at this...
On 02/17/2016 01:42 PM, Luis Machado wrote:
> I missed 5/5... Mostly nits, overall it looks sane to me.
>
> On 02/17/2016 12:44 AM, Pedro Alves wrote:
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 15210c9..f72937c 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step,
>> enum gdb_signal sig)
>> target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
>>
>> target_resume (resume_ptid, step, sig);
>> +
>> + target_do_resume ();
>> }
>>
>
> I'm looking at the sequence of function names and they don't look too
> clear.
>
> do_target_resume/target_resume/target_do_resume.
>
> Should we have better names for these functions? Ones that make it more
> explicit what each function is doing and the fact that we are
> potentially defering resumptions? Like "queue_resume_actions" for
> target_resume or "commit_resumption_actions" for target_do_resume?
You're right. I'm not looking forward to changing target_resume,
since that's a target method implemented by all targets.
But I changed target_do_resume to target_commit_resume.
>> +/* Check whether any event pending in the vStopped queue would prevent
>> + a global or process wildcard vCont action. Clear
>> + *may_global_wildcard if we can't do a global wildcard (vCont;c),
>> + and clear the event inferior's may_wildcard_vcont flag if we can do
>> + a process-wide wildcard resume (vCont;c:pPID.-1). */
>> +
>
> ... inferior's may_wildcard_vcont flag if we can do ...
>
> Can do or can't do?
Can't. Fixed.
>> +/* Commit a series of resumption requests previously prepared with
>> + target_resume calls.
>> +
>> + GDB always call target_do_resume after calling target_resume or
>> + more times. A target may thus use this method in coordination with
>
>
> "GDB always calls ... target_resume one or more times."?
Fixed too. Below's what I pushed to master.
This added a new cleanup, which would better done with a
scoped_restore nowadays. I'll fix that as a follow up.
From 85ad3aaf403d2104c82010494d3d4a93a36e2e6f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 26 Oct 2016 11:08:28 +0100
Subject: [PATCH] gdb: Coalesce/aggregate (async) vCont packets/actions
Currently, with "maint set target-non-stop on", that is, when gdb
connects with the non-stop/asynchronous variant of the remote
protocol, even with "set non-stop off", GDB always sends one vCont
packet per thread resumed. This patch makes GDB aggregate and
coalesce vCont packets, so we send vCont packets like "vCont;s:p1.1;c"
in non-stop mode too.
Basically, this is done by:
- Adding a new target method target_commit_resume that is called
after calling target_resume one or more times. When resuming a
batch of threads, we'll only call target_commit_resume once after
calling target_resume for all threads.
- Making the remote target defer sending the actual vCont packet to
target_commit_resume.
Special care must be taken to avoid sending a vCont action with a
"wildcard" thread-id (all threads of process / all threads) when that
would resume threads/processes that should not be resumed. See
remote_commit_resume comments for details.
Unlike all-stop's remote_resume implementation, this handles the case
of too many actions resulting in a too-big vCont packet, by flushing
the vCont packet and starting a new one.
E.g., imagining that the "c" action in:
vCont;s:1;c
overflows the packet buffer, we split the actions like:
vCont;s:1
vCont;c
Tested on x86_64 Fedora 20, with and without "maint set
target-non-stop on".
Also tested with a hack that makes remote_commit_resume flush the vCont
packet after every action appended (which caught a few bugs).
gdb/ChangeLog:
2016-10-26 Pedro Alves <palves@redhat.com>
* inferior.h (ALL_NON_EXITED_INFERIORS): New macro.
* infrun.c (do_target_resume): Call target_commit_resume.
(proceed): Defer target_commit_resume while looping over threads,
resuming them. Call target_commit_resume at the end.
* record-btrace.c (record_btrace_commit_resume): New function.
(init_record_btrace_ops): Install it as to_commit_resume method.
* record-full.c (record_full_commit_resume): New function.
(record_full_wait_1): Call the beneath target's to_commit_resume
method.
(init_record_full_ops): Install record_full_commit_resume as
to_commit_resume method.
* remote.c (struct private_thread_info) <last_resume_step,
last_resume_sig, vcont_resumed>: New fields.
(remote_add_thread): Set the new thread's vcont_resumed flag.
(demand_private_info): Delete.
(get_private_info_thread, get_private_info_ptid): New functions.
(remote_update_thread_list): Adjust.
(process_initial_stop_replies): Clear the thread's vcont_resumed
flag.
(remote_resume): If connected in non-stop mode, record the resume
request and return early.
(struct private_inferior): New.
(struct vcont_builder): New.
(vcont_builder_restart, vcont_builder_flush)
(vcont_builder_push_action): New functions.
(MAX_ACTION_SIZE): New macro.
(remote_commit_resume): New function.
(thread_pending_fork_status, is_pending_fork_parent_thread): New
functions.
(check_pending_event_prevents_wildcard_vcont_callback)
(check_pending_events_prevent_wildcard_vcont): New functions.
(process_stop_reply): Adjust. Clear the thread's vcont_resumed
flag.
(init_remote_ops): Install remote_commit_resume.
* target-delegates.c: Regenerate.
* target.c (defer_target_commit_resume): New global.
(target_commit_resume, make_cleanup_defer_target_commit_resume):
New functions.
* target.h (struct target_ops) <to_commit_resume>: New field.
(target_resume): Update comments.
(target_commit_resume): New declaration.
---
gdb/ChangeLog | 44 +++++
gdb/inferior.h | 6 +
gdb/infrun.c | 8 +
gdb/record-btrace.c | 11 ++
gdb/record-full.c | 11 ++
gdb/remote.c | 452 ++++++++++++++++++++++++++++++++++++++++++++++---
gdb/target-delegates.c | 26 +++
gdb/target.c | 28 +++
gdb/target.h | 47 +++--
9 files changed, 599 insertions(+), 34 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f8296b6..6e24938 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,49 @@
2016-10-26 Pedro Alves <palves@redhat.com>
+ * inferior.h (ALL_NON_EXITED_INFERIORS): New macro.
+ * infrun.c (do_target_resume): Call target_commit_resume.
+ (proceed): Defer target_commit_resume while looping over threads,
+ resuming them. Call target_commit_resume at the end.
+ * record-btrace.c (record_btrace_commit_resume): New function.
+ (init_record_btrace_ops): Install it as to_commit_resume method.
+ * record-full.c (record_full_commit_resume): New function.
+ (record_full_wait_1): Call the beneath target's to_commit_resume
+ method.
+ (init_record_full_ops): Install record_full_commit_resume as
+ to_commit_resume method.
+ * remote.c (struct private_thread_info) <last_resume_step,
+ last_resume_sig, vcont_resumed>: New fields.
+ (remote_add_thread): Set the new thread's vcont_resumed flag.
+ (demand_private_info): Delete.
+ (get_private_info_thread, get_private_info_ptid): New functions.
+ (remote_update_thread_list): Adjust.
+ (process_initial_stop_replies): Clear the thread's vcont_resumed
+ flag.
+ (remote_resume): If connected in non-stop mode, record the resume
+ request and return early.
+ (struct private_inferior): New.
+ (struct vcont_builder): New.
+ (vcont_builder_restart, vcont_builder_flush)
+ (vcont_builder_push_action): New functions.
+ (MAX_ACTION_SIZE): New macro.
+ (remote_commit_resume): New function.
+ (thread_pending_fork_status, is_pending_fork_parent_thread): New
+ functions.
+ (check_pending_event_prevents_wildcard_vcont_callback)
+ (check_pending_events_prevent_wildcard_vcont): New functions.
+ (process_stop_reply): Adjust. Clear the thread's vcont_resumed
+ flag.
+ (init_remote_ops): Install remote_commit_resume.
+ * target-delegates.c: Regenerate.
+ * target.c (defer_target_commit_resume): New global.
+ (target_commit_resume, make_cleanup_defer_target_commit_resume):
+ New functions.
+ * target.h (struct target_ops) <to_commit_resume>: New field.
+ (target_resume): Update comments.
+ (target_commit_resume): New declaration.
+
+2016-10-26 Pedro Alves <palves@redhat.com>
+
* inferior.c (exit_inferior_1): Free 'priv'.
2016-10-26 Pedro Alves <palves@redhat.com>
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 54c6f65..300cc10 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -513,6 +513,12 @@ extern struct cleanup *save_current_inferior (void);
#define ALL_INFERIORS(I) \
for ((I) = inferior_list; (I); (I) = (I)->next)
+/* Traverse all non-exited inferiors. */
+
+#define ALL_NON_EXITED_INFERIORS(I) \
+ ALL_INFERIORS (I) \
+ if ((I)->pid != 0)
+
extern struct inferior *inferior_list;
/* Prune away automatically added inferiors that aren't required
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 00bba16..5e62472 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2366,6 +2366,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
target_resume (resume_ptid, step, sig);
+
+ target_commit_resume ();
}
/* Resume the inferior, but allow a QUIT. This is useful if the user
@@ -2984,6 +2986,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
struct execution_control_state ecss;
struct execution_control_state *ecs = &ecss;
struct cleanup *old_chain;
+ struct cleanup *defer_resume_cleanup;
int started;
/* If we're stopped at a fork/vfork, follow the branch set by the
@@ -3125,6 +3128,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
until the target stops again. */
tp->prev_pc = regcache_read_pc (regcache);
+ defer_resume_cleanup = make_cleanup_defer_target_commit_resume ();
+
started = start_step_over ();
if (step_over_info_valid_p ())
@@ -3189,6 +3194,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
error (_("Command aborted."));
}
+ do_cleanups (defer_resume_cleanup);
+ target_commit_resume ();
+
discard_cleanups (old_chain);
/* Tell the event loop to wait for it to stop. If the target
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 257d0b0..4808129 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2150,6 +2150,16 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step,
}
}
+/* The to_commit_resume method of target record-btrace. */
+
+static void
+record_btrace_commit_resume (struct target_ops *ops)
+{
+ if ((execution_direction != EXEC_REVERSE)
+ && !record_btrace_is_replaying (ops, minus_one_ptid))
+ ops->beneath->to_commit_resume (ops->beneath);
+}
+
/* Cancel resuming TP. */
static void
@@ -2875,6 +2885,7 @@ init_record_btrace_ops (void)
ops->to_get_unwinder = &record_btrace_to_get_unwinder;
ops->to_get_tailcall_unwinder = &record_btrace_to_get_tailcall_unwinder;
ops->to_resume = record_btrace_resume;
+ ops->to_commit_resume = record_btrace_commit_resume;
ops->to_wait = record_btrace_wait;
ops->to_stop = record_btrace_stop;
ops->to_update_thread_list = record_btrace_update_thread_list;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index e4dd55b..50f235d 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1002,6 +1002,15 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step,
target_async (1);
}
+/* "to_commit_resume" method for process record target. */
+
+static void
+record_full_commit_resume (struct target_ops *ops)
+{
+ if (!RECORD_FULL_IS_REPLAY)
+ ops->beneath->to_commit_resume (ops->beneath);
+}
+
static int record_full_get_sig = 0;
/* SIGINT signal handler, registered by "to_wait" method. */
@@ -1172,6 +1181,7 @@ record_full_wait_1 (struct target_ops *ops,
"target beneath\n");
ops->beneath->to_resume (ops->beneath, ptid, step,
GDB_SIGNAL_0);
+ ops->beneath->to_commit_resume (ops->beneath);
continue;
}
}
@@ -1975,6 +1985,7 @@ init_record_full_ops (void)
record_full_ops.to_close = record_full_close;
record_full_ops.to_async = record_full_async;
record_full_ops.to_resume = record_full_resume;
+ record_full_ops.to_commit_resume = record_full_commit_resume;
record_full_ops.to_wait = record_full_wait;
record_full_ops.to_disconnect = record_disconnect;
record_full_ops.to_detach = record_detach;
diff --git a/gdb/remote.c b/gdb/remote.c
index 96f0ad5..517e36d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -452,6 +452,24 @@ struct private_thread_info
/* This is set to the data address of the access causing the target
to stop for a watchpoint. */
CORE_ADDR watch_data_address;
+
+ /* Fields used by the vCont action coalescing implemented in
+ remote_resume / remote_commit_resume. remote_resume stores each
+ thread's last resume request in these fields, so that a later
+ remote_commit_resume knows which is the proper action for this
+ thread to include in the vCont packet. */
+
+ /* True if the last target_resume call for this thread was a step
+ request, false if a continue request. */
+ int last_resume_step;
+
+ /* The signal specified in the last target_resume call for this
+ thread. */
+ enum gdb_signal last_resume_sig;
+
+ /* Whether this thread was already vCont-resumed on the remote
+ side. */
+ int vcont_resumed;
};
static void
@@ -1805,6 +1823,9 @@ remote_add_inferior (int fake_pid_p, int pid, int attached,
return inf;
}
+static struct private_thread_info *
+ get_private_info_thread (struct thread_info *info);
+
/* Add thread PTID to GDB's thread list. Tag it as executing/running
according to RUNNING. */
@@ -1812,6 +1833,7 @@ static void
remote_add_thread (ptid_t ptid, int running, int executing)
{
struct remote_state *rs = get_remote_state ();
+ struct thread_info *thread;
/* GDB historically didn't pull threads in the initial connection
setup. If the remote target doesn't even have a concept of
@@ -1820,10 +1842,11 @@ remote_add_thread (ptid_t ptid, int running, int executing)
might be confusing to the user. Be silent then, preserving the
age old behavior. */
if (rs->starting_up)
- add_thread_silent (ptid);
+ thread = add_thread_silent (ptid);
else
- add_thread (ptid);
+ thread = add_thread (ptid);
+ get_private_info_thread (thread)->vcont_resumed = executing;
set_executing (ptid, executing);
set_running (ptid, running);
}
@@ -1918,25 +1941,40 @@ remote_notice_new_inferior (ptid_t currthread, int executing)
}
}
-/* Return the private thread data, creating it if necessary. */
+/* Return THREAD's private thread data, creating it if necessary. */
static struct private_thread_info *
-demand_private_info (ptid_t ptid)
+get_private_info_thread (struct thread_info *thread)
{
- struct thread_info *info = find_thread_ptid (ptid);
+ gdb_assert (thread != NULL);
- gdb_assert (info);
-
- if (!info->priv)
+ if (thread->priv == NULL)
{
- info->priv = XNEW (struct private_thread_info);
- info->private_dtor = free_private_thread_info;
- info->priv->core = -1;
- info->priv->extra = NULL;
- info->priv->name = NULL;
+ struct private_thread_info *priv = XNEW (struct private_thread_info);
+
+ thread->private_dtor = free_private_thread_info;
+ thread->priv = priv;
+
+ priv->core = -1;
+ priv->extra = NULL;
+ priv->name = NULL;
+ priv->name = NULL;
+ priv->last_resume_step = 0;
+ priv->last_resume_sig = GDB_SIGNAL_0;
+ priv->vcont_resumed = 0;
}
- return info->priv;
+ return thread->priv;
+}
+
+/* Return PTID's private thread data, creating it if necessary. */
+
+static struct private_thread_info *
+get_private_info_ptid (ptid_t ptid)
+{
+ struct thread_info *info = find_thread_ptid (ptid);
+
+ return get_private_info_thread (info);
}
/* Call this function as a result of
@@ -3280,7 +3318,7 @@ remote_update_thread_list (struct target_ops *ops)
remote_notice_new_inferior (item->ptid, executing);
- info = demand_private_info (item->ptid);
+ info = get_private_info_ptid (item->ptid);
info->core = item->core;
info->extra = item->extra;
item->extra = NULL;
@@ -3913,6 +3951,7 @@ process_initial_stop_replies (int from_tty)
set_executing (event_ptid, 0);
set_running (event_ptid, 0);
+ thread->priv->vcont_resumed = 0;
}
/* "Notice" the new inferiors before anything related to
@@ -5701,6 +5740,26 @@ remote_resume (struct target_ops *ops,
{
struct remote_state *rs = get_remote_state ();
+ /* When connected in non-stop mode, the core resumes threads
+ individually. Resuming remote threads directly in target_resume
+ would thus result in sending one packet per thread. Instead, to
+ minimize roundtrip latency, here we just store the resume
+ request; the actual remote resumption will be done in
+ target_commit_resume / remote_commit_resume, where we'll be able
+ to do vCont action coalescing. */
+ if (target_is_non_stop_p () && execution_direction != EXEC_REVERSE)
+ {
+ struct private_thread_info *remote_thr;
+
+ if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid))
+ remote_thr = get_private_info_ptid (inferior_ptid);
+ else
+ remote_thr = get_private_info_ptid (ptid);
+ remote_thr->last_resume_step = step;
+ remote_thr->last_resume_sig = siggnal;
+ return;
+ }
+
/* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
(explained in remote-notif.c:handle_notification) so
remote_notif_process is not called. We need find a place where
@@ -5736,6 +5795,283 @@ remote_resume (struct target_ops *ops,
if (!target_is_non_stop_p ())
rs->waiting_for_stop_reply = 1;
}
+
+static void check_pending_events_prevent_wildcard_vcont
+ (int *may_global_wildcard_vcont);
+static int is_pending_fork_parent_thread (struct thread_info *thread);
+
+/* Private per-inferior info for target remote processes. */
+
+struct private_inferior
+{
+ /* Whether we can send a wildcard vCont for this process. */
+ int may_wildcard_vcont;
+};
+
+/* Structure used to track the construction of a vCont packet in the
+ outgoing packet buffer. This is used to send multiple vCont
+ packets if we have more actions than would fit a single packet. */
+
+struct vcont_builder
+{
+ /* Pointer to the first action. P points here if no action has been
+ appended yet. */
+ char *first_action;
+
+ /* Where the next action will be appended. */
+ char *p;
+
+ /* The end of the buffer. Must never write past this. */
+ char *endp;
+};
+
+/* Prepare the outgoing buffer for a new vCont packet. */
+
+static void
+vcont_builder_restart (struct vcont_builder *builder)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ builder->p = rs->buf;
+ builder->endp = rs->buf + get_remote_packet_size ();
+ builder->p += xsnprintf (builder->p, builder->endp - builder->p, "vCont");
+ builder->first_action = builder->p;
+}
+
+/* If the vCont packet being built has any action, send it to the
+ remote end. */
+
+static void
+vcont_builder_flush (struct vcont_builder *builder)
+{
+ struct remote_state *rs;
+
+ if (builder->p == builder->first_action)
+ return;
+
+ rs = get_remote_state ();
+ putpkt (rs->buf);
+ getpkt (&rs->buf, &rs->buf_size, 0);
+ if (strcmp (rs->buf, "OK") != 0)
+ error (_("Unexpected vCont reply in non-stop mode: %s"), rs->buf);
+}
+
+/* The largest action is range-stepping, with its two addresses. This
+ is more than sufficient. If a new, bigger action is created, it'll
+ quickly trigger a failed assertion in append_resumption (and we'll
+ just bump this). */
+#define MAX_ACTION_SIZE 200
+
+/* Append a new vCont action in the outgoing packet being built. If
+ the action doesn't fit the packet along with previous actions, push
+ what we've got so far to the remote end and start over a new vCont
+ packet (with the new action). */
+
+static void
+vcont_builder_push_action (struct vcont_builder *builder,
+ ptid_t ptid, int step, enum gdb_signal siggnal)
+{
+ char buf[MAX_ACTION_SIZE + 1];
+ char *endp;
+ size_t rsize;
+
+ endp = append_resumption (buf, buf + sizeof (buf),
+ ptid, step, siggnal);
+
+ /* Check whether this new action would fit in the vCont packet along
+ with previous actions. If not, send what we've got so far and
+ start a new vCont packet. */
+ rsize = endp - buf;
+ if (rsize > builder->endp - builder->p)
+ {
+ vcont_builder_flush (builder);
+ vcont_builder_restart (builder);
+
+ /* Should now fit. */
+ gdb_assert (rsize <= builder->endp - builder->p);
+ }
+
+ memcpy (builder->p, buf, rsize);
+ builder->p += rsize;
+ *builder->p = '\0';
+}
+
+/* to_commit_resume implementation. */
+
+static void
+remote_commit_resume (struct target_ops *ops)
+{
+ struct remote_state *rs = get_remote_state ();
+ struct inferior *inf;
+ struct thread_info *tp;
+ int any_process_wildcard;
+ int may_global_wildcard_vcont;
+ struct vcont_builder vcont_builder;
+
+ /* If connected in all-stop mode, we'd send the remote resume
+ request directly from remote_resume. Likewise if
+ reverse-debugging, as there are no defined vCont actions for
+ reverse execution. */
+ if (!target_is_non_stop_p () || execution_direction == EXEC_REVERSE)
+ return;
+
+ /* Try to send wildcard actions ("vCont;c" or "vCont;c:pPID.-1")
+ instead of resuming all threads of each process individually.
+ However, if any thread of a process must remain halted, we can't
+ send wildcard resumes and must send one action per thread.
+
+ Care must be taken to not resume threads/processes the server
+ side already told us are stopped, but the core doesn't know about
+ yet, because the events are still in the vStopped notification
+ queue. For example:
+
+ #1 => vCont s:p1.1;c
+ #2 <= OK
+ #3 <= %Stopped T05 p1.1
+ #4 => vStopped
+ #5 <= T05 p1.2
+ #6 => vStopped
+ #7 <= OK
+ #8 (infrun handles the stop for p1.1 and continues stepping)
+ #9 => vCont s:p1.1;c
+
+ The last vCont above would resume thread p1.2 by mistake, because
+ the server has no idea that the event for p1.2 had not been
+ handled yet.
+
+ The server side must similarly 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 p1.2
+ #4 => vCont s:p1.1;c
+ #5 <= OK
+
+ Above, the server must not resume thread p1.2. GDB can't know
+ that p1.2 stopped until it acks the %Stopped notification, and
+ since from GDB's perspective all threads should be running, it
+ sends a "c" action.
+
+ 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. */
+
+ /* Start by assuming a global wildcard (vCont;c) is possible. */
+ may_global_wildcard_vcont = 1;
+
+ /* And assume every process is individually wildcard-able too. */
+ ALL_NON_EXITED_INFERIORS (inf)
+ {
+ if (inf->priv == NULL)
+ inf->priv = XNEW (struct private_inferior);
+ inf->priv->may_wildcard_vcont = 1;
+ }
+
+ /* Check for any pending events (not reported or processed yet) and
+ disable process and global wildcard resumes appropriately. */
+ check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont);
+
+ ALL_NON_EXITED_THREADS (tp)
+ {
+ /* If a thread of a process is not meant to be resumed, then we
+ can't wildcard that process. */
+ if (!tp->executing)
+ {
+ tp->inf->priv->may_wildcard_vcont = 0;
+
+ /* And if we can't wildcard a process, we can't wildcard
+ everything either. */
+ may_global_wildcard_vcont = 0;
+ continue;
+ }
+
+ /* If a thread is the parent of an unfollowed fork, then we
+ can't do a global wildcard, as that would resume the fork
+ child. */
+ if (is_pending_fork_parent_thread (tp))
+ may_global_wildcard_vcont = 0;
+ }
+
+ /* Now let's build the vCont packet(s). Actions must be appended
+ from narrower to wider scopes (thread -> process -> global). If
+ we end up with too many actions for a single packet vcont_builder
+ flushes the current vCont packet to the remote side and starts a
+ new one. */
+ vcont_builder_restart (&vcont_builder);
+
+ /* Threads first. */
+ ALL_NON_EXITED_THREADS (tp)
+ {
+ struct private_thread_info *remote_thr = tp->priv;
+
+ if (!tp->executing || remote_thr->vcont_resumed)
+ continue;
+
+ gdb_assert (!thread_is_in_step_over_chain (tp));
+
+ if (!remote_thr->last_resume_step
+ && remote_thr->last_resume_sig == GDB_SIGNAL_0
+ && tp->inf->priv->may_wildcard_vcont)
+ {
+ /* We'll send a wildcard resume instead. */
+ remote_thr->vcont_resumed = 1;
+ continue;
+ }
+
+ vcont_builder_push_action (&vcont_builder, tp->ptid,
+ remote_thr->last_resume_step,
+ remote_thr->last_resume_sig);
+ remote_thr->vcont_resumed = 1;
+ }
+
+ /* Now check whether we can send any process-wide wildcard. This is
+ to avoid sending a global wildcard in the case nothing is
+ supposed to be resumed. */
+ any_process_wildcard = 0;
+
+ ALL_NON_EXITED_INFERIORS (inf)
+ {
+ if (inf->priv->may_wildcard_vcont)
+ {
+ any_process_wildcard = 1;
+ break;
+ }
+ }
+
+ if (any_process_wildcard)
+ {
+ /* If all processes are wildcard-able, then send a single "c"
+ action, otherwise, send an "all (-1) threads of process"
+ continue action for each running process, if any. */
+ if (may_global_wildcard_vcont)
+ {
+ vcont_builder_push_action (&vcont_builder, minus_one_ptid,
+ 0, GDB_SIGNAL_0);
+ }
+ else
+ {
+ ALL_NON_EXITED_INFERIORS (inf)
+ {
+ if (inf->priv->may_wildcard_vcont)
+ {
+ vcont_builder_push_action (&vcont_builder,
+ pid_to_ptid (inf->pid),
+ 0, GDB_SIGNAL_0);
+ }
+ }
+ }
+ }
+
+ vcont_builder_flush (&vcont_builder);
+}
+
\f
/* Non-stop version of target_stop. Uses `vCont;t' to stop a remote
@@ -6102,7 +6438,7 @@ struct queue_iter_param
struct stop_reply *output;
};
-/* Determine if THREAD is a pending fork parent thread. ARG contains
+/* Determine if THREAD_PTID is a pending fork parent thread. ARG contains
the pid of the process that owns the threads we want to check, or
-1 if we want to check all threads. */
@@ -6120,6 +6456,29 @@ is_pending_fork_parent (struct target_waitstatus *ws, int event_pid,
return 0;
}
+/* Return the thread's pending status used to determine whether the
+ thread is a fork parent stopped at a fork event. */
+
+static struct target_waitstatus *
+thread_pending_fork_status (struct thread_info *thread)
+{
+ if (thread->suspend.waitstatus_pending_p)
+ return &thread->suspend.waitstatus;
+ else
+ return &thread->pending_follow;
+}
+
+/* Determine if THREAD is a pending fork parent thread. */
+
+static int
+is_pending_fork_parent_thread (struct thread_info *thread)
+{
+ struct target_waitstatus *ws = thread_pending_fork_status (thread);
+ int pid = -1;
+
+ return is_pending_fork_parent (ws, pid, thread->ptid);
+}
+
/* Check whether EVENT is a fork event, and if it is, remove the
fork child from the context list passed in DATA. */
@@ -6159,12 +6518,7 @@ remove_new_fork_children (struct threads_listing_context *context)
fork child threads from the CONTEXT list. */
ALL_NON_EXITED_THREADS (thread)
{
- struct target_waitstatus *ws;
-
- if (thread->suspend.waitstatus_pending_p)
- ws = &thread->suspend.waitstatus;
- else
- ws = &thread->pending_follow;
+ struct target_waitstatus *ws = thread_pending_fork_status (thread);
if (is_pending_fork_parent (ws, pid, thread->ptid))
{
@@ -6182,6 +6536,56 @@ remove_new_fork_children (struct threads_listing_context *context)
remove_child_of_pending_fork, ¶m);
}
+/* Check whether EVENT would prevent a global or process wildcard
+ vCont action. */
+
+static int
+check_pending_event_prevents_wildcard_vcont_callback
+ (QUEUE (stop_reply_p) *q,
+ QUEUE_ITER (stop_reply_p) *iter,
+ stop_reply_p event,
+ void *data)
+{
+ struct inferior *inf;
+ int *may_global_wildcard_vcont = (int *) data;
+
+ if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED
+ || event->ws.kind == TARGET_WAITKIND_NO_HISTORY)
+ return 1;
+
+ if (event->ws.kind == TARGET_WAITKIND_FORKED
+ || event->ws.kind == TARGET_WAITKIND_VFORKED)
+ *may_global_wildcard_vcont = 0;
+
+ inf = find_inferior_ptid (event->ptid);
+
+ /* This may be the first time we heard about this process.
+ Regardless, we must not do a global wildcard resume, otherwise
+ we'd resume this process too. */
+ *may_global_wildcard_vcont = 0;
+ if (inf != NULL)
+ inf->priv->may_wildcard_vcont = 0;
+
+ return 1;
+}
+
+/* Check whether any event pending in the vStopped queue would prevent
+ a global or process wildcard vCont action. Clear
+ *may_global_wildcard if we can't do a global wildcard (vCont;c),
+ and clear the event inferior's may_wildcard_vcont flag if we can't
+ do a process-wide wildcard resume (vCont;c:pPID.-1). */
+
+static void
+check_pending_events_prevent_wildcard_vcont (int *may_global_wildcard)
+{
+ struct notif_client *notif = ¬if_client_stop;
+
+ remote_notif_get_pending_events (notif);
+ QUEUE_iterate (stop_reply_p, stop_reply_queue,
+ check_pending_event_prevents_wildcard_vcont_callback,
+ may_global_wildcard);
+}
+
/* Remove stop replies in the queue if its pid is equal to the given
inferior's pid. */
@@ -6812,10 +7216,11 @@ process_stop_reply (struct stop_reply *stop_reply,
}
remote_notice_new_inferior (ptid, 0);
- remote_thr = demand_private_info (ptid);
+ remote_thr = get_private_info_ptid (ptid);
remote_thr->core = stop_reply->core;
remote_thr->stop_reason = stop_reply->stop_reason;
remote_thr->watch_data_address = stop_reply->watch_data_address;
+ remote_thr->vcont_resumed = 0;
}
stop_reply_xfree (stop_reply);
@@ -13114,6 +13519,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_detach = remote_detach;
remote_ops.to_disconnect = remote_disconnect;
remote_ops.to_resume = remote_resume;
+ remote_ops.to_commit_resume = remote_commit_resume;
remote_ops.to_wait = remote_wait;
remote_ops.to_fetch_registers = remote_fetch_registers;
remote_ops.to_store_registers = remote_store_registers;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 57e7939..73e45dd 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -109,6 +109,28 @@ debug_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal ar
fputs_unfiltered (")\n", gdb_stdlog);
}
+static void
+delegate_commit_resume (struct target_ops *self)
+{
+ self = self->beneath;
+ self->to_commit_resume (self);
+}
+
+static void
+tdefault_commit_resume (struct target_ops *self)
+{
+}
+
+static void
+debug_commit_resume (struct target_ops *self)
+{
+ fprintf_unfiltered (gdb_stdlog, "-> %s->to_commit_resume (...)\n", debug_target.to_shortname);
+ debug_target.to_commit_resume (&debug_target);
+ fprintf_unfiltered (gdb_stdlog, "<- %s->to_commit_resume (", debug_target.to_shortname);
+ target_debug_print_struct_target_ops_p (&debug_target);
+ fputs_unfiltered (")\n", gdb_stdlog);
+}
+
static ptid_t
delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3)
{
@@ -4108,6 +4130,8 @@ install_delegators (struct target_ops *ops)
ops->to_disconnect = delegate_disconnect;
if (ops->to_resume == NULL)
ops->to_resume = delegate_resume;
+ if (ops->to_commit_resume == NULL)
+ ops->to_commit_resume = delegate_commit_resume;
if (ops->to_wait == NULL)
ops->to_wait = delegate_wait;
if (ops->to_fetch_registers == NULL)
@@ -4413,6 +4437,7 @@ install_dummy_methods (struct target_ops *ops)
ops->to_detach = tdefault_detach;
ops->to_disconnect = tdefault_disconnect;
ops->to_resume = tdefault_resume;
+ ops->to_commit_resume = tdefault_commit_resume;
ops->to_wait = default_target_wait;
ops->to_fetch_registers = tdefault_fetch_registers;
ops->to_store_registers = tdefault_store_registers;
@@ -4570,6 +4595,7 @@ init_debug_target (struct target_ops *ops)
ops->to_detach = debug_detach;
ops->to_disconnect = debug_disconnect;
ops->to_resume = debug_resume;
+ ops->to_commit_resume = debug_commit_resume;
ops->to_wait = debug_wait;
ops->to_fetch_registers = debug_fetch_registers;
ops->to_store_registers = debug_store_registers;
diff --git a/gdb/target.c b/gdb/target.c
index cb89e75..246d292 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2329,6 +2329,34 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
clear_inline_frame_state (ptid);
}
+/* If true, target_commit_resume is a nop. */
+static int defer_target_commit_resume;
+
+/* See target.h. */
+
+void
+target_commit_resume (void)
+{
+ struct target_ops *t;
+
+ if (defer_target_commit_resume)
+ return;
+
+ current_target.to_commit_resume (¤t_target);
+}
+
+/* See target.h. */
+
+struct cleanup *
+make_cleanup_defer_target_commit_resume (void)
+{
+ struct cleanup *old_chain;
+
+ old_chain = make_cleanup_restore_integer (&defer_target_commit_resume);
+ defer_target_commit_resume = 1;
+ return old_chain;
+}
+
void
target_pass_signals (int numsigs, unsigned char *pass_signals)
{
diff --git a/gdb/target.h b/gdb/target.h
index 176f332..a54b3db 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -461,6 +461,8 @@ struct target_ops
int TARGET_DEBUG_PRINTER (target_debug_print_step),
enum gdb_signal)
TARGET_DEFAULT_NORETURN (noprocess ());
+ void (*to_commit_resume) (struct target_ops *)
+ TARGET_DEFAULT_IGNORE ();
ptid_t (*to_wait) (struct target_ops *,
ptid_t, struct target_waitstatus *,
int TARGET_DEBUG_PRINTER (target_debug_print_options))
@@ -1328,19 +1330,42 @@ extern void target_detach (const char *, int);
extern void target_disconnect (const char *, int);
-/* Resume execution of the target process PTID (or a group of
- threads). 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 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.
+
+ 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);
+/* Commit a series of resumption requests previously prepared with
+ target_resume calls.
+
+ GDB always calls target_commit_resume after calling target_resume
+ one or more times. A target may thus use this method in
+ coordination with the target_resume method to batch target-side
+ resumption requests. In that case, the target doesn't actually
+ resume in its target_resume implementation. Instead, it prepares
+ the resumption in target_resume, and defers the actual resumption
+ to target_commit_resume. E.g., the remote target uses this to
+ coalesce multiple resumption requests in a single vCont packet. */
+extern void target_commit_resume ();
+
+/* Setup to defer target_commit_resume calls, and return a cleanup
+ that reactivates target_commit_resume, if it was previously
+ active. */
+struct cleanup *make_cleanup_defer_target_commit_resume ();
+
/* For target_read_memory see target/target.h. */
/* The default target_ops::to_wait implementation. */
--
2.5.5
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were
2016-02-17 12:32 ` Pedro Alves
@ 2016-10-26 15:39 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2016-10-26 15:39 UTC (permalink / raw)
To: Luis Machado, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-10-26 15:39 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves
2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves
2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves
2016-02-17 15:56 ` Eli Zaretskii
2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves
2016-02-17 11:45 ` Luis Machado
2016-02-17 12:32 ` Pedro Alves
2016-02-17 13:44 ` Luis Machado
2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves
2016-02-17 13:42 ` Luis Machado
2016-10-26 15:35 ` Pedro Alves
2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves
2016-02-17 11:46 ` Luis Machado
2016-02-17 12:32 ` Pedro Alves
2016-10-26 15:39 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).