diff mbox

[PATCHv3,2/2] gdb/remote: Restore support for 'S' stop reply packet

Message ID f8df2b2e16a5f95a13a760fecb9d04cb641ddd22.1583149853.git.andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess March 2, 2020, 11:54 a.m. UTC
With this commit:

  commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2
  Date:   Fri Jan 10 20:06:08 2020 +0000

      Multi-target support

There was a regression in GDB's support for older aspects of the
remote protocol.  Specifically, when a target sends the 'S' stop reply
packet (which doesn't include a thread-id) then GDB has to figure out
which thread actually stopped.

Before the above commit GDB figured this out by using inferior_ptid in
process_stop_reply, which contained the ptid of the current
process/thread.  This would be fine for single threaded
inferiors (which is the only place using an S packet makes sense), but
in the general case, relying on inferior_ptid for processing a stop is
wrong - there's no reason to believe that what was GDB's current
thread will be the same thread that just stopped in the inferior.

With the above commit the inferior_ptid now has the value null_ptid
inside process_stop_reply, this can be seen in do_target_wait, where
we call switch_to_inferior_no_thread before calling do_target_wait_1.

The problem this causes can be seen in the new test that runs
gdbserver using the flag --disable-packet=T, and causes GDB to throw
this assertion:

  inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.

A similar problem was fixed in this commit:

  commit 3cada74087687907311b52781354ff551e10a0ed
  Date:   Thu Jan 11 00:23:04 2018 +0000

      Fix backwards compatibility with old GDBservers (PR remote/22597)

However, this commit deals with the case where the T packet doesn't
include a thread-id, not the S packet case.  This commit solves the
problem providing a thread-id at the GDB side if the remote target
doesn't provide one.  The thread-id provided comes from
remote_state::general_thread, however, though this does work, I don't
think it is the ideal solution.

The remote_state tracks two threads, the continue_thread and the
general_thread, these are updated when GDB asks the remote target to
switch threads.  The general_thread is set before performing things
like register or memory accesses, and the continue_thread is set
before things like continue or step commands.  Further, the
general_thread is updated after a target stops to reference the thread
that stopped.

The first thing to note from the above description is that we have a
cycle of dependency, when a T packet arrives without a thread-id we
fill in the thread-id from the general_thread data.  The thread-id
from the stop event is then used to set the general_thread.  This in
itself feels a little weird.

The second question is why use the general_thread at all? You'd think
given how they are originally set that the continue thread would be a
better choice.  The problem with this is that the continue_thread, if
the user just does "continue", will be set to the minus_one_ptid, in
the remote protocol this means all threads.  When the stop arrives
with no thread-id and we use continue_thread we end up with a very
similar assertion to before because we now end up trying to lookup a
thread using the minus_one_ptid.  By contrast, once GDB has connected
to a remote target the general_thread will be set to a valid
thread-id, after which, if the inferior is single threaded, and stop
events arrive without a thread-id, everything works fine.

There is one slight weirdness with the above behaviour though.  When
GDB first connects to the remote target inferior_ptid is null_ptid,
however, upon connecting we query the remote for its threads.  As the
thread information arrives GDB adds the threads to its internal
database, and this process involves setting inferior_ptid to the id of
each new thread in turn.  Once we know about all the threads we wait
for a stop event from the remote target to indicate that GDB is now in
control of the inferior.

The problem is that after adding the new threads we don't reset
inferior_ptid, and the code path we use to wait for the inferior for
this first wait also doesn't reset inferior_ptid, it just turns out
that during the initial connection inferior_ptid is not null_ptid.
This is lucky, because during the initial connection the
general_thread variable _is_ set to null_ptid.

So, during the initial connection, if the first stop event is missing
a thread-id then we "provide" a thead-id from general_thread.  This
turns out to also be null_ptid meaning no thread-id is known, and then
during ::process_stop_reply we fill in the missing thread-id using
inferior_ptid.

This was all discussed on the mailing list here:

  https://sourceware.org/ml/gdb-patches/2020-02/msg01011.html

My proposal for a fix then is:

 1. Move the call to switch_to_inferior_no_thread into
 do_target_wait_1, this means that in call cases where we are waiting
 for an inferior the inferior_ptid will be set to null_ptid.  This is
 good as no wait code should rely on inferior_ptid.

 2. Remove the use of general_thread from the 'T' packet processing.
 The general_thread read here was only ever correct by chance, and we
 shouldn't be using it this way.

 3. Remove use of inferior_ptid from ::process_stop_event as this is
 wrong, and will always be null_ptid now anyway.

 4. When a stop_even has null_ptid due to a lack of thread-id (either
 from a T packet or an S packet) then pick the first non exited thread
 in the inferior and use that.  This will be fine for single threaded
 inferiors.  A multi-threaded inferior really should be using T
 packets with a thread-id, so we give a warning if the inferior is
 multi-threaded, and we are still missing a thread-id.

 5. Extend the existing test that covered the T packet with missing
 thread-id to also cover the S packet.

gdb/ChangeLog:

	* remote.c (remote_target::remote_parse_stop_reply): Don't use the
	general_thread if the stop reply is missing a thread-id.
	(remote_target::process_stop_reply): Use the first non-exited
	thread if the target didn't pass a thread-id.
	* infrun.c (do_target_wait): Move call to
	switch_to_inferior_no_thread to ....
	(do_target_wait_1): ... here.

gdb/testsuite/ChangeLog:

	* gdb.server/stop-reply-no-thread.exp: Add test where T packet is
	disabled.
---
 gdb/ChangeLog                                     | 10 +++
 gdb/infrun.c                                      |  8 ++-
 gdb/remote.c                                      | 43 ++++++++----
 gdb/testsuite/ChangeLog                           |  5 ++
 gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++---------
 5 files changed, 101 insertions(+), 45 deletions(-)

Comments

Pedro Alves March 2, 2020, 12:25 p.m. UTC | #1
On 3/2/20 11:54 AM, Andrew Burgess wrote:

> My proposal for a fix then is:
> 
>  1. Move the call to switch_to_inferior_no_thread into
>  do_target_wait_1, this means that in call cases where we are waiting

"in all cases"

>  for an inferior the inferior_ptid will be set to null_ptid.  This is
>  good as no wait code should rely on inferior_ptid.
> 
>  2. Remove the use of general_thread from the 'T' packet processing.
>  The general_thread read here was only ever correct by chance, and we
>  shouldn't be using it this way.
> 
>  3. Remove use of inferior_ptid from ::process_stop_event as this is
>  wrong, and will always be null_ptid now anyway.
> 
>  4. When a stop_even has null_ptid due to a lack of thread-id (either

"stop_even" -> "stop_event"

>  from a T packet or an S packet) then pick the first non exited thread
>  in the inferior and use that.  This will be fine for single threaded

"in the inferior" -> "in the target".

>  inferiors.  A multi-threaded inferior really should be using T

Instead of
  "A multi-threaded inferior",
we should say
  "A multi-thread or multi-inferior aware remote server/stub"
or something around that.

>  packets with a thread-id, so we give a warning if the inferior is
>  multi-threaded, and we are still missing a thread-id.

"inferior" -> "target".

The "inferior" -> "target" distinction I'm making in these
small remarks above matters, because say the remote server
is debugging two single-threaded inferiors.  We still want to
(and do) warn.

> 
>  5. Extend the existing test that covered the T packet with missing
>  thread-id to also cover the S packet.

Excellent.

> 
> gdb/ChangeLog:
> 
> 	* remote.c (remote_target::remote_parse_stop_reply): Don't use the
> 	general_thread if the stop reply is missing a thread-id.
> 	(remote_target::process_stop_reply): Use the first non-exited
> 	thread if the target didn't pass a thread-id.
> 	* infrun.c (do_target_wait): Move call to
> 	switch_to_inferior_no_thread to ....
> 	(do_target_wait_1): ... here.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.server/stop-reply-no-thread.exp: Add test where T packet is
> 	disabled.
> ---
>  gdb/ChangeLog                                     | 10 +++
>  gdb/infrun.c                                      |  8 ++-
>  gdb/remote.c                                      | 43 ++++++++----
>  gdb/testsuite/ChangeLog                           |  5 ++
>  gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++---------
>  5 files changed, 101 insertions(+), 45 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d9a6f733519..43199b17b05 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3456,6 +3456,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
>    ptid_t event_ptid;
>    struct thread_info *tp;
>  
> +  /* We know that we are looking for an event in inferior INF, but we don't

"in the inferior INF" -> "in the target of inferior INF".

The distinction is important -- target_wait may well return an event
for an inferior different from INF.

> +     know which thread the event might come from.  As such we want to make
> +     sure that INFERIOR_PTID is reset so that non of the wait code relies

"that non of" -> "that none of" ?

> +     on it - doing so is always a mistake.  */
> +  switch_to_inferior_no_thread (inf);
> +

OK with the nits above fixed.  Thanks much for doing this!

Pedro Alves
Andrew Burgess March 2, 2020, 3:19 p.m. UTC | #2
Thanks for all the feedback.  I pushed the patches with the fixes you
suggested.

* Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]:

> On 3/2/20 11:54 AM, Andrew Burgess wrote:
> 
> > My proposal for a fix then is:
> > 
> >  1. Move the call to switch_to_inferior_no_thread into
> >  do_target_wait_1, this means that in call cases where we are waiting
> 
> "in all cases"
> 
> >  for an inferior the inferior_ptid will be set to null_ptid.  This is
> >  good as no wait code should rely on inferior_ptid.
> > 
> >  2. Remove the use of general_thread from the 'T' packet processing.
> >  The general_thread read here was only ever correct by chance, and we
> >  shouldn't be using it this way.
> > 
> >  3. Remove use of inferior_ptid from ::process_stop_event as this is
> >  wrong, and will always be null_ptid now anyway.
> > 
> >  4. When a stop_even has null_ptid due to a lack of thread-id (either
> 
> "stop_even" -> "stop_event"
> 
> >  from a T packet or an S packet) then pick the first non exited thread
> >  in the inferior and use that.  This will be fine for single threaded
> 
> "in the inferior" -> "in the target".
> 
> >  inferiors.  A multi-threaded inferior really should be using T
> 
> Instead of
>   "A multi-threaded inferior",
> we should say
>   "A multi-thread or multi-inferior aware remote server/stub"
> or something around that.
> 
> >  packets with a thread-id, so we give a warning if the inferior is
> >  multi-threaded, and we are still missing a thread-id.
> 
> "inferior" -> "target".
> 
> The "inferior" -> "target" distinction I'm making in these
> small remarks above matters, because say the remote server
> is debugging two single-threaded inferiors.  We still want to
> (and do) warn.

