[PATCH v3 4/5] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

From: Simon Marchi <simon.marchi@efficios.com>

The rationale for this patch comes from the ROCm port [1], the goal
being to reduce the number of back and forths between GDB and the target
when doing successive operations.  I'll start with explaining the
rationale and then go over the implementation.  In the ROCm / GPU world,
the term "wave" is somewhat equivalent to a "thread" in GDB.  So if you
read if from a GPU stand point, just s/thread/wave/.

ROCdbgapi, the library used by GDB [2] to communicate with the GPU
target, gives the illusion that it's possible for the debugger to
control (start and stop) individual threads.  But in reality, this is
not how it works.  Under the hood, all threads of a queue are controlled
as a group.  To stop one thread in a group of running ones, the state of
all threads is retrieved from the GPU, all threads are destroyed, and all
threads but the one we want to stop are re-created from the saved state.
The net result, from the point of view of GDB, is that the library
stopped one thread.  The same thing goes if we want to resume one thread
while others are running: the state of all running threads is retrieved
from the GPU, they are all destroyed, and they are all re-created,
including the thread we want to resume.

This leads to some inefficiencies when combined with how GDB works, here
are two examples:

 - Stopping all threads: because the target operates in non-stop mode,
   when the user interface mode is all-stop, GDB must stop all threads
   individually when presenting a stop.  Let's suppose we have 1000
   threads and the user does ^C.  GDB asks the target to stop one
   thread.  Behind the scenes, the library retrieves 1000 thread states
   and restores the 999 others still running ones.  GDB asks the target
   to stop another one.  The target retrieves 999 thread states and
   restores the 998 remaining ones.  That means that to stop 1000
   threads, we did 1000 back and forths with the GPU.  It would have
   been much better to just retrieve the states once and stop there.

 - Resuming with pending events: suppose the 1000 threads hit a
   breakpoint at the same time.  The breakpoint is conditional and
   evaluates to true for the first thread, to false for all others.  GDB
   pulls one event (for the first thread) from the target, decides that
   it should present a stop, so stops all threads using
   stop_all_threads.  All these other threads have a breakpoint event to
   report, which is saved in `thread_info::suspend::waitstatus` for
   later.  When the user does "continue", GDB resumes that one thread
   that did hit the breakpoint.  It then processes the pending events
   one by one as if they just arrived.  It picks one, evaluates the
   condition to false, and resumes the thread.  It picks another one,
   evaluates the condition to false, and resumes the thread.  And so on.
   In between each resumption, there is a full state retrieval and
   re-creation.  It would be much nicer if we could wait a little bit
   before sending those threads on the GPU, until it processed all those
   pending events.

To address this kind of performance issue, ROCdbgapi has a concept
called "forward progress required", which is a boolean state that allows
its user (i.e. GDB) to say "I'm doing a bunch of operations, you can
hold off putting the threads on the GPU until I'm done" (the "forward
progress not required" state).  Turning forward progress back on
indicates to the library that all threads that are supposed to be
running should now be really running on the GPU.

It turns out that GDB has a similar concept, though not as general,
commit_resume.  On difference is that commit_resume is not stateful: the
target can't look up "does the core need me to schedule resumed threads
for execution right now".  It is also specifically linked to the resume
method, it is not used in other contexts.  The target accumulates
resumption requests through target_ops::resume calls, and then commits
those resumptions when target_ops::commit_resume is called.  The target
has no way to check if it's ok to leave resumed threads stopped in other
target methods.

To bridge the gap, this patch generalizes the commit_resume concept in
GDB to match the forward progress concept of ROCdbgapi.  The current
name (commit_resume) can be interpreted as "commit the previous resume
calls".  I renamed the concept to "commit_resumed", as in "commit the
threads that are resumed".

In the new version, we have two things in process_stratum_target:

 - the commit_resumed_state field: indicates whether GDB requires this
   target to have resumed threads committed to the execution
   target/device.  If false, the target is allowed to leave resumed
   threads un-committed at the end of whatever method it is executing.

 - the commit_resumed method: called when commit_resumed_state
   transitions from false to true.  While commit_resumed_state was
   false, the target may have left some resumed threads un-committed.
   This method being called tells it that it should commit them back to
   the execution device.

