gdb: make the remote target track its own thread resume state

The next patch moves the target commit_resume method to be a
process_stratum_target-only method.  The only non-process targets that
currently implement the commit_resume method are the btrace and full
record targets.  The only reason they need to do so is to prevent a
commit resume from reaching the beneath (process) target if they are
currently replaying.

This is important if a record target is used on top of the remote target
(the only process target implementing the commit_resume method).
Currently, the remote target checks the `thread_info::executing` flag of
a thread to know if it should commit resume that thread:

    if (!tp->executing || remote_thr->vcont_resumed)
      continue;

The `tp->executing` flag is set by infrun when it has asked the target
stack to resume the thread, and therefore if the thread is executing,
from its point of view.  It _not_ equivalent to whether the remote
target was asked to resume this thread.

Indeed, if infrun asks the target stack to resume some thread while the
record target is replaying, the record target won't forward the resume
request the remote target beneath, because we don't actually want to
resume the thread on the execution target.  But the `tp->executing` flag
is still set, because from the point of view of infrun, the thread
executes.  So, if the commit_resume call wasn't intercepted by the
record target as it is today and did reach the remote target, the remote
target would say "Oh, this thread should be executing and I haven't
vCont-resumed it!  I must vCont-resume it!".  But that would be wrong,
because it was never asked to resume this thread, the resume request did
not reach it.  This is why the record targets currently need to
implement commit_resume: to prevent the beneath target from
commit_resuming threads it wasn't asked to resume.

Since commit_resume will become a method on process_stratum_target in
the following patch, record targets won't have a chance to intercept the
calls and that would result in the remote target commit_resuming threads
it shouldn't.  To avoid this, this patch makes the remote target track
its own thread resumption state.  That means, tracking which threads it
was asked to resume via target_ops::resume.  Regardless of the context
of this patch, I think this change makes it easier to understand how
resume / commit_resume works in the remote target.  It makes the target
more self-contained, as it only depends on what it gets asked to do via
the target methods, and not on tp->executing, which is a flag maintained
from the point of view of infrun.

I initially made it so this state was only used when the remote target
operates in non-stop mode, since commit_resume is only used when the
target is non-stop.  However, it's more consistent and it can be useful
to maintain this state even in all-stop too.  In all-stop, receiving a
stop notification for one thread means all threads of the target are
considered stopped.

From the point of view of the remote target, there are three states a
thread can be in:

 1. not resumed
 2. resumed but pending vCont-resume
 3. resumed

State 2 only exists when the target is non-stop.

As of this patch, valid state transitions are:

 - 1 -> 2 (through the target resume method if in non-stop)
 - 2 -> 3 (through the target commit_resume method if in non-stop)
 - 1 -> 3 (through the target resume method if in all-stop)
 - 3 -> 1 (through a remote stop notification / reporting an event to the
   event loop)

A subsequent patch will make it possible to go from 2 to 1, in case
infrun asks to stop a thread that was resumed but not commit-resumed
yet.  I don't think it can happen as of now.

In terms of code, this patch replaces the vcont_resumed field with an
enumeration that explicitly represents the three states described above.
The last_resume_sig and last_resume_step fields are moved to a structure
which is clearly identified as only used when the thread is in the
"resumed but pending vCont-resume" state.

gdb/ChangeLog:

	* remote.c (enum class resume_state): New.
	(struct resumed_pending_vcont_info): New.
	(struct remote_thread_info) <resume_state, set_not_resumed,
	set_resumed_pending_vcont, resumed_pending_vcont_info,
	set_resumed, m_resume_state, m_resumed_pending_vcont_info>:
	New.
	<last_resume_step, last_resume_sig, vcont_resumed>: Remove.
	(remote_target::remote_add_thread): Adjust.
	(remote_target::process_initial_stop_replies): Adjust.
	(remote_target::resume): Adjust.
	(remote_target::commit_resume): Rely on state in
	remote_thread_info and not on tp->executing.
	(remote_target::process_stop_reply): Adjust.

Change-Id: I10480919ccb4552faa62575e447a36dbe7c2d523

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 23f6356ceef3e63c587a0d282a1c6eb74576060c..cd4865619c8badc9bb11160c08e9bc78d7c8b013 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2021-01-13  Simon Marchi  <simon.marchi@efficios.com>
+
+       * remote.c (enum class resume_state): New.
+       (struct resumed_pending_vcont_info): New.
+       (struct remote_thread_info) <resume_state, set_not_resumed,
+       set_resumed_pending_vcont, resumed_pending_vcont_info,
+       set_resumed, m_resume_state, m_resumed_pending_vcont_info>:
+       New.
+       <last_resume_step, last_resume_sig, vcont_resumed>: Remove.
+       (remote_target::remote_add_thread): Adjust.
+       (remote_target::process_initial_stop_replies): Adjust.
+       (remote_target::resume): Adjust.
+       (remote_target::commit_resume): Rely on state in
+       remote_thread_info and not on tp->executing.
+       (remote_target::process_stop_reply): Adjust.
+
 2021-01-13  Simon Marchi  <simon.marchi@polymtl.ca>
 
        * arc-tdep.h (arc_debug_printf): New.

