[PATCH 3/3] gdb: make thread_info executing and resumed state more consistent

[Andrew Burgess <andrew.burgess@embecosm.com>]

This commit was inspired by this comment from Simon:

  https://sourceware.org/pipermail/gdb-patches/2021-July/180694.html

The comment is about two thread_info flags m_executing and m_resumed.
The m_resumed flag indicates that at the infrun level GDB has set the
thread running, while the m_executing flag indicates that the thread
is actually running at the target level.

It is very common that a thread can be m_resumed, but not m_executing,
that is, core GDB thinks the thread is, or should be, running, but at
the target level the thread isn't currently running.

The comment Simon made was in reply to a patch I posted where I found
a case where a thread was marked m_executing, but not m_resumed, that
is, at the infrun level GDB thought the thread was stopped, but at the
target level the thread was actually running.  Simon suggests that
this feels like an invalid state, and after thinking about it, I
agree.

So, this commit adds some new asserts to GDB.  The core idea here is
that the resumed and executing flags should only change in the
following pattern, to begin with everything is set false:

  Resumed: false
  Executing: false

Then infrun marks the thread resumed:

  Resumed: true
  Executing: false

Then a target starts the thread executing:

  Resumed: true
  Executing: true

The thread stops, the target spots this and marks the thread no longer
executing:

  Resumed: true
  Executing: false

And finally, infrun acknowledges that the thread is now no longer
resumed:

  Resumed: false
  Executing: false

Notice that at no point is resumed false, while executing is true.

And so we can add some asserts:

 1. The resumed flag should only change state when the executing flag
 is false, and

 2. The executing flag should only change state when the resumed flag
 is true.

I added these asserts and ....

.... it turns out these rules are broken all over the place in GDB, we
have problems like:

 (a) When new threads appear they are marked executing, but not
 resumed, and

 (b) In some places we mark a single thread as resumed, but then
 actually set multiple threads executing.

For (a) it could be argued that this is a legitimate state - this is
actually the problem I addressed in the original patch that Simon was
replying too, however, we don't need to support this as a separate
state, so if we can make this case follow the expected set of state
transitions then it allows us to reduce the number of states that GDB
can be in, which I think is a good thing.

Case (b) seems to just be a bug to me.

The interesting changes in this commit then are:

  * gdbthread.h (set_stop_pc): Add assert that the stop_pc can only be
  set when a thread is neither resumed, or executing.

  * infcmd.c (post_create_inferior): Ensure thread is non-resumed
  before clearing the stop_pc.

  * infrun.c (struct scoped_mark_thread_resumed): This new class is
  used to ensure that all the required threads are marked resumed when
  required, this addresses issue (b) above.  I make use of this new
  class in...

  (do_target_resume):  Use scoped_mark_thread_resumed to mark all
  threads resumed prior to actually calling into the target to resume
  the threads.  Placing this call here allows me to remove some calls
  to thread_info::set_resumed() in other places...

  (resume_1): Remove a call to thread_info::set_resumed() from here.

  (handle_inferior_event): Mark the thread as non-resumed prior to
  setting the stop_pc, this thread is stopping.  Additionally, when we
  need to place a thread back into the resumed state so that we can
  later find its pending event, we must mark the thread resumed after
  setting the stop_pc, see the asserts added to set_stop_pc for why.

  (keep_going_stepped_thread): Remove a call to set_resumed thanks to
  our changes in do_target_resume.

  * remote.c (remote_target::remote_add_thread): Mark the new thread
  as resumed.

  (remote_target::process_initial_stop_replies): Mark the thread as
  non-resumed.

  * thread.c (add_thread_silent): New threads are now created in the
  resumed state.  This reflects the reality of how GDB thinks about
  new threads, they appear in a running state, and we then process a
  stop event from (or about) the thread, and bring the thread to a
  stop.  As we most often first see the thread while processing the
  stop event then it makes sense (I think) that the thread starts as
  resumed, but not executing, as core GDB should consider the thread
  resumed at a high level, but in reality the thread is most likely
  stopped due to the event we are currently processing.

  (thread_info::set_executing): Add an early exit patch like we
  already have in thread_info::set_executing().  Also add the
  assertion that is one of the two core asserts for this patch.

  (thread_info::set_resumed): Add the other core assert for this
  patch.