Let's take the "Stopping all threads" scenario from above and see how it
would work with the ROCm target with this change.  Before stopping all
threads, GDB would set the target's commit_resumed_state field to false.
It would then ask the target to stop the first thread.  The target would
retrieve all threads' state from the GPU and mark that one as stopped.
Since commit_resumed_state is false, it leaves all the other threads
(still resumed) stopped.  GDB would then proceed to call target_stop for
all the other threads.  Since resumed threads are not committed, this
doesn't do any back and forth with the GPU.

To simplify the implementation of targets, I made it so that when
calling certain target methods, the contract between the core and the
targets guarantees that commit_resumed_state is false.  This way, the
target doesn't need two paths, one commit_resumed_state == true and one
for commit_resumed_state == false.  It can just assert that
commit_resumed_state is false and work with that assumption.  This also
helps catch places where we forgot to disable commit_resumed_state
before calling the method, which represents a probable optimization
opportunity.

To have some confidence that this contract between the core and the
targets is respected, I added assertions in the linux-nat target
methods, even though the linux-nat target doesn't actually use that
feature.  Since linux-nat is tested much more than other targets, this
will help catch these issues quicker.

To ensure that commit_resumed_state is always turned back on (only if
necessary, see below) and the commit_resumed method is called when doing
so, I introduced the scoped_disabled_commit_resumed RAII object, which
replaces make_scoped_defer_process_target_commit_resume.  On
construction, it clears the commit_resumed_state flag of all process
targets.  On destruction, it turns it back on (if necessary) and calls
the commit_resumed method.  The nested case is handled by having a
"nesting" counter: only when the counter goes back to 0 is
commit_resumed_state turned back on.

On destruction, commit-resumed is not re-enabled for a given target if:

 1. this target has no threads resumed, or
 2. this target at least one thread with a pending status known to the
    core (saved in thread_info::suspend::waitstatus).

The first point is not technically necessary, because a proper
commit_resumed implementation would be a no-op if the target has no
resumed threads.  But since we have a flag do to a quick check, I think
it doesn't hurt.

The second point is more important: together with the
scoped_disable_commit_resumed instance added in fetch_inferior_event, it
makes it so the "Resuming with pending events" described above is
handled efficiently.  Here's what happens in that case:

 1. The user types "continue".
 2. Upon destruction, the scoped_disable_commit_resumed in the `proceed`
    function does not enable commit-resumed, as it sees other threads
    have pending statuses.
 3. fetch_inferior_event is called to handle another event, one thread
    is resumed.  Because there are still more threads with pending
    statuses, the destructor of scoped_disable_commit_resumed in
    fetch_inferior_event still doesn't enable commit-resumed.
 4. Rinse and repeat step 3, until the last pending status is handled by
    fetch_inferior_event.  In that case, scoped_disable_commit_resumed's
    destructor sees there are no more threads with pending statues, so
    it asks the target to commit resumed threads.

This allows us to avoid all unnecessary back and forths, there is a
single commit_resumed call.

This change required remote_target::remote_stop_ns to learn how to
handle stopping threads that were resumed but pending vCont.  The
simplest example where that happens is when using the remote target in
all-stop, but with "maint set target-non-stop on", to force it to
operate in non-stop mode under the hood.  If two threads hit a
breakpoint at the same time, GDB will receive two stop replies.  It will
present the stop for one thread and save the other one in
thread_info::suspend::waitstatus.

Before this patch, when doing "continue", GDB first resumes the thread
without a pending status:

    Sending packet: $vCont;c:p172651.172676#f3

It then consumes the pending status in the next fetch_inferior_event
call:

    [infrun] do_target_wait_1: Using pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP for Thread 1517137.1517137.
    [infrun] target_wait (-1.0.0, status) =
    [infrun]   1517137.1517137.0 [Thread 1517137.1517137],
    [infrun]   status->kind = stopped, signal = GDB_SIGNAL_TRAP

It then realizes it needs to stop all threads to present the stop, so
stops the thread it just resumed:

    [infrun] stop_all_threads:   Thread 1517137.1517137 not executing
    [infrun] stop_all_threads:   Thread 1517137.1517174 executing, need stop
    remote_stop called
    Sending packet: $vCont;t:p172651.172676#04