diff --git a/gdb/remote.c b/gdb/remote.c
index 6dacc24307eaf358c34f9b9c1633e38df365aa94..a657902080d7bfa42e23fe7eda876f70971cd7a5 100644 (file)
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1050,6 +1050,38 @@ static struct cmd_list_element *remote_show_cmdlist;
 
 static bool use_range_stepping = true;
 
+/* From the remote target's point of view, each thread is in one of these three
+   states.  */
+enum class resume_state
+{
+  /* Not resumed - we haven't been asked to resume this thread.  */
+  NOT_RESUMED,
+
+  /* We have been asked to resume this thread, but haven't sent a vCont action
+     for it yet.  We'll need to consider it next time commit_resume is
+     called.  */
+  RESUMED_PENDING_VCONT,
+
+  /* We have been asked to resume this thread, and we have sent a vCont action
+     for it.  */
+  RESUMED,
+};
+
+/* Information about a thread's pending vCont-resume.  Used when a thread is in
+   the remote_resume_state::RESUMED_PENDING_VCONT state.  remote_target::resume
+   stores this information which is then picked up by
+   remote_target::commit_resume to know which is the proper action for this
+   thread to include in the vCont packet.  */
+struct resumed_pending_vcont_info
+{
+  /* True if the last resume call for this thread was a step request, false
+     if a continue request.  */
+  bool step;
+
+  /* The signal specified in the last resume call for this thread.  */
+  gdb_signal sig;
+};
+
 /* Private data that we'll store in (struct thread_info)->priv.  */
 struct remote_thread_info : public private_thread_info
 {
@@ -1068,23 +1100,61 @@ struct remote_thread_info : public private_thread_info
      to stop for a watchpoint.  */
   CORE_ADDR watch_data_address = 0;
 
-  /* 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.  */
+  /* Get the thread's resume state.  */
+  enum resume_state resume_state () const
+  {
+    return m_resume_state;
+  }
 
-  /* True if the last target_resume call for this thread was a step
-     request, false if a continue request.  */
-  int last_resume_step = 0;
+  /* Put the thread in the NOT_RESUMED state.  */
+  void set_not_resumed ()
+  {
+    m_resume_state = resume_state::NOT_RESUMED;
+  }
 
-  /* The signal specified in the last target_resume call for this
-     thread.  */
-  gdb_signal last_resume_sig = GDB_SIGNAL_0;
+  /* Put the thread in the RESUMED_PENDING_VCONT state.  */
+  void set_resumed_pending_vcont (bool step, gdb_signal sig)
+  {
+    m_resume_state = resume_state::RESUMED_PENDING_VCONT;
+    m_resumed_pending_vcont_info.step = step;
+    m_resumed_pending_vcont_info.sig = sig;
+  }
+
+  /* Get the information this thread's pending vCont-resumption.
 
-  /* Whether this thread was already vCont-resumed on the remote
-     side.  */
-  int vcont_resumed = 0;
+     Must only be called if the thread is in the RESUMED_PENDING_VCONT resume
+     state.  */
+  const struct resumed_pending_vcont_info &resumed_pending_vcont_info () const
+  {
+    gdb_assert (m_resume_state == resume_state::RESUMED_PENDING_VCONT);
+
+    return m_resumed_pending_vcont_info;
+  }
+
+  /* Put the thread in the VCONT_RESUMED state.  */
+  void set_resumed ()
+  {
+    m_resume_state = resume_state::RESUMED;
+  }
+
+private:
+  /* Resume state for this thread.  This is used to implement vCont action
+     coalescing (only when the target operates in non-stop mode).
+
+     remote_target::resume moves the thread to the RESUMED_PENDING_VCONT state,
+     which notes that this thread must be considered in the next commit_resume
+     call.
+
+     remote_target::commit_resume sends a vCont packet with actions for the
+     threads in the RESUMED_PENDING_VCONT state and moves them to the
+     VCONT_RESUMED state.
+
+     When reporting a stop to the core for a thread, that thread is moved back
+     to the NOT_RESUMED state.  */
+  enum resume_state m_resume_state = resume_state::NOT_RESUMED;
+
+  /* Extra info used if the thread is in the RESUMED_PENDING_VCONT state.  */
+  struct resumed_pending_vcont_info m_resumed_pending_vcont_info;
 };
 
 remote_state::remote_state ()
@@ -2443,7 +2513,10 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
   else
     thread = add_thread (this, ptid);
 