This series has been tested on X86-64 GNU/Linux with the unix,
native-gdbserver, and native-extended-gdbserver boards.
---
 gdb/gdbthread.h |  2 ++
 gdb/infcmd.c    |  1 +
 gdb/infrun.c    | 70 ++++++++++++++++++++++++++++++++++++++++++-------
 gdb/remote.c    |  4 ++-
 gdb/thread.c    | 14 ++++++++++
 5 files changed, 81 insertions(+), 10 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 245445a859b..120315ca25f 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -336,6 +336,8 @@ class thread_info : public refcounted_object,
 
   void set_stop_pc (CORE_ADDR stop_pc)
   {
+    gdb_assert (!m_resumed);
+    gdb_assert (!m_executing);
     m_suspend.stop_pc = stop_pc;
   }
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index d948f4bafc5..34924e17abe 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -249,6 +249,7 @@ post_create_inferior (int from_tty)
      missing registers info), ignore it.  */
   thread_info *thr = inferior_thread ();
 
+  thr->set_resumed (false);
   thr->clear_stop_pc ();
   try
     {
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5554523a049..07847ade325 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2100,6 +2100,52 @@ internal_resume_ptid (int user_step)
     return user_visible_resume_ptid (user_step);
 }
 
+/* Before calling into target code to set inferior threads executing we
+   must mark all threads as resumed.  If an exception is thrown while
+   trying to set the threads executing then we should mark the threads as
+   non-resumed.
+
+   Create an instance of this struct before */
+struct scoped_mark_thread_resumed
+{
+  /* Constructor.  All threads matching PTID will be marked as resumed.  */
+  scoped_mark_thread_resumed (process_stratum_target *targ, ptid_t ptid)
+    : m_target (targ), m_ptid (ptid)
+  {
+    gdb_assert (m_target != nullptr);
+    set_resumed (m_target, m_ptid, true);
+  }
+
+  /* Destructor.  If M_TARGET is not nullptr then mark all threads matching
+     M_PTID as no longer being resumed.  The expectation is that on the
+     exception path this will be called with M_TARGET still set to a valid
+     target.  If however, the threads were successfully set executing then
+     this->commit() will have been called, and M_TARGET will now be
+     nullptr.  */
+  ~scoped_mark_thread_resumed ()
+  {
+    if (m_target != nullptr)
+      set_resumed (m_target, m_ptid, false);
+  }
+
+  /* Called once all of the threads have successfully be set executing (by
+     calling into the target code).  Clears M_TARGET as an indication that,
+     when this object is destructed, we should leave all matching threads
+     as being marked resumed.  */
+  void commit ()
+  {
+    m_target = nullptr;
+  }
+
+private:
+
+  /* The target used for marking threads as resumed or non-resumed.  */
+  process_stratum_target *m_target;
+
+  /* The thread (or threads) to mark as resumed.  */
+  ptid_t m_ptid;
+};
+
 /* Wrapper for target_resume, that handles infrun-specific
    bookkeeping.  */
 
@@ -2108,6 +2154,11 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
 {
   struct thread_info *tp = inferior_thread ();
 
+  /* Create a scoped_mark_thread_resumed to mark all threads matching
+     RESUME_PTID as resumed.  */
+  process_stratum_target *curr_target = current_inferior ()->process_target ();
+  scoped_mark_thread_resumed scoped_resume (curr_target, resume_ptid);
+
   gdb_assert (!tp->stop_requested);
 
   /* Install inferior's terminal modes.  */
@@ -2146,6 +2197,9 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
 
   if (target_can_async_p ())
     target_async (1);
+
+  /* Call commit so SCOPED_RESUME leaves threads marked as resumed.  */
+  scoped_resume.commit ();
 }
 
 /* Resume the inferior.  SIG is the signal to give the inferior
@@ -2305,7 +2359,6 @@ resume_1 (enum gdb_signal sig)
 
 	      resume_ptid = internal_resume_ptid (user_step);
 	      do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
-	      tp->set_resumed (true);
 	      return;
 	    }
 	}
@@ -2514,7 +2567,6 @@ resume_1 (enum gdb_signal sig)
     }
 
   do_target_resume (resume_ptid, step, sig);
-  tp->set_resumed (true);
 }
 
 /* Resume the inferior.  SIG is the signal to give the inferior
@@ -5616,6 +5668,9 @@ handle_inferior_event (struct execution_control_state *ecs)
 	 execd thread for that case (this is a nop otherwise).  */
       ecs->event_thread = inferior_thread ();
 
