[PATCH 0/4] gdb/remote: spot stop packets sent for the wrong thread

[PATCH 0/4] gdb/remote: spot stop packets sent for the wrong thread

[Andrew Burgess]

This isn't a fix for bug gdb/26819, rather, it's an extension to GDB
to hopefully catch the bad packet from the remote before it triggers
an assertion GDB.

Currently it still leaves GDB in a broken state (i.e. unable to
interact with any threads, see patch #4 for more detail), but it at
least gives an error that hint to the user that the problem is the
remote, and stops GDB from falling over with an assertion, which
(understandably) makes it look like GDB is at fault.

I'd love suggestions for how to improve patch #4, so any feedback is
welcome.

Thanks,
Andrew

---

Andrew Burgess (4):
  gdb/remote: Use true/false instead of 1/0
  gdb/remote: better management of remote_state::starting_up flag
  gdb/remote: use remote_add_thread from extended_remote_target::attach
  gdb/remote: error if a stop arrives for a non-resumed thread

 gdb/ChangeLog | 23 +++++++++++++++++++++
 gdb/remote.c  | 56 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 67 insertions(+), 12 deletions(-)

-- 
2.25.4

[PATCH 1/4] gdb/remote: Use true/false instead of 1/0

[Andrew Burgess]

The remote_state::starting_up member variable is already of type bool,
but in some places we still write to it using 1 and 0.  This commit
just updates things to use true and false.

There should be no user visible change after this commit.

gdb/ChangeLog:

	* remote.c (remote_target::start_remote): Set 'starting_up' using
	boolean values instead of integers.
---
 gdb/ChangeLog | 5 +++++
 gdb/remote.c  | 8 ++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index de04aab43dc..531d43a692b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4667,7 +4667,7 @@ remote_target::start_remote (int from_tty, int extended_p)
      Ctrl-C before we're connected and synced up can't interrupt the
      target.  Instead, it offers to drop the (potentially wedged)
      connection.  */
-  rs->starting_up = 1;
+  rs->starting_up = true;
 
   QUIT;
 
@@ -4808,7 +4808,7 @@ remote_target::start_remote (int from_tty, int extended_p)
 
 	  /* We're connected, but not running.  Drop out before we
 	     call start_remote.  */
-	  rs->starting_up = 0;
+	  rs->starting_up = false;
 	  return;
 	}
       else
@@ -4923,7 +4923,7 @@ remote_target::start_remote (int from_tty, int extended_p)
 
 	  /* We're connected, but not running.  Drop out before we
 	     call start_remote.  */
-	  rs->starting_up = 0;
+	  rs->starting_up = false;
 	  return;
 	}
 
@@ -4968,7 +4968,7 @@ remote_target::start_remote (int from_tty, int extended_p)
      target, our symbols have been relocated, and we're merged the
      target's tracepoints with ours.  We're done with basic start
      up.  */
-  rs->starting_up = 0;
+  rs->starting_up = false;
 
   /* Maybe breakpoints are global and need to be inserted now.  */
   if (breakpoints_should_be_inserted_now ())
-- 
2.25.4

[PATCH 2/4] gdb/remote: better management of remote_state::starting_up flag

[Andrew Burgess]

Use scoped_restore to ensure the starting_up flag is always reset to
false when leaving remote_target::start_remote.

There was no bug that inspired this change, rather, while looking at
how this flag was used I spotted that there are a couple of error()
calls in remote_target::start_remote, which would leave GDB with the
remote_state::starting_up flag set to true, which doesn't seem right.

At the end of remote_target::start_remote we were previously setting
remote_state::starting_up back to false before the end of the
function, I've retained this behaviour for now, it should be harmless
to set the starting_up flag back to false and then (later) have the
scoped_restore also set the flag back to false.

I don't believe there should be any user visible changes after this
commit.  In the non-error case I think this is obviously true, the
starting_up flag was set to true on entry to ::start_remote, and was
set back to false before we returned, this is still the case.