This is an unnecessary resume/stop.  With this patch, we don't commit
resumed threads after proceeding, because of the pending status:

    [infrun] maybe_commit_resumed_all_process_targets: not requesting commit-resumed for target extended-remote, a thread has a pending waitstatus

When GDB handles the pending status and stop_all_threads runs, we stop a
resumed but pending vCont thread:

    remote_stop_ns: Enqueueing phony stop reply for thread pending vCont-resume (1520940, 1520976, 0)

That thread was never actually resumed on the remote stub / gdbserver.
This is why remote_stop_ns needed to learn this new trick of enqueueing
phony stop replies.

Note that this patch only considers pending statuses known to the core
of GDB, that is the events that were pulled out of the target and stored
in `thread_info::suspend::waitstatus`.  In some cases, we could also
avoid unnecessary back and forth when the target has events that it has
not yet reported the core.  I plan to implement this as a subsequent
patch, once this series has settled.

gdb/ChangeLog:

	* infrun.h (struct scoped_disable_commit_resumed): New.
	* infrun.c (do_target_resume): Remove
	maybe_commit_resume_process_target call.
	(maybe_commit_resume_all_process_targets): Rename to...
	(maybe_commit_resumed_all_process_targets): ... this.  Skip
	targets that have no executing threads or resumed threads with
	a pending status.
	(scoped_disable_commit_resumed_depth): New.
	(scoped_disable_commit_resumed::scoped_disable_commit_resumed):
	New.
	(scoped_disable_commit_resumed::~scoped_disable_commit_resumed):
	New.
	(proceed): Use scoped_disable_commit_resumed.
	(fetch_inferior_event): Use scoped_disable_commit_resumed.
	* process-stratum-target.h (class process_stratum_target):
	<commit_resume>: Rename to...
	<commit_resumed>: ... this.
	<commit_resumed_state>: New.
	(all_process_targets): New.
	(maybe_commit_resume_process_target): Remove.
	(make_scoped_defer_process_target_commit_resume): Remove.
	* process-stratum-target.c (all_process_targets): New.
	(defer_process_target_commit_resume): Remove.
	(maybe_commit_resume_process_target): Remove.
	(make_scoped_defer_process_target_commit_resume): Remove.
	* linux-nat.c (linux_nat_target::resume): Add gdb_assert.
	(linux_nat_target::wait): Add gdb_assert.
	(linux_nat_target::stop): Add gdb_assert.
	* infcmd.c (run_command_1): Use scoped_disable_commit_resumed.
	(attach_command): Use scoped_disable_commit_resumed.
	(detach_command): Use scoped_disable_commit_resumed.
	(interrupt_target_1): Use scoped_disable_commit_resumed.
	* mi/mi-main.c (exec_continue): Use
	scoped_disable_commit_resumed.
	* record-full.c (record_full_wait_1): Change
	commit_resumed_state around calling commit_resumed.
	* remote.c (class remote_target) <commit_resume>: Rename to...
	<commit_resumed>: ... this.
	(remote_target::resume): Add gdb_assert.
	(remote_target::commit_resume): Rename to...
	(remote_target::commit_resumed): ... this.  Check if there is
	any thread pending vCont resume.
	(struct stop_reply): Move up.
	(remote_target::remote_stop_ns): Generate stop replies for
	resumed but pending vCont threads.
	(remote_target::wait_ns): Add gdb_assert.

[1] https://github.com/ROCm-Developer-Tools/ROCgdb/
[2] https://github.com/ROCm-Developer-Tools/ROCdbgapi

Change-Id: I836135531a29214b21695736deb0a81acf8cf566
---
 gdb/infcmd.c                 |   8 +++
 gdb/infrun.c                 | 116 +++++++++++++++++++++++++++++++----
 gdb/infrun.h                 |  41 +++++++++++++
 gdb/linux-nat.c              |   5 ++
 gdb/mi/mi-main.c             |   2 +
 gdb/process-stratum-target.c |  37 +++++------
 gdb/process-stratum-target.h |  63 +++++++++++--------
 gdb/record-full.c            |   4 +-
 gdb/remote.c                 | 111 +++++++++++++++++++++++----------
 9 files changed, 292 insertions(+), 95 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 6f0ed952de67..b7595e42e265 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -488,6 +488,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
       uiout->flush ();
     }
 
