[PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions

[PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions

[Pedro Alves]

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

[PATCH 1/5] gdb: Clean up remote.c:remote_resume

[Pedro Alves]

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, &notif_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

[PATCH 2/5] gdb: Free inferior->priv when inferior exits

[Pedro Alves]

(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

[PATCH 3/5] gdb/doc: Clarify vCont packet description

[Pedro Alves]

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

[PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions

[Pedro Alves]

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, &param);
 }
 
+/* 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 = &notif_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 (&current_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

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

[Pedro Alves]

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

Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume

[Luis Machado]

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, &notif_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
>

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

[Luis Machado]

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.

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

[Pedro Alves]

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

Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume

[Pedro Alves]

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

Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions

[Luis Machado]

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, &param);
>   }
>
> +/* 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."?

Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume

[Luis Machado]

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.

Re: [PATCH 3/5] gdb/doc: Clarify vCont packet description

[Eli Zaretskii]

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

Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions

[Pedro Alves]

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, &param);
 }
 
+/* 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 = &notif_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 (&current_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

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

[Pedro Alves]

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