I hadn't really considered this case, however, this raises a
follow on question:

Before entering the target wait code we call
switch_to_inferior_no_thread, partly, as I understand it because
having inferior_ptid pointing at a thread leads to invalid code
that relies on this thread being _the_ event thread, when really we
need to extract the inferior and thread-id from the stop event.

So, why do we allow the current_inferior to remain valid while we
perform the wait?  Instead, why don't we clear both current_inferior
and inferior_ptid, and then enter the wait code?

Thanks,
Andrew

> 
> > 
> >  5. Extend the existing test that covered the T packet with missing
> >  thread-id to also cover the S packet.
> 
> Excellent.
> 
> > 
> > gdb/ChangeLog:
> > 
> > 	* remote.c (remote_target::remote_parse_stop_reply): Don't use the
> > 	general_thread if the stop reply is missing a thread-id.
> > 	(remote_target::process_stop_reply): Use the first non-exited
> > 	thread if the target didn't pass a thread-id.
> > 	* infrun.c (do_target_wait): Move call to
> > 	switch_to_inferior_no_thread to ....
> > 	(do_target_wait_1): ... here.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.server/stop-reply-no-thread.exp: Add test where T packet is
> > 	disabled.
> > ---
> >  gdb/ChangeLog                                     | 10 +++
> >  gdb/infrun.c                                      |  8 ++-
> >  gdb/remote.c                                      | 43 ++++++++----
> >  gdb/testsuite/ChangeLog                           |  5 ++
> >  gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++---------
> >  5 files changed, 101 insertions(+), 45 deletions(-)
> > 
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index d9a6f733519..43199b17b05 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -3456,6 +3456,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
> >    ptid_t event_ptid;
> >    struct thread_info *tp;
> >  
> > +  /* We know that we are looking for an event in inferior INF, but we don't
> 
> "in the inferior INF" -> "in the target of inferior INF".
> 
> The distinction is important -- target_wait may well return an event
> for an inferior different from INF.
> 
> > +     know which thread the event might come from.  As such we want to make
> > +     sure that INFERIOR_PTID is reset so that non of the wait code relies
> 
> "that non of" -> "that none of" ?
> 
> > +     on it - doing so is always a mistake.  */
> > +  switch_to_inferior_no_thread (inf);
> > +
> 
> OK with the nits above fixed.  Thanks much for doing this!
> 
> Pedro Alves
>
Pedro Alves March 2, 2020, 7:07 p.m. UTC | #3
On 3/2/20 3:19 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]:

>> "inferior" -> "target".
>>
>> The "inferior" -> "target" distinction I'm making in these
>> small remarks above matters, because say the remote server
>> is debugging two single-threaded inferiors.  We still want to
>> (and do) warn.
> 
> I hadn't really considered this case, however, this raises a
> follow on question:
> 
> Before entering the target wait code we call
> switch_to_inferior_no_thread, partly, as I understand it because
> having inferior_ptid pointing at a thread leads to invalid code
> that relies on this thread being _the_ event thread, when really we
> need to extract the inferior and thread-id from the stop event.

The main reason we switch the inferior is that the target stack is a
property of the inferior now.  Each inferior has its own target stack
(see inferior::m_target_stack).  Note, target stack, not target.
A single target may be used by different inferiors.  E.g.,
when remote debugging, if you're debugging two inferiors under control
of the same gdbserver, each inferior will have a pointer to the
same remote target instance in its target stack.

To call any target method, we must be sure to make the relevant
inferior current, because target methods consult the inferior's
target stack to know which target to call.  When target methods
defer to the target beneath, they'll again consult the target stack
stored in the current inferior (the "this->beneath ()->foo()" calls
in target-delegates.c).

So to call the target_wait method to poll events out of some target,
you need to switch to some representative inferior that uses the
process_stratum target you want to poll.

However, a reason we need to switch to all inferiors in turn and not
just a subset sufficient to cover all instantiated process_stratum
targets, is the strata above process_stratum, like record_stratum.
Those targets aren't shared between inferiors, and they generate
target events as well.

I suspect it may be possible to come up with a cleaner design here,
but I haven't been able to come up with one that's as simple as the
current one, given all the constraints of the rest of current
gdb's design.  I didn't try very hard though.

Since we're switching the inferior around, the question 
of -- which thread in the inferior should we pick? -- immediately
arises.  And the best answer is "none".  Switching to no-thread also
has the property that it "catches" target backends relying on
inferior_ptid to infer things about the next stopping event,
when they should not.  (Another point that helps with seeing how that's
wrong is to consider that inferior_ptid includes no field
that would indicate which target instance the ptid came from.
So e.g., if you're debugging against two remote stubs, it could well
be the case that when you get to target_wait, inferior_ptid points at
a thread of the other target, even though it seemingly looks
like a valid thread in the current target.)

> 
> So, why do we allow the current_inferior to remain valid while we
> perform the wait?  Instead, why don't we clear both current_inferior
> and inferior_ptid, and then enter the wait code?

It's impossible to clear current_inferior with the current design.
There must always be a current inferior.

Thanks,
Pedro Alves
Andrew Burgess March 2, 2020, 7:24 p.m. UTC | #4
* Pedro Alves <palves@redhat.com> [2020-03-02 19:07:33 +0000]:

> On 3/2/20 3:19 PM, Andrew Burgess wrote:
> > * Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]:
> 
> >> "inferior" -> "target".
> >>
> >> The "inferior" -> "target" distinction I'm making in these
> >> small remarks above matters, because say the remote server
> >> is debugging two single-threaded inferiors.  We still want to
> >> (and do) warn.
> > 
> > I hadn't really considered this case, however, this raises a
> > follow on question:
> > 
> > Before entering the target wait code we call
> > switch_to_inferior_no_thread, partly, as I understand it because
> > having inferior_ptid pointing at a thread leads to invalid code
> > that relies on this thread being _the_ event thread, when really we
> > need to extract the inferior and thread-id from the stop event.
> 
> The main reason we switch the inferior is that the target stack is a
> property of the inferior now.  Each inferior has its own target stack
> (see inferior::m_target_stack).  Note, target stack, not target.
> A single target may be used by different inferiors.  E.g.,
> when remote debugging, if you're debugging two inferiors under control
> of the same gdbserver, each inferior will have a pointer to the
> same remote target instance in its target stack.
> 
> To call any target method, we must be sure to make the relevant
> inferior current, because target methods consult the inferior's
> target stack to know which target to call.  When target methods
> defer to the target beneath, they'll again consult the target stack
> stored in the current inferior (the "this->beneath ()->foo()" calls
> in target-delegates.c).
> 
> So to call the target_wait method to poll events out of some target,
> you need to switch to some representative inferior that uses the
> process_stratum target you want to poll.
> 
> However, a reason we need to switch to all inferiors in turn and not
> just a subset sufficient to cover all instantiated process_stratum
> targets, is the strata above process_stratum, like record_stratum.
> Those targets aren't shared between inferiors, and they generate
> target events as well.
> 
> I suspect it may be possible to come up with a cleaner design here,
> but I haven't been able to come up with one that's as simple as the
> current one, given all the constraints of the rest of current
> gdb's design.  I didn't try very hard though.
> 
> Since we're switching the inferior around, the question 
> of -- which thread in the inferior should we pick? -- immediately
> arises.  And the best answer is "none".  Switching to no-thread also
> has the property that it "catches" target backends relying on
> inferior_ptid to infer things about the next stopping event,
> when they should not.  (Another point that helps with seeing how that's
> wrong is to consider that inferior_ptid includes no field
> that would indicate which target instance the ptid came from.
> So e.g., if you're debugging against two remote stubs, it could well
> be the case that when you get to target_wait, inferior_ptid points at
> a thread of the other target, even though it seemingly looks
> like a valid thread in the current target.)
> 
> > 
> > So, why do we allow the current_inferior to remain valid while we
> > perform the wait?  Instead, why don't we clear both current_inferior
> > and inferior_ptid, and then enter the wait code?
> 
> It's impossible to clear current_inferior with the current design.
> There must always be a current inferior.

Thanks for the explanation.