+  scoped_disable_commit_resumed disable_commit_resumed ("running");
+
   /* We call get_inferior_args() because we might need to compute
      the value now.  */
   run_target->create_inferior (exec_file,
@@ -2591,6 +2593,8 @@ attach_command (const char *args, int from_tty)
   if (non_stop && !attach_target->supports_non_stop ())
     error (_("Cannot attach to this target in non-stop mode"));
 
+  scoped_disable_commit_resumed disable_commit_resumed ("attaching");
+
   attach_target->attach (args, from_tty);
   /* to_attach should push the target, so after this point we
      shouldn't refer to attach_target again.  */
@@ -2746,6 +2750,8 @@ detach_command (const char *args, int from_tty)
   if (inferior_ptid == null_ptid)
     error (_("The program is not being run."));
 
+  scoped_disable_commit_resumed disable_commit_resumed ("detaching");
+
   query_if_trace_running (from_tty);
 
   disconnect_tracing ();
@@ -2814,6 +2820,8 @@ stop_current_target_threads_ns (ptid_t ptid)
 void
 interrupt_target_1 (bool all_threads)
 {
+  scoped_disable_commit_resumed inhibit ("interrupting");
+
   if (non_stop)
     {
       if (all_threads)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1a27af51b7e9..92a1102cb595 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2172,8 +2172,6 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
 
   target_resume (resume_ptid, step, sig);
 
-  maybe_commit_resume_process_target (tp->inf->process_target ());
-
   if (target_can_async_p ())
     target_async (1);
 }
@@ -2760,17 +2758,109 @@ schedlock_applies (struct thread_info *tp)
 					    execution_direction)));
 }
 
-/* Calls maybe_commit_resume_process_target on all process targets.  */
+/* Maybe require all process stratum targets to commit their resumed threads.
+
+   A specific process stratum target is not required to do so if:
+
+   - it has no resumed threads
+   - it has a thread with a pending status  */
 
 static void
-maybe_commit_resume_all_process_targets ()
+maybe_commit_resumed_all_process_targets ()
 {
-  scoped_restore_current_thread restore_thread;
+  /* This is an optional to avoid unnecessary thread switches. */
+  gdb::optional<scoped_restore_current_thread> restore_thread;
 
   for (process_stratum_target *target : all_non_exited_process_targets ())
     {
+      gdb_assert (!target->commit_resumed_state);
+
+      if (!target->threads_executing)
+	{
+	  infrun_debug_printf ("not re-enabling forward progress for target "
+			       "%s, no executing threads",
+			       target->shortname ());
+	  continue;
+	}
+
+      /* If a thread from this target has some status to report, we better
+	 handle it before requiring the target to commit its resumed threads:
+	 handling the status might lead to resuming more threads.  */
+      bool has_thread_with_pending_status = false;
+      for (thread_info *thread : all_non_exited_threads (target))
+	if (thread->resumed && thread->suspend.waitstatus_pending_p)
+	  {
+	    has_thread_with_pending_status = true;
+	    break;
+	  }
+
+      if (has_thread_with_pending_status)
+	{
+	  infrun_debug_printf ("not requesting commit-resumed for target %s, a"
+			       "thread has a pending waitstatus",
+			       target->shortname ());
+	  continue;
+	}
+
+      if (!restore_thread.has_value ())
+	restore_thread.emplace ();
+
       switch_to_target_no_thread (target);
-      maybe_commit_resume_process_target (target);
+      infrun_debug_printf ("enabling commit-resumed for target %s",
+			   target->shortname());
+
+      target->commit_resumed_state = true;
+      target->commit_resumed ();
+    }
+}
+
+/* To track nesting of scoped_disable_commit_resumed objects.  */
+
+static int scoped_disable_commit_resumed_depth = 0;
+
+scoped_disable_commit_resumed::scoped_disable_commit_resumed
+  (const char *reason)
+  : m_reason (reason)
+{
+  infrun_debug_printf ("reason=%s", m_reason);
+
+  for (process_stratum_target *target : all_process_targets ())
+    {
+      if (scoped_disable_commit_resumed_depth == 0)
+	{
+	  /* This is the outermost instance.  */
+	  target->commit_resumed_state = false;
+	}
+      else
+	{
+	  /* This is not the outermost instance, we expect COMMIT_RESUMED_STATE
+	     to have been cleared by the outermost instance.  */
+	  gdb_assert (!target->commit_resumed_state);
+	}
+    }
+
+  ++scoped_disable_commit_resumed_depth;
+}
+
+scoped_disable_commit_resumed::~scoped_disable_commit_resumed ()
+{
+  infrun_debug_printf ("reason=%s", m_reason);
+
+  gdb_assert (scoped_disable_commit_resumed_depth > 0);
+
+  --scoped_disable_commit_resumed_depth;
+
+  if (scoped_disable_commit_resumed_depth == 0)
+    {
+      /* This is the outermost instance.  */
+      maybe_commit_resumed_all_process_targets ();
+    }
+  else
+    {
+      /* This is not the outermost instance, we expect COMMIT_RESUMED_STATE to
+	 still be false.  */
+      for (process_stratum_target *target : all_process_targets ())
+	gdb_assert (!target->commit_resumed_state);
     }
 }
 
