[binutils-gdb] gdb/remote: Restore support for 'S' stop reply packet

[binutils-gdb] gdb/remote: Restore support for 'S' stop reply packet

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=24ed6739b699f329c2c45aedee5f8c7d2f54e493

commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu Jan 30 14:35:40 2020 +0000

    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.

Diff:
---
 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/ChangeLog b/gdb/ChangeLog
index 591ef40..47481bd 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2020-03-02  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* 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.
+
 2020-02-29  Jon Turney  <jon.turney@dronecode.org.uk>
 
 	* debuginfod-support.c: Include defs.h first.
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);
diff --git a/gdb/remote.c b/gdb/remote.c
index 4a70ab3..9b73faf 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/ChangeLog b/gdb/testsuite/ChangeLog
index e5ea91d..488a328 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-03-02  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.server/stop-reply-no-thread.exp: Add test where T packet is
+	disabled.
+
 2020-03-02  Pedro Alves  <palves@redhat.com>
 	      Tom de Vries  <tdevries@suse.de>
 
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
index 45407bc..ffc1c27 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
+}