+      /* If we did select a new thread, make sure its non-resumed.  */
+      ecs->event_thread->set_resumed (false);
+
       ecs->event_thread->set_stop_pc
 	(regcache_read_pc (get_thread_regcache (ecs->event_thread)));
 
@@ -5865,12 +5920,6 @@ finish_step_over (struct execution_control_state *ecs)
 
 	  /* Record the event thread's event for later.  */
 	  save_waitstatus (tp, &ecs->ws);
-	  /* This was cleared early, by handle_inferior_event.  Set it
-	     so this pending event is considered by
-	     do_target_wait.  */
-	  tp->set_resumed (true);
-
-	  gdb_assert (!tp->executing ());
 
 	  regcache = get_thread_regcache (tp);
 	  tp->set_stop_pc (regcache_read_pc (regcache));
@@ -5881,6 +5930,10 @@ finish_step_over (struct execution_control_state *ecs)
 			       target_pid_to_str (tp->ptid).c_str (),
 			       currently_stepping (tp));
 
+	  /* This was cleared early, by handle_inferior_event.  Set it so
+	     this pending event is considered by do_target_wait.  */
+	  tp->set_resumed (true);
+
 	  /* This in-line step-over finished; clear this so we won't
 	     start a new one.  This is what handle_signal_stop would
 	     do, if we returned false.  */
@@ -7530,7 +7583,6 @@ keep_going_stepped_thread (struct thread_info *tp)
 				     get_frame_address_space (frame),
 				     tp->stop_pc ());
 
-      tp->set_resumed (true);
       resume_ptid = internal_resume_ptid (tp->control.stepping_command);
       do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
     }
diff --git a/gdb/remote.c b/gdb/remote.c
index b6da6b086a2..0cfed0bdf65 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2546,8 +2546,9 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
      when we process a matching stop reply.  */
   get_remote_thread_info (thread)->set_resumed ();
 
-  set_executing (this, ptid, executing);
   set_running (this, ptid, running);
+  set_resumed (this, ptid, running);
+  set_executing (this, ptid, executing);
 
   return thread;
 }
@@ -4586,6 +4587,7 @@ remote_target::process_initial_stop_replies (int from_tty)
 	evthread->set_pending_waitstatus (ws);
 
       set_executing (this, event_ptid, false);
+      set_resumed (this, event_ptid, false);
       set_running (this, event_ptid, false);
       get_remote_thread_info (evthread)->set_not_resumed ();
     }
diff --git a/gdb/thread.c b/gdb/thread.c
index 10c3dcd6991..aceb233be80 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -262,6 +262,13 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid)
   tp = new_thread (inf, ptid);
   gdb::observers::new_thread.notify (tp);
 
+  /* All threads are created in a resumed state, that is as soon as GDB
+     sees a new thread it is expected to be running as far as the core of
+     GDB is concerned.  At a target level the thread is probably stopped
+     right now, hence the executing flag is left initialized to false.  */
+  tp->set_resumed (true);
+  gdb_assert (!tp->executing ());
+
   return tp;
 }
 
@@ -322,6 +329,11 @@ thread_info::deletable () const
 void
 thread_info::set_executing (bool executing)
 {
+  if (executing == m_executing)
+    return;
+
+  gdb_assert (m_resumed);
+
   m_executing = executing;
   if (executing)
     this->clear_stop_pc ();
@@ -335,6 +347,8 @@ thread_info::set_resumed (bool resumed)
   if (resumed == m_resumed)
     return;
 
+  gdb_assert (!m_executing);
+
   process_stratum_target *proc_target = this->inf->process_target ();
 
   /* If we transition from resumed to not resumed, we might need to remove
-- 
2.25.4