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

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

* Simon Marchi <simon.marchi@polymtl.ca> [2021-09-01 11:09:31 -0400]:

> 
> 
> On 2021-08-30 4:03 p.m., Andrew Burgess wrote:
> > 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.
> 
> Did you consider collapsing the two fields (resumed and executing) into
> a single one, with three states?  The names are silly, but as an
> example:
> 
> enum class thread_resumption_state
> {
>   NOT_RESUMED,
>   RESUMED_BUT_NOT_TARGET_RESUMED,
>   RESUMED_AND_TARGET_RESUMED,
> };
> 
> I think that would end up simpler than two booleans with rules.  But
> since it would probably be a pretty big change, I will accept "nope" as
> an answer.

I did think about this, but I guess I'm not quite sure how this ends
up being any simpler than the two booleans with rules.

My assumption is that we'd want to keep the API as
executing/set_executing and resumed/set_resumed as this represents the
questions we want to ask.

As such something like set_executing would become:

  void
  thread_info::set_executing (bool e)
  {
    if (m_state == RESUMED_AND_TARGET_RESUMED)
      return;

    gdb_assert (m_state == RESUMED_BUT_NOT_TARGET_RESUMED);

    m_state = RESUMED_AND_TARGET_RESUMED;
  }

Which doesn't feel any less complex to me.

Of course, we could change the API so the user sets the state
directly, like:

  void
  thread_info_::set_executing_resumed_state (enum state_type s)
  {
    /* ... */
  }

But then we'd _still_ end up having to assert the correct state
transitions.

Maybe I'm just not understanding what you're suggesting though...

> 
> > 
> > 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.
> 
> Should clear_stop_pc have the same assertions as set_stop_pc?  I
> guess you tried that and it didn't work?

I've added these in the new patch.

> 
> > @@ -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);
> > +
> 
> This one is in the TARGET_WAITKIND_EXECD handling... I'm wondering if it
> should be target_ops::follow_exec's contract that any new thread of the
> new inferior should be not resumed.  Because follow_exec is always
> called in a context where the event thread on entry is not resumed.  So
> it's not like new threads that are added in target_ops::wait, for
> example.  For those, it makes sense to be added as resumed.
> 
> I think it would just mean adding a set_resumed (false) call in
> process_stratum_target::follow_exec.  And possibly some assertions in
> the follow_exec free function.  For example, I would add to the existing
> assertions:
> 
>   /* If a new inferior was created, the target must have added at least
>      one thread.  If the same inferior was used, we have left one
>      thread.  */
>   gdb_assert (!inf->thread_list.empty ());
> 
>   /* Any added thread must have been added in the not resumed state.  */
>   for (thread_info *thread : inf->threads ())
>     gdb_assert (!thread->resumed ());
> 
> PS: if taking into consideration my comment about the observer seeing
> the correct resumed state, in add_thread_silent (below), then perhaps we
> would need add_thread_silent to take a "resumed" parameter, so it can
> set the right resumed state before calling the observer.

I think I've addressed all of this in the new patch below.  The thread
coming out of ::follow_exec should now be non-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 ());
> 
> I don't know if it matters, but should resumed be set before calling the
> new_thread observer?  So that observers see the thread in the state we
> intend it to be in?

I checked, and as far as I can tell it doesn't currently matter, but I
agree we should try to do the right thing here, so I've changed this
in the new patch version.

Below is V2 of this patch.  There's a lot of change since V1.

 - The add_thread* calls now take a flag, and threads can be started
   as resumed or non-resumed.

 - Some of the state changes, like in ::follow_exec, are now pushed
   back to where the thread is created, as a result, some places where
   in V1 I was changing the state, I'm now asserting the state.

 - Added some asserts to clear_stop_pc

One big consequence of this patch is that we now expect that threads
are created in the "correct" resumed/non-resumed state.  A place where
this is definitely now wrong is during the initial creation of a
process to debug - threads are created in the default resumed state,
which is not correct.

I've fixed this for the native Linux and remote targets, but for all
other targets this will likely be incorrect.

