gdb/remote: Restore support for 'S' stop reply packet

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
targets (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 target.

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 target 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 target.

The problem is that after adding the new threads we don't reset
inferior_ptid, and the code path we use to wait for a stop event from
the target also doesn't reset inferior_ptid, so it 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 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 all 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_event 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 target and use that.  This will be fine for single threaded
 targets.  A multi-thread or multi-inferior aware remote target
 should be using T packets with a thread-id, so we give a warning if
 the target 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.

1 files changed, 6 insertions, 2 deletions

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d9a6f73..0f2b9a5 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 the target of 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 none 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);