@@ -2994,8 +3084,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
   cur_thr->prev_pc = regcache_read_pc_protected (regcache);
 
   {
-    scoped_restore save_defer_tc
-      = make_scoped_defer_process_target_commit_resume ();
+    scoped_disable_commit_resumed disable_commit_resumed ("proceeding");
 
     started = start_step_over ();
 
@@ -3065,8 +3154,6 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
       }
   }
 
-  maybe_commit_resume_all_process_targets ();
-
   finish_state.release ();
 
   /* If we've switched threads above, switch back to the previously
@@ -3819,8 +3906,15 @@ fetch_inferior_event ()
       = make_scoped_restore (&execution_direction,
 			     target_execution_direction ());
 
+    /* Allow process stratum targets to pause their resumed threads while we
+       handle the event.  */
+    scoped_disable_commit_resumed disable_commit_resumed ("handling event");
+
     if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
-      return;
+      {
+	infrun_debug_printf ("do_target_wait returned no event");
+	return;
+      }
 
     gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
 
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 7160b60f1368..5c32c0c97f6e 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -269,4 +269,45 @@ extern void all_uis_check_sync_execution_done (void);
    started or re-started).  */
 extern void all_uis_on_sync_execution_starting (void);
 
+/* RAII object to temporarily disable the requirement for process stratum
+   targets to commit their resumed threads.
+
+   On construction, set process_stratum_target::commit_resumed_state to false
+   for all process stratum targets.
+
+   On destruction, call maybe_commit_resumed_all_process_targets.
+
+   In addition, track creation of nested scoped_disable_commit_resumed objects,
+   for cases like this:
+
+     void
+     inner_func ()
+     {
+       scoped_disable_commit_resumed disable;
+       // do stuff
+     }
+
+     void
+     outer_func ()
+     {
+       scoped_disable_commit_resumed disable;
+
+       for (... each thread ...)
+	 inner_func ();
+     }
+
+   In this case, we don't want the `disable` in `inner_func` to require targets
+   to commit resumed threads in its destructor.  */
+
+struct scoped_disable_commit_resumed
+{
+  scoped_disable_commit_resumed (const char *reason);
+  ~scoped_disable_commit_resumed ();
+
+  DISABLE_COPY_AND_ASSIGN (scoped_disable_commit_resumed);
+
+private:
+  const char *m_reason;
+};
+
 #endif /* INFRUN_H */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index dc524cf10dc1..9adec81ba132 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1661,6 +1661,8 @@ linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 			   ? strsignal (gdb_signal_to_host (signo)) : "0"),
 			  target_pid_to_str (inferior_ptid).c_str ());
 
+  gdb_assert (!this->commit_resumed_state);
+
   /* A specific PTID means `step only this process id'.  */
   resume_many = (minus_one_ptid == ptid
 		 || ptid.is_pid ());