As I'm not able to test these targets I made the choice to, instead of
asserting that GDB has the correct state, fix the state.  This can be
seen in gdb_startup_inferior (fork-child.c) and one in run_command_1
(infcmd.c), where I've added comments about this.

I don't really see a way around this.  If I make the default thread
state be non-resumed then the inferior creation code would work, but
any target specific code to handle creating new threads would
potentially be wrong (as threads would be created non-resumed when
they should be resumed).

I could potentially just add the asserts and let all the other targets
break; then offer help as/when these issues are reported.

Thoughts on this welcome.

Thanks,
Andrew

----

commit 32b05d0b4f46070df5583bc8e411034da8dc4d5a
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Aug 16 12:09:45 2021 +0100

    gdb: make thread_info executing and resumed state more consistent
    
    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, the goal of this commit is to add some 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 these 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.
    
    My initial approach was to retain the idea that threads are always
    created in the non-executing, non-resumed state, and then find all the
    places where new threads are created (which should be in the resumed
    state), and mark the thread resumed in that case.
    
    However, after looking at the changes for a while, it felt like it
    would be simpler to create threads as resumed by default and instead
    force the threads back to non-resumed in the few cases where this was
    appropriate.  The appropriate case being (as far as I can tell), just
    the initial threads created as part of starting up a new inferior.
    
    So, that's what this patch does.  The thread creation routines now
    take a flag to indicate if the new thread should be created resumed or
    not.  Threads created as part of the ::create_inferior process should
    be created non-resumed, while all other threads are created resumed.
    
    The only problem is that this change requires updates to all targets
    that supply ::create_inferior, and I'm mostly only able to test
    GNU/Linux targets.
    
    And so, I have two changes, one in gdb_startup_inferior (fork-child.c)
    and one in run_command_1 (infcmd.c) where, instead of asserting that
    a thread is in a particular resumed/non-resumed state, I am setting
    the thread back to non-resumed.  I believe that this should fix up
    GDB's state for any target that is not (yet) doing the correct thing
    when creating its initial threads.
    
    Here is a description of all the individual changes:
    
      * corelow.c (add_to_thread_list): When adding threads as part of
      setting up a core target, the threads should not be resumed.
    
      (core_target_open): When adding threads as part of setting up a core
      target, the threads should not be resumed.  For sanity, assert that
      the threads of a core target are not resumed once the initial target
      has been setup.
    
      * fork-child.c (gdb_startup_inferior): Upon entry to this function
      no threads in the inferior should be resumed, however, I suspect not
      all targets comply with this assertion, so for now I don't actually
      assert anything.  As we're about to resume the inferior though, mark
      all threads as resumed.
    
      * gdbthread.h (set_stop_pc): Add assert that the stop_pc can only be
      set when a thread is neither resumed, or executing.
    
      (clear_stop_pc): The stop_pc is cleared as part of setting the
      thread executing.  And, as a thread is only set executing after it
      is marked resumed, we can add some asserts to this function to
      validate GDB's state.
    
      (add_thread): Update header comment, add extra 'resumed_p' parameter.
    
      (add_thread_silent): Add extra 'resumed_p' parameter.
    
      (add_thread_with_info): Add extra 'resumed_p' parameter.
    
      * inf-ptrace.c (inf_ptrace_target::create_inferior): The initial
      thread is created non-resumed.
    
      * infcmd.c (post_create_inferior): Only attempt to set the stop_pc
      once per-stop.  I've updated the comment, and re-indented the code
      under the new if condition.
    
      (run_command_1): After creating the inferior all threads should be
      non-resumed, however, I suspect that not all targets comply with
      this assertion, so, instead of asserting this, I set all threads
      back to non-resumed to save any badly behaved targets.
    
      * infrun.c (follow_fork_inferior): After a fork all threads in the
      child should already have been marked as non-resumed.
    
      (follow_exec): Upon entry the thread performing the exec should
      already be marked as non-resumed.  After the exec the selected
      thread should also be non-resumed.
    
      (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): Assert that the thread is not resumed when
      handling an exec event.
    
      (finish_step_over): 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.
    
      * process-stratum-target.c (process_stratum_target::follow_exec):
      If creating a new thread to handle the exec in, then it should be
      created non-resumed.
    
      (process_stratum_target::follow_fork): If creating a new thread to
      follow the child of a fork, the new thread should be non-resumed.
    
      * remote.c (remote_target::add_current_inferior_and_thread): Add
      resumed_p parameter.  Use this parameter to decide if the new thread
      should be started resumed or not.
    
      (remote_target::remote_add_thread): Mark the thread resumed before
      marking it executed.
    
      (remote_target::process_initial_stop_replies): Mark the thread as
      non-resumed.
    
      (remote_target::start_remote): Create the initial thread in a
      resumed state.
    
      (extended_remote_target::create_inferior): Create the initial thread
      in a non-resumed state.
    
      * thread.c (add_thread_silent): Take a new resumed_p parameter, use
      this to set if the new thread is resumed or not.  All new threads
      are created non-executing, so assert this.
    
      (add_thread_with_info): Pass through the new resumed_p parameter.
    
      (add_thread): Pass through the new resumed_p parameter.
    
      (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.  Mark
      the thread executing after clearing the stop_pc, this allows the
      assertion we added to thread_info::clear_stop_pc above to work.
    
      (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.

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 711e86c4cd4..d6846b8f609 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -347,7 +347,8 @@ add_to_thread_list (asection *asect, asection *reg_sect)
 
   ptid_t ptid (pid, lwpid);
 
-  thread_info *thr = add_thread (inf->process_target (), ptid);
+  /* Pass false to indicate the thread should be added non-resumed.  */
+  thread_info *thr = add_thread (inf->process_target (), ptid, false);
 
 /* Warning, Will Robinson, looking at BFD private data! */
 
@@ -505,7 +506,7 @@ core_target_open (const char *arg, int from_tty)
       if (thread == NULL)
 	{
 	  inferior_appeared (current_inferior (), CORELOW_PID);
-	  thread = add_thread_silent (target, ptid_t (CORELOW_PID));
+	  thread = add_thread_silent (target, ptid_t (CORELOW_PID), false);
 	}
 
       switch_to_thread (thread);
@@ -514,6 +515,10 @@ core_target_open (const char *arg, int from_tty)
   if (current_program_space->exec_bfd () == nullptr)
     locate_exec_from_corefile_build_id (core_bfd, from_tty);
 
+  /* Threads in a core file are not resumed.  */
+  for (thread_info *thread : current_inferior ()->threads ())
+    gdb_assert (!thread->resumed ());
+
   post_create_inferior (from_tty);
 
   /* Now go through the target stack looking for threads since there
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 3ce7d64b855..8890ea41e05 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -126,10 +126,24 @@ gdb_startup_inferior (pid_t pid, int num_traps)
   inferior *inf = current_inferior ();
   process_stratum_target *proc_target = inf->process_target ();
 
+  for (thread_info *thread : inf->threads ())
+    {
+      /* Ideally, all targets would created their initial threads in the
+	 non-resumed state, and only when we get here would we mark the
+	 threads as resumed.  However, this is not currently the case, some
+	 targets create their initial threads in the resumed state (for no
+	 particular reason other than historical), and so we can't assert
+	 anything about the state of the inferior threads at this point.
+
+	 What we'd like to say is:  gdb_assert (!thread->resumed ());   */
+      thread->set_resumed (true);
+    }
+
   ptid_t ptid = startup_inferior (proc_target, pid, num_traps, NULL, NULL);
 
   /* Mark all threads non-executing.  */
   set_executing (proc_target, ptid, false);
+  set_resumed (proc_target, ptid, false);
 
   return ptid;
 }
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 4b271943d50..9ffe9233c77 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -345,6 +345,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;
   }
 