In the error () case we would previously leave the ::starting_up flag
set to true, however, this only effected the Ctrl-C handling, thread
creation, or tracepoint download.  As throwing an error during
::start_remote would cause the remote_target to be popped from the
target stack, we would never enter any of the code that checked the
::starting_up flag.  And so, I claim, we will not see any changes in
the error () case either.

gdb/ChangeLog:

	* remote.c (remote_target::start_remote): Use scoped_restore to
	manage remote_state::starting_up flag.
---
 gdb/ChangeLog |  5 +++++
 gdb/remote.c  | 13 +++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 531d43a692b..97268151a59 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4667,7 +4667,9 @@ remote_target::start_remote (int from_tty, int extended_p)
      Ctrl-C before we're connected and synced up can't interrupt the
      target.  Instead, it offers to drop the (potentially wedged)
      connection.  */
-  rs->starting_up = true;
+  gdb_assert (!rs->starting_up);
+  scoped_restore starting_up_restore
+    = make_scoped_restore (&rs->starting_up, true);
 
   QUIT;
 
@@ -4808,7 +4810,6 @@ remote_target::start_remote (int from_tty, int extended_p)
 
 	  /* We're connected, but not running.  Drop out before we
 	     call start_remote.  */
-	  rs->starting_up = false;
 	  return;
 	}
       else
@@ -4923,7 +4924,6 @@ remote_target::start_remote (int from_tty, int extended_p)
 
 	  /* We're connected, but not running.  Drop out before we
 	     call start_remote.  */
-	  rs->starting_up = false;
 	  return;
 	}
 
@@ -4967,7 +4967,12 @@ remote_target::start_remote (int from_tty, int extended_p)
   /* The thread and inferior lists are now synchronized with the
      target, our symbols have been relocated, and we're merged the
      target's tracepoints with ours.  We're done with basic start
-     up.  */
+     up.
+
+     We manually set the starting_up flag here despite having
+     STARTING_UP_RESTORE which will do this for us at the end of our
+     scope, now we are fully connected we want things like Ctrl-C handling
+     to behave as normal during the following code.  */
   rs->starting_up = false;
 
   /* Maybe breakpoints are global and need to be inserted now.  */
-- 
2.25.4

[PATCH 3/4] gdb/remote: use remote_add_thread from extended_remote_target::attach

[Andrew Burgess]

When attaching we currently call add_thread_silent directly, rather
than calling remote_add_thread.  The problem this causes me is that in
remote_add_thread we set the new threads remote_state to RESUMED (new
threads are assumed to be running), but we don't do this in the attach
case.

I believe that this was just an oversight when the new resumed_state
mechanism was introduced.

In a later commit I would like to check that stop packets only arrive
from the remote for threads that are resumed.  As attached threads are
not marked as resumed we are left in a situation where the first stop
packet after an attach arrives for a thread that is not marked as
resumed.

In this commit I have the ::attach code call ::remote_add_thread, this
means that the new thread is correctly marked as resumed.

The only problem is that ::remote_add_thread only calls
add_thread_silent if we are in the process of initialising the remote
target, otherwise it calls add_thread, this would be a change in
behaviour.

To avoid this I considered setting remote_state::starting_up within
the ::attach code, however, this would mean that a Ctrl-C during the
attach would also change its behaviour, which is not what we want.

And so, in this commit, I add a new remote_state::attaching flag,
which is set to true for the duration of ::attach, this flag is then
checked in ::remote_add_thread just like remote_state::starting_up,
and causes GDB to call add_thread_silent.

There's no test for this change, but, after the _next_ commit, if this
change is _not_ included, then we see failures in
gdb.server/ext-attach.exp.

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* remote.c (remote_state) <attaching>: New member variable.
	(remote_target::remote_add_thread): Check the attaching flag too.
	(extended_remote_target::attach): Set remote_state::attaching true
	for the scope of this function, and use remote_add_thread.