@@ -3406,6 +3408,8 @@ linux_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   linux_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
 			  target_options_to_string (target_options).c_str ());
 
+  gdb_assert (!this->commit_resumed_state);
+
   /* Flush the async file first.  */
   if (target_is_async_p ())
     async_file_flush ();
@@ -4166,6 +4170,7 @@ linux_nat_stop_lwp (struct lwp_info *lwp)
 void
 linux_nat_target::stop (ptid_t ptid)
 {
+  gdb_assert (!this->commit_resumed_state);
   iterate_over_lwps (ptid, linux_nat_stop_lwp);
 }
 
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 9a14d78e1e27..e5653ea3e3f5 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -266,6 +266,8 @@ exec_continue (char **argv, int argc)
 {
   prepare_execution_command (current_top_target (), mi_async_p ());
 
+  scoped_disable_commit_resumed disable_commit_resumed ("mi continue");
+
   if (non_stop)
     {
       /* In non-stop mode, 'resume' always resumes a single thread.
diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index 1436a550ac04..9877f0d81931 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -99,6 +99,20 @@ all_non_exited_process_targets ()
 
 /* See process-stratum-target.h.  */
 
+std::set<process_stratum_target *>
+all_process_targets ()
+{
+  /* Inferiors may share targets.  To eliminate duplicates, use a set.  */
+  std::set<process_stratum_target *> targets;
+  for (inferior *inf : all_inferiors ())
+    if (inf->process_target () != nullptr)
+      targets.insert (inf->process_target ());
+
+  return targets;
+}
+
+/* See process-stratum-target.h.  */
+
 void
 switch_to_target_no_thread (process_stratum_target *target)
 {
@@ -108,26 +122,3 @@ switch_to_target_no_thread (process_stratum_target *target)
       break;
     }
 }
-
-/* If true, `maybe_commit_resume_process_target` is a no-op.  */
-
-static bool defer_process_target_commit_resume;
-
-/* See target.h.  */
-
-void
-maybe_commit_resume_process_target (process_stratum_target *proc_target)
-{
-  if (defer_process_target_commit_resume)
-    return;
-
-  proc_target->commit_resume ();
-}
-
-/* See process-stratum-target.h.  */
-
-scoped_restore_tmpl<bool>
-make_scoped_defer_process_target_commit_resume ()
-{
-  return make_scoped_restore (&defer_process_target_commit_resume, true);
-}
diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h
index c8060c46be93..3cea911dee09 100644
--- a/gdb/process-stratum-target.h
+++ b/gdb/process-stratum-target.h
@@ -63,19 +63,10 @@ class process_stratum_target : public target_ops
   bool has_registers () override;
   bool has_execution (inferior *inf) override;
 
-  /* Commit a series of resumption requests previously prepared with
-     resume calls.
+  /* Ensure that all resumed threads are committed to the target.
 
-     GDB always calls `commit_resume` on the process stratum target after
-     calling `resume` on a target stack.  A process stratum target may thus use
-     this method in coordination with its `resume` method to batch resumption
-     requests.  In that case, the target doesn't actually resume in its
-     `resume` implementation.  Instead, it takes note of resumption intent in
-     `resume`, and defers the actual resumption `commit_resume`.
-
-     E.g., the remote target uses this to coalesce multiple resumption requests
-     in a single vCont packet.  */
-  virtual void commit_resume () {}
+     See the description of COMMIT_RESUMED_STATE for more details.  */
+  virtual void commit_resumed () {}
 
   /* True if any thread is, or may be executing.  We need to track
      this separately because until we fully sync the thread list, we
@@ -86,6 +77,35 @@ class process_stratum_target : public target_ops
 
   /* The connection number.  Visible in "info connections".  */
   int connection_number = 0;
+
+  /* Whether resumed threads must be committed to the target.
+
+     When true, resumed threads must be committed to the execution target.
+
+     When false, the process stratum target may leave resumed threads stopped
+     when it's convenient or efficient to do so.  When the core requires resumed
+     threads to be committed again, this is set back to true and calls the
+     `commit_resumed` method to allow the target to do so.
+
+     To simplify the implementation of process stratum targets, the following
+     methods are guaranteed to be called with COMMIT_RESUMED_STATE set to
+     false:
+
+       - resume
+       - stop
+       - wait
+
+     Knowing this, the process stratum target doesn't need to implement
+     different behaviors depending on the COMMIT_RESUMED_STATE, and can
+     simply assert that it is false.
+
+     Process stratum targets can take advantage of this to batch resumption
+     requests, for example.  In that case, the target doesn't actually resume in
+     its `resume` implementation.  Instead, it takes note of the resumption
+     intent in `resume` and defers the actual resumption to `commit_resumed`.
+     For example, the remote target uses this to coalesce multiple resumption
+     requests in a single vCont packet.  */
+  bool commit_resumed_state = false;
 };
 
 /* Downcast TARGET to process_stratum_target.  */