@@ -352,6 +354,8 @@ class thread_info : public refcounted_object,
 
   void clear_stop_pc ()
   {
+    gdb_assert (m_resumed);
+    gdb_assert (!m_executing);
     m_suspend.stop_pc.reset ();
   }
 
@@ -547,22 +551,25 @@ using inferior_ref
 /* Create an empty thread list, or empty the existing one.  */
 extern void init_thread_list (void);
 
-/* Add a thread to the thread list, print a message
-   that a new thread is found, and return the pointer to
-   the new thread.  Caller my use this pointer to 
-   initialize the private thread data.  */
+/* Add a thread to the thread list, print a message that a new thread is
+   found, and return the pointer to the new thread.  Caller my use this
+   pointer to initialize the private thread data.  When RESUMED_P is true
+   the thread is created in a resumed state, otherwise, the thread is
+   created in a non-resumed state.  */
 extern struct thread_info *add_thread (process_stratum_target *targ,
-				       ptid_t ptid);
+				       ptid_t ptid, bool resumed_p = true);
 
 /* Same as add_thread, but does not print a message about new
    thread.  */
 extern struct thread_info *add_thread_silent (process_stratum_target *targ,
-					      ptid_t ptid);
+					      ptid_t ptid,
+					      bool resumed_p = true);
 
 /* Same as add_thread, and sets the private info.  */
 extern struct thread_info *add_thread_with_info (process_stratum_target *targ,
 						 ptid_t ptid,
-						 private_thread_info *);
+						 private_thread_info *,
+						 bool resumed_p = true);
 
 /* Delete thread THREAD and notify of thread exit.  If the thread is
    currently not deletable, don't actually delete it but still tag it
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index afa38de6ef7..0366bb940e0 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -94,7 +94,7 @@ inf_ptrace_target::create_inferior (const char *exec_file,
   /* We have something that executes now.  We'll be running through
      the shell at this point (if startup-with-shell is true), but the
      pid shouldn't change.  */