---
 gdb/ChangeLog |  7 +++++++
 gdb/remote.c  | 20 +++++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 97268151a59..85ab16e894e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -252,6 +252,9 @@ class remote_state
      about the remote side's threads, relocating symbols, etc.).  */
   bool starting_up = false;
 
+  /* True if we are going through the ::attach code.  */
+  bool attaching = false;
+
   /* If we negotiated packet size explicitly (and thus can bypass
      heuristics for the largest packet size that will not overflow
      a buffer in the stub), this will be set to that packet size.
@@ -2536,7 +2539,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
      consider that a single-threaded target, mentioning a new thread
      might be confusing to the user.  Be silent then, preserving the
      age old behavior.  */
-  if (rs->starting_up)
+  if (rs->starting_up || rs->attaching)
     thread = add_thread_silent (this, ptid);
   else
     thread = add_thread (this, ptid);
@@ -5977,6 +5980,12 @@ extended_remote_target::attach (const char *args, int from_tty)
   int pid;
   char *wait_status = NULL;
 
+  /* Let ::remote-add_thread know we are going through the initial attach
+     process, and so should not announce the first thread to the user.  */
+  gdb_assert (!rs->attaching);
+  scoped_restore attaching_restore
+    = make_scoped_restore (&rs->attaching, true);
+
   pid = parse_pid_to_attach (args);
 
   /* Remote PID can be freely equal to getpid, do not check it here the same
@@ -6045,14 +6054,11 @@ extended_remote_target::attach (const char *args, int from_tty)
 	 ptid.  */
       ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
 
-      /* Add the main thread to the thread list.  */
-      thread_info *thr = add_thread_silent (this, curr_ptid);
+      /* Add the main thread to the thread list, this thread is initially
+	 assumed to be executing/running.  */
+      thread_info *thr = remote_add_thread (curr_ptid, true, true);
 
       switch_to_thread (thr);
-
-      /* Don't consider the thread stopped until we've processed the
-	 saved stop reply.  */
-      set_executing (this, thr->ptid, true);
     }
 
   /* Next, if the target can specify a description, read it.  We do
-- 
2.25.4

[PATCH 4/4] gdb/remote: error if a stop arrives for a non-resumed thread

[Andrew Burgess]

When looking at bug gdb/26819 I spotted that due to the configuration
of OpenOCD, OpenOCD was sending GDB a stop reply for a thread that GDB
was not expecting to be executing.

If GDB was doing a non-displaced step-over, and so, had only sent a
single step request to one specific thread, GDB would, assert that the
thread that stopped should be in the process of completing a
step-over.  This would result in this assertion:

  infrun.c:5672: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.

This commit is an attempt to have GDB spot when the incoming stop
packet from the target is invalid and throw an error before we hit the
above assert.

So, in this commit, when processing the stop in the remote_target
code, if the stop is for a thread that is not resumed, throw an error.

There's no test for this, as it requires that a target send back an
invalid stop packet, one that includes the wrong thread-id, I don't
currently have any good ideas for how I would trigger such a remote
packet.

When looking at the original example from the bug report (which uses
the RISC-V spike simulator and OpenOCD), when the invalid packet
arrives the GDB session now looks like this:

  (gdb) continue
  stop received from remote for thread that should not be executing
  (gdb) info threads
    Id   Target Id                                       Frame
    1    Thread 1 (Name: riscv.cpu0, state: single-step) (running)
  * 2    Thread 2 (Name: riscv.cpu1, state: single-step) (running)

Unfortunately, at this point the session is effectively dead as the
threads are still in the running state, but GDB is in all-stop mode,
so isn't really expecting to be in this state.

Given that, should we get to this point, we know that the remote is
not following the rules, I'm not hugely worried.  Even if we could put
GDB back into a usable state, we couldn't reasonable expect to
continue working with the remote when we know it's broken.

gdb/ChangeLog:

	PR gdb/26819
	* remote.c (remote_target::process_stop_reply): Catch the case
	where a stop reply arrives for a thread that was not resumed.
---
 gdb/ChangeLog |  6 ++++++
 gdb/remote.c  | 21 +++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/gdb/remote.c b/gdb/remote.c
index 85ab16e894e..1bb6f0303a3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8057,6 +8057,27 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
 
       remote_notice_new_inferior (ptid, false);
       remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
+
+      /* Check that the thread that is reported stopped is one that was
+	 previously running.  This catches the case where a badly behaving
+	 target reports a stop for a thread that GDB thinks should already
+	 be stopped.
+
+	 In many cases, reporting a stop for a thread that GDB thinks is
+	 already stopped, turns out to be harmless, however, there are
+	 cases, (e.g. when single stepping), where reporting a stop against
+	 an unexpected thread will trigger assertion failures from within
+	 infrun.
+
+	 The one exception to this logic is when we stop after an exec
+	 call.  The exec will replace the main thread of the inferior, even
+	 if the exec is performed in some other thread.  Thus, GDB might
+	 step thread 2, but the exec-stop is reported in thread 1.  */
+      if (remote_thr->get_resume_state () != resume_state::RESUMED
+	  && status->kind != TARGET_WAITKIND_EXECD)
+	error (_("stop received from remote for thread that should "
+		 "not be executing"));
+
       remote_thr->core = stop_reply->core;
       remote_thr->stop_reason = stop_reply->stop_reason;
       remote_thr->watch_data_address = stop_reply->watch_data_address;