Andrew
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d9a6f733519..43199b17b05 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3456,6 +3456,12 @@  do_target_wait_1 (inferior *inf, ptid_t ptid,
   ptid_t event_ptid;
   struct thread_info *tp;
 
+  /* We know that we are looking for an event in inferior INF, but we don't
+     know which thread the event might come from.  As such we want to make
+     sure that INFERIOR_PTID is reset so that non of the wait code relies
+     on it - doing so is always a mistake.  */
+  switch_to_inferior_no_thread (inf);
+
   /* First check if there is a resumed thread with a wait status
      pending.  */
   if (ptid == minus_one_ptid || ptid.is_pid ())
@@ -3651,8 +3657,6 @@  do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, int options)
 
   auto do_wait = [&] (inferior *inf)
   {
-    switch_to_inferior_no_thread (inf);
-
     ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options);
     ecs->target = inf->process_target ();
     return (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
diff --git a/gdb/remote.c b/gdb/remote.c
index 4a70ab3fb0d..9b73faf9a34 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7402,18 +7402,14 @@  Packet: '%s'\n"),
 		     reported expedited registers.  */
 		  if (event->ptid == null_ptid)
 		    {
+		      /* If there is no thread-id information then leave
+			 the event->ptid as null_ptid.  Later in
+			 process_stop_reply we will pick a suitable
+			 thread.  */
 		      const char *thr = strstr (p1 + 1, ";thread:");
 		      if (thr != NULL)
 			event->ptid = read_ptid (thr + strlen (";thread:"),
 						 NULL);
-		      else
-			{
-			  /* Either the current thread hasn't changed,
-			     or the inferior is not multi-threaded.
-			     The event must be for the thread we last
-			     set as (or learned as being) current.  */
-			  event->ptid = event->rs->general_thread;
-			}
 		    }
 
 		  if (rsa == NULL)
@@ -7668,10 +7664,35 @@  remote_target::process_stop_reply (struct stop_reply *stop_reply,
   *status = stop_reply->ws;
   ptid = stop_reply->ptid;
 
-  /* If no thread/process was reported by the stub, assume the current
-     inferior.  */
+  /* If no thread/process was reported by the stub then use the first
+     non-exited thread in the current target.  */
   if (ptid == null_ptid)
-    ptid = inferior_ptid;
+    {
+      for (thread_info *thr : all_non_exited_threads (this))
+	{
+	  if (ptid != null_ptid)
+	    {
+	      static bool warned = false;
+
+	      if (!warned)
+		{
+		  /* If you are seeing this warning then the remote target
+		     has multiple threads and either sent an 'S' stop
+		     packet, or a 'T' stop packet without a thread-id.  In
+		     both of these cases GDB is unable to know which thread
+		     just stopped and is now having to guess.  The correct
+		     action is to fix the remote target to send the correct
+		     packet (a 'T' packet and include a thread-id).  */
+		  warning (_("multi-threaded target stopped without sending "
+			     "a thread-id, using first non-exited thread"));
+		  warned = true;
+		}
+	      break;
+	    }
+	  ptid = thr->ptid;
+	}
+      gdb_assert (ptid != null_ptid);
+    }
 
   if (status->kind != TARGET_WAITKIND_EXITED
       && status->kind != TARGET_WAITKIND_SIGNALLED
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
index 45407bc31de..ffc1c27dcb4 100644
--- a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
@@ -32,43 +32,59 @@  if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
     return -1
 }
 
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
+# Run the tests with different features of GDBserver disabled.
+proc run_test { disable_feature } {
+    global binfile gdb_prompt decimal
 
-# Start GDBserver, with ";thread:NNN" in T stop replies disabled,
-# emulating old gdbservers when debugging single-threaded programs.
-set res [gdbserver_start "--disable-packet=Tthread" $binfile]
-set gdbserver_protocol [lindex $res 0]
-set gdbserver_gdbport [lindex $res 1]
+    clean_restart ${binfile}
 
-# Disable XML-based thread listing, and multi-process extensions.
-gdb_test_no_output "set remote threads-packet off"
-gdb_test_no_output "set remote multiprocess-feature-packet off"
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
 
-set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
-if ![gdb_assert {$res == 0} "connect"] {
-    return
-}
+    set res [gdbserver_start "--disable-packet=${disable_feature}" $binfile]
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
 
-# There should be only one thread listed.
-set test "info threads"
-gdb_test_multiple $test $test {
-    -re "2 Thread.*$gdb_prompt $" {
-	fail $test
-    }
-    -re "has terminated.*$gdb_prompt $" {
-	fail $test
+    # Disable XML-based thread listing, and multi-process extensions.
+    gdb_test_no_output "set remote threads-packet off"
+    gdb_test_no_output "set remote multiprocess-feature-packet off"
+
+    set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+    if ![gdb_assert {$res == 0} "connect"] {
+	return
     }
-    -re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" {
-	pass $test
+
+    # There should be only one thread listed.
+    set test "info threads"
+    gdb_test_multiple $test $test {
+	-re "2 Thread.*$gdb_prompt $" {
+	    fail $test
+	}
+	-re "has terminated.*$gdb_prompt $" {
+	    fail $test
+	}
+	-re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" {
+	    pass $test
+	}
     }
-}
 
-gdb_breakpoint "main"
+    gdb_breakpoint "main"
 
-# Bad GDB behaved like this:
-#  (gdb) c
-#  Cannot execute this command without a live selected thread.
-#  (gdb)
-gdb_test "c" "Breakpoint $decimal, main.*" "continue to main"
+    # Bad GDB behaved like this:
+    #  (gdb) c
+    #  Cannot execute this command without a live selected thread.
+    #  (gdb)
+    gdb_test "c" "Breakpoint $decimal, main.*" "continue to main"
+}
+
+# Disable different features within gdbserver:
+#
+# Tthread: Start GDBserver, with ";thread:NNN" in T stop replies disabled,
+#          emulating old gdbservers when debugging single-threaded programs.
+#
+# T: Start GDBserver with the entire 'T' stop reply packet disabled,
+#    GDBserver will instead send the 'S' stop reply.
+foreach_with_prefix to_disable { Tthread T } {
+    run_test $to_disable
+}