-  thread_info *thr = add_thread_silent (this, ptid);
+  thread_info *thr = add_thread_silent (this, ptid, false);
   switch_to_thread (thr);
 
   unpusher.release ();
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index d948f4bafc5..d31ba5af144 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -244,22 +244,29 @@ post_create_inferior (int from_tty)
      don't need to.  */
   target_find_description ();
 
-  /* Now that we know the register layout, retrieve current PC.  But
-     if the PC is unavailable (e.g., we're opening a core file with
-     missing registers info), ignore it.  */
+  /* Now that we know the register layout, update the stop_pc if it is not
+     already set.  If GDB attached to a running process then the stop_pc
+     will have been set while processing the stop events triggered during
+     the attach.  If this is a core file, or we're just starting a new
+     process, then the stop_pc will not currently be set.
+
+     But, if the PC is unavailable (e.g., we're opening a core file with
+     missing registers info), ignore it.  Obviously, if we're trying to
+     debug a running process and we can't read the PC then this is bad and
+     shouldn't be ignored, but we'll soon hit errors trying to read the PC
+     elsewhere in GDB, so ignoring this here is fine.  */
   thread_info *thr = inferior_thread ();
-
-  thr->clear_stop_pc ();
-  try
-    {
-      regcache *rc = get_thread_regcache (thr);
-      thr->set_stop_pc (regcache_read_pc (rc));
-    }
-  catch (const gdb_exception_error &ex)
-    {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw;
-    }
+  if (!thr->stop_pc_p ())
+    try
+      {
+	regcache *rc = get_thread_regcache (thr);
+	thr->set_stop_pc (regcache_read_pc (rc));
+      }
+    catch (const gdb_exception_error &ex)
+      {
+	if (ex.error != NOT_AVAILABLE_ERROR)
+	  throw;
+      }
 
   if (current_program_space->exec_bfd ())
     {
@@ -453,6 +460,19 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
      shouldn't refer to run_target again.  */
   run_target = NULL;
 
+  /* After creating the inferior its threads are not resumed.  */
+  for (thread_info *thread : current_inferior ()->threads ())
+    {
+      /* Ideally, all targets would leave their threads non-resumed after
+	 the create_inferior call above.  Unfortunately, this is probably
+	 not the case.  The 'probably' here is simply due to an inability
+	 to test all targets.  So, rather than asserting that all threads
+	 are non-resumed here, set the threads to be non-resumed.
+
+	 We'd like to say: gdb_assert (!thread->resumed ());  */
+      thread->set_resumed (false);
+    }
+
   /* We're starting off a new process.  When we get out of here, in
      non-stop mode, finish the state of all threads of that process,
      but leave other threads alone, as they may be stopped in internal
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d1ac9b4cbbb..ca4568ba272 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -580,7 +580,13 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
   /* If there is a child inferior, target_follow_fork must have created a thread
      for it.  */
   if (child_inf != nullptr)
-    gdb_assert (!child_inf->thread_list.empty ());
+    {
+      gdb_assert (!child_inf->thread_list.empty ());
+
+      /* Any added thread should have been marked non-resumed already.  */
+      for (thread_info *thread : child_inf->threads ())
+	gdb_assert (!thread->resumed ());
+    }
 
   /* Detach the parent if needed.  */
   if (follow_child)
@@ -1091,6 +1097,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
      breakpoint or similar, it's gone now.  We cannot truly
      step-to-next statement through an exec().  */
   thread_info *th = inferior_thread ();
+  gdb_assert (!th->resumed ());
   th->control.step_resume_breakpoint = NULL;
   th->control.exception_resume_breakpoint = NULL;
   th->control.single_step_breakpoints = NULL;
@@ -1172,6 +1179,11 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
   gdb_assert (current_inferior () == inf);
   gdb_assert (current_program_space == inf->pspace);
 
+  /* The inferior (and so thread) might have changed depending on which
+     path we took through the above code.  However, the currently selected
+     thread should not be in a resumed state.  */
+  gdb_assert (!inferior_thread ()->resumed ());
+
   /* Attempt to open the exec file.  SYMFILE_DEFER_BP_RESET is used
      because the proper displacement for a PIE (Position Independent
      Executable) main symbol file will only be computed by
@@ -2100,6 +2112,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 +2166,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 +2209,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 +2371,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 +2579,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 +5680,10 @@ 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 switch threads above, then the new thread should still
+	 be in the non-resumed state.  */
+      gdb_assert (!ecs->event_thread->resumed ());
+
       ecs->event_thread->set_stop_pc
 	(regcache_read_pc (get_thread_regcache (ecs->event_thread)));
 
@@ -5865,12 +5933,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 +5943,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 +7596,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/process-stratum-target.c b/gdb/process-stratum-target.c
index 4c726419b99..b6871403317 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -100,7 +100,7 @@ process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid,
 	 may decide to unpush itself from the original inferior's target stack
 	 after that, at its discretion.  */
       follow_inf->push_target (orig_inf->process_target ());
-      thread_info *t = add_thread (follow_inf->process_target (), ptid);
+      thread_info *t = add_thread (follow_inf->process_target (), ptid, false);
 
       /* Leave the new inferior / thread as the current inferior / thread.  */
       switch_to_thread (t);
@@ -118,7 +118,7 @@ process_stratum_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
   if (child_inf != nullptr)
     {
       child_inf->push_target (this);
-      add_thread_silent (this, child_ptid);
+      add_thread_silent (this, child_ptid, false);
     }
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index b6da6b086a2..bb96149f70f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -751,7 +751,8 @@ class remote_target : public process_stratum_target
   int remote_resume_with_vcont (ptid_t ptid, int step,
 				gdb_signal siggnal);
 
-  thread_info *add_current_inferior_and_thread (const char *wait_status);
+  thread_info *add_current_inferior_and_thread (const char *wait_status,
+						bool resumed_p);
 
   ptid_t wait_ns (ptid_t ptid, struct target_waitstatus *status,
 		  target_wait_flags options);
@@ -2546,8 +2547,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;
 }