@@ -101,24 +121,13 @@ as_process_stratum_target (target_ops *target)
 
 extern std::set<process_stratum_target *> all_non_exited_process_targets ();
 
+/* Return a collection of all existing process stratum targets.  */
+
+extern std::set<process_stratum_target *> all_process_targets ();
+
 /* Switch to the first inferior (and program space) of TARGET, and
    switch to no thread selected.  */
 
 extern void switch_to_target_no_thread (process_stratum_target *target);
 
-/* Commit a series of resumption requests previously prepared with
-   target_resume calls.
-
-   This function is a no-op if commit resumes are deferred (see
-   `make_scoped_defer_process_target_commit_resume`).  */
-
-extern void maybe_commit_resume_process_target
-  (process_stratum_target *target);
-
-/* Setup to defer `commit_resume` calls, and re-set to the previous status on
-   destruction.  */
-
-extern scoped_restore_tmpl<bool>
-  make_scoped_defer_process_target_commit_resume ();
-
 #endif /* !defined (PROCESS_STRATUM_TARGET_H) */
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 56ab29479874..fad355afdf4f 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1263,7 +1263,9 @@ record_full_wait_1 (struct target_ops *ops,
 					    "issuing one more step in the "
 					    "target beneath\n");
 		      ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
-		      proc_target->commit_resume ();
+		      proc_target->commit_resumed_state = true;
+		      proc_target->commit_resumed ();
+		      proc_target->commit_resumed_state = false;
 		      continue;
 		    }
 		}
diff --git a/gdb/remote.c b/gdb/remote.c
index f8150f39fb5c..be53886c1837 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -421,7 +421,7 @@ class remote_target : public process_stratum_target
   void detach (inferior *, int) override;
   void disconnect (const char *, int) override;
 