-  get_remote_thread_info (thread)->vcont_resumed = executing;
+  /* We start by assuming threads are resumed.  That state then gets updated
+     when we process a matching stop reply.  */
+  get_remote_thread_info (thread)->set_resumed ();
+
   set_executing (this, ptid, executing);
   set_running (this, ptid, running);
 
@@ -4472,7 +4545,7 @@ remote_target::process_initial_stop_replies (int from_tty)
 
       set_executing (this, event_ptid, false);
       set_running (this, event_ptid, false);
-      get_remote_thread_info (evthread)->vcont_resumed = 0;
+      get_remote_thread_info (evthread)->set_not_resumed ();
     }
 
   /* "Notice" the new inferiors before anything related to
@@ -6307,9 +6380,9 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
      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.  */
+     request (put the thread in RESUMED_PENDING_VCONT state); the actual remote
+     resumption will be done in remote_target::commit_resume, where we'll be
+     able to do vCont action coalescing.  */
   if (target_is_non_stop_p () && ::execution_direction != EXEC_REVERSE)
     {
       remote_thread_info *remote_thr;
@@ -6319,8 +6392,11 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
       else
        remote_thr = get_remote_thread_info (this, ptid);
 
-      remote_thr->last_resume_step = step;
-      remote_thr->last_resume_sig = siggnal;
+      /* We don't expect the core to ask to resume an already resumed (from
+         its point of view) thread.  */
+      gdb_assert (remote_thr->resume_state () == resume_state::NOT_RESUMED);
+
+      remote_thr->set_resumed_pending_vcont (step, siggnal);
       return;
     }
 
@@ -6339,6 +6415,10 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
   if (!remote_resume_with_vcont (ptid, step, siggnal))
     remote_resume_with_hc (ptid, step, siggnal);
 
+  /* Update resumed state tracked by the remote target.  */
+  for (thread_info *tp : all_non_exited_threads (this, ptid))
+    get_remote_thread_info (tp)->set_resumed ();
+
   /* 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
@@ -6562,9 +6642,11 @@ remote_target::commit_resume ()
 
   for (thread_info *tp : all_non_exited_threads (this))
     {
+      remote_thread_info *priv = get_remote_thread_info (tp);
+
       /* If a thread of a process is not meant to be resumed, then we
         can't wildcard that process.  */
-      if (!tp->executing)
+      if (priv->resume_state () == resume_state::NOT_RESUMED)
        {
          get_remote_inferior (tp->inf)->may_wildcard_vcont = false;
 
@@ -6593,24 +6675,24 @@ remote_target::commit_resume ()
     {
       remote_thread_info *remote_thr = get_remote_thread_info (tp);
 
-      if (!tp->executing || remote_thr->vcont_resumed)
+      /* If the thread was previously vCont-resumed, no need to send a specific
+        action for it.  If we didn't receive a resume request for it, don't
+        send an action for it either.  */
+      if (remote_thr->resume_state () != resume_state::RESUMED_PENDING_VCONT)
        continue;
 
       gdb_assert (!thread_is_in_step_over_chain (tp));
 
-      if (!remote_thr->last_resume_step
-         && remote_thr->last_resume_sig == GDB_SIGNAL_0
-         && get_remote_inferior (tp->inf)->may_wildcard_vcont)
-       {
-         /* We'll send a wildcard resume instead.  */
-         remote_thr->vcont_resumed = 1;
-         continue;
-       }
+      const resumed_pending_vcont_info &info
+       = remote_thr->resumed_pending_vcont_info ();
 
-      vcont_builder.push_action (tp->ptid,
-                                remote_thr->last_resume_step,
-                                remote_thr->last_resume_sig);
-      remote_thr->vcont_resumed = 1;
+      /* Check if we need to send a specific action for this thread.  If not,
+         it will be included in a wildcard resume instead.  */
+      if (info.step || info.sig != GDB_SIGNAL_0
+         || !get_remote_inferior (tp->inf)->may_wildcard_vcont)
+       vcont_builder.push_action (tp->ptid, info.step, info.sig);
+
+      remote_thr->set_resumed ();
     }
 
   /* Now check whether we can send any process-wide wildcard.  This is
@@ -7764,7 +7846,20 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
       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;
+
+      if (target_is_non_stop_p ())
+       {
+         /* If the target works in non-stop mode, a stop-reply indicates that
+            only this thread stopped.  */
+         remote_thr->set_not_resumed ();
+       }
+      else
+       {
+         /* If the target works in all-stop mode, a stop-reply indicates that
+            all the target's threads stopped.  */
+         for (thread_info *tp : all_non_exited_threads (this))
+           get_remote_thread_info (tp)->set_not_resumed ();
+       }
     }
 
   delete stop_reply;