@@ -4440,7 +4442,8 @@ remote_target::get_current_thread (const char *wait_status)
    The function returns pointer to the main thread of the inferior. */
 
 thread_info *
-remote_target::add_current_inferior_and_thread (const char *wait_status)
+remote_target::add_current_inferior_and_thread (const char *wait_status,
+						bool resumed_p)
 {
   struct remote_state *rs = get_remote_state ();
   bool fake_pid_p = false;
@@ -4471,7 +4474,7 @@ remote_target::add_current_inferior_and_thread (const char *wait_status)
   /* Add the main thread and switch to it.  Don't try reading
      registers yet, since we haven't fetched the target description
      yet.  */
-  thread_info *tp = add_thread_silent (this, curr_ptid);
+  thread_info *tp = add_thread_silent (this, curr_ptid, resumed_p);
   switch_to_thread_no_regs (tp);
 
   return tp;
@@ -4586,6 +4589,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 ();
     }
@@ -4841,7 +4845,8 @@ remote_target::start_remote (int from_tty, int extended_p)
 	  /* Target has no concept of threads at all.  GDB treats
 	     non-threaded target as single-threaded; add a main
 	     thread.  */
-	  thread_info *tp = add_current_inferior_and_thread (wait_status);
+	  thread_info *tp
+	    = add_current_inferior_and_thread (wait_status, true);
 	  get_remote_thread_info (tp)->set_resumed ();
 	}
       else