-- 
2.25.4

Re: [PATCH 1/4] gdb/remote: Use true/false instead of 1/0

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2021-06-09 21:06:14 +0100]:

> The remote_state::starting_up member variable is already of type bool,
> but in some places we still write to it using 1 and 0.  This commit
> just updates things to use true and false.
> 
> There should be no user visible change after this commit.
> 
> gdb/ChangeLog:
> 
> 	* remote.c (remote_target::start_remote): Set 'starting_up' using
> 	boolean values instead of integers.

I have pushed just this patch from this series.  The rest of this
series will need a review before it can be merged.

Thanks,
Andrew


> ---
>  gdb/ChangeLog | 5 +++++
>  gdb/remote.c  | 8 ++++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index de04aab43dc..531d43a692b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -4667,7 +4667,7 @@ remote_target::start_remote (int from_tty, int extended_p)
>       Ctrl-C before we're connected and synced up can't interrupt the
>       target.  Instead, it offers to drop the (potentially wedged)
>       connection.  */
> -  rs->starting_up = 1;
> +  rs->starting_up = true;
>  
>    QUIT;
>  
> @@ -4808,7 +4808,7 @@ remote_target::start_remote (int from_tty, int extended_p)
>  
>  	  /* We're connected, but not running.  Drop out before we
>  	     call start_remote.  */
> -	  rs->starting_up = 0;
> +	  rs->starting_up = false;
>  	  return;
>  	}
>        else
> @@ -4923,7 +4923,7 @@ remote_target::start_remote (int from_tty, int extended_p)
>  
>  	  /* We're connected, but not running.  Drop out before we
>  	     call start_remote.  */
> -	  rs->starting_up = 0;
> +	  rs->starting_up = false;
>  	  return;
>  	}
>  
> @@ -4968,7 +4968,7 @@ remote_target::start_remote (int from_tty, int extended_p)
>       target, our symbols have been relocated, and we're merged the
>       target's tracepoints with ours.  We're done with basic start
>       up.  */
> -  rs->starting_up = 0;
> +  rs->starting_up = false;
>  
>    /* Maybe breakpoints are global and need to be inserted now.  */
>    if (breakpoints_should_be_inserted_now ())
> -- 
> 2.25.4
>