-  void commit_resume () override;
+  void commit_resumed () override;
   void resume (ptid_t, int, enum gdb_signal) override;
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
@@ -6376,6 +6376,8 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
 {
   struct remote_state *rs = get_remote_state ();
 
+  gdb_assert (!this->commit_resumed_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
@@ -6565,7 +6567,7 @@ vcont_builder::push_action (ptid_t ptid, bool step, gdb_signal siggnal)
 /* to_commit_resume implementation.  */
 
 void
-remote_target::commit_resume ()
+remote_target::commit_resumed ()
 {
   int any_process_wildcard;
   int may_global_wildcard_vcont;
@@ -6640,6 +6642,8 @@ remote_target::commit_resume ()
      disable process and global wildcard resumes appropriately.  */
   check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont);
 
+  bool any_pending_vcont_resume = false;
+
   for (thread_info *tp : all_non_exited_threads (this))
     {
       remote_thread_info *priv = get_remote_thread_info (tp);
@@ -6656,6 +6660,9 @@ remote_target::commit_resume ()
 	  continue;
 	}
 
+      if (priv->resume_state () == resume_state::RESUMED_PENDING_VCONT)
+	any_pending_vcont_resume = true;
+
       /* 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.  */
@@ -6663,6 +6670,11 @@ remote_target::commit_resume ()
 	may_global_wildcard_vcont = 0;
     }
 
+  /* We didn't have any resumed thread pending a vCont resume, so nothing to
+     do.  */
+  if (!any_pending_vcont_resume)
+    return;
+
   /* 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
@@ -6735,7 +6747,35 @@ remote_target::commit_resume ()
   vcont_builder.flush ();
 }
 
-\f
+struct stop_reply : public notif_event
+{
+  ~stop_reply ();
+
+  /* The identifier of the thread about this event  */
+  ptid_t ptid;
+
+  /* The remote state this event is associated with.  When the remote
+     connection, represented by a remote_state object, is closed,
+     all the associated stop_reply events should be released.  */
+  struct remote_state *rs;
+
+  struct target_waitstatus ws;
+
+  /* The architecture associated with the expedited registers.  */
+  gdbarch *arch;
+
+  /* Expedited registers.  This makes remote debugging a bit more
+     efficient for those targets that provide critical registers as
+     part of their normal status mechanism (as another roundtrip to
+     fetch them is avoided).  */
+  std::vector<cached_reg_t> regcache;
+
+  enum target_stop_reason stop_reason;
+
+  CORE_ADDR watch_data_address;
+
+  int core;
+};
 
 /* Non-stop version of target_stop.  Uses `vCont;t' to stop a remote
    thread, all threads of a remote process, or all threads of all
@@ -6748,6 +6788,39 @@ remote_target::remote_stop_ns (ptid_t ptid)
   char *p = rs->buf.data ();
   char *endp = p + get_remote_packet_size ();
 
+  gdb_assert (!this->commit_resumed_state);
+
+  /* If any threads that needs to stop are pending a vCont resume, generate
+     dummy stop_reply events.  */
+  for (thread_info *tp : all_non_exited_threads (this, ptid))
+    {
+      remote_thread_info *remote_thr = get_remote_thread_info (tp);
+
+      if (remote_thr->resume_state () == resume_state::RESUMED_PENDING_VCONT)
+	{
+	  if (remote_debug)
+	    {
+	      fprintf_unfiltered (gdb_stdlog,
+				  "remote_stop_ns: Enqueueing phony stop reply "
+				  "for thread pending vCont-resume "
+				  "(%d, %ld, %ld)\n",
+				  tp->ptid.pid(), tp->ptid.lwp (),
+				  tp->ptid.tid ());
+	    }
+
+	  stop_reply *sr = new stop_reply ();
+	  sr->ptid = tp->ptid;
+	  sr->rs = rs;
+	  sr->ws.kind = TARGET_WAITKIND_STOPPED;
+	  sr->ws.value.sig = GDB_SIGNAL_0;
+	  sr->arch = tp->inf->gdbarch;
+	  sr->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+	  sr->watch_data_address = 0;
+	  sr->core = 0;
+	  this->push_stop_reply (sr);
+	}
+    }
+
   /* FIXME: This supports_vCont_probed check is a workaround until
      packet_support is per-connection.  */
   if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN
@@ -6955,36 +7028,6 @@ remote_console_output (const char *msg)
   gdb_stdtarg->flush ();
 }
 
-struct stop_reply : public notif_event
-{
-  ~stop_reply ();
-
-  /* The identifier of the thread about this event  */
-  ptid_t ptid;
-
-  /* The remote state this event is associated with.  When the remote
-     connection, represented by a remote_state object, is closed,
-     all the associated stop_reply events should be released.  */
-  struct remote_state *rs;
-
-  struct target_waitstatus ws;
-
-  /* The architecture associated with the expedited registers.  */
-  gdbarch *arch;
-
-  /* Expedited registers.  This makes remote debugging a bit more
-     efficient for those targets that provide critical registers as
-     part of their normal status mechanism (as another roundtrip to
-     fetch them is avoided).  */
-  std::vector<cached_reg_t> regcache;
-
-  enum target_stop_reason stop_reason;
-
-  CORE_ADDR watch_data_address;
-
-  int core;
-};
-
 /* Return the length of the stop reply queue.  */
 
 int
@@ -7877,6 +7920,8 @@ remote_target::wait_ns (ptid_t ptid, struct target_waitstatus *status,
   int ret;
   int is_notif = 0;
 
+  gdb_assert (!this->commit_resumed_state);
+
   /* If in non-stop mode, get out of getpkt even if a
      notification is received.	*/
 
-- 
2.29.2