@@ -10488,7 +10493,7 @@ Remote replied unexpectedly while setting startup-with-shell: %s"),
 
   /* vRun's success return is a stop reply.  */
   stop_reply = run_worked ? rs->buf.data () : NULL;
-  add_current_inferior_and_thread (stop_reply);
+  add_current_inferior_and_thread (stop_reply, false);
 
   /* Get updated offsets, if the stub uses qOffsets.  */
   get_offsets ();
diff --git a/gdb/thread.c b/gdb/thread.c
index 10c3dcd6991..a74ba94bbe8 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -245,7 +245,7 @@ new_thread (struct inferior *inf, ptid_t ptid)
 }
 
 struct thread_info *
-add_thread_silent (process_stratum_target *targ, ptid_t ptid)
+add_thread_silent (process_stratum_target *targ, ptid_t ptid, bool resumed_p)
 {
   gdb_assert (targ != nullptr);
 
@@ -260,6 +260,14 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid)
     delete_thread (tp);
 
   tp = new_thread (inf, ptid);
+
+  /* Upon creation, all threads are non-executing, and non-resumed.  Before
+     notifying observers of the new thread, we set the resumed flag to the
+     desired value.  */
+  gdb_assert (!tp->executing ());
+  tp->set_resumed (resumed_p);
+
+  /* Announce the new thread to all observers.  */
   gdb::observers::new_thread.notify (tp);
 
   return tp;
@@ -267,9 +275,9 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid)
 
 struct thread_info *
 add_thread_with_info (process_stratum_target *targ, ptid_t ptid,
-		      private_thread_info *priv)
+		      private_thread_info *priv, bool resumed_p)
 {
-  thread_info *result = add_thread_silent (targ, ptid);
+  thread_info *result = add_thread_silent (targ, ptid, resumed_p);
 
   result->priv.reset (priv);
 
@@ -281,9 +289,9 @@ add_thread_with_info (process_stratum_target *targ, ptid_t ptid,
 }
 
 struct thread_info *
-add_thread (process_stratum_target *targ, ptid_t ptid)
+add_thread (process_stratum_target *targ, ptid_t ptid, bool resumed_p)
 {
-  return add_thread_with_info (targ, ptid, NULL);
+  return add_thread_with_info (targ, ptid, NULL, resumed_p);
 }
 
 private_thread_info::~private_thread_info () = default;
@@ -322,9 +330,15 @@ thread_info::deletable () const
 void
 thread_info::set_executing (bool executing)
 {
-  m_executing = executing;
+  if (executing == m_executing)
+    return;
+
+  gdb_assert (m_resumed);
+
   if (executing)
     this->clear_stop_pc ();
+
+  m_executing = executing;
 }
 
 /* See gdbthread.h.  */
@@ -335,6 +349,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