[binutils-gdb] Convert previous_inferior_ptid to strong reference to thread_info

[binutils-gdb] Convert previous_inferior_ptid to strong reference to thread_info

[Pedro Alves]

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

commit a81871f7136c0c0ae6e9d921cb0717829543ee61
Author: Pedro Alves <pedro@palves.net>
Date:   Thu Nov 17 23:08:58 2022 +0000

    Convert previous_inferior_ptid to strong reference to thread_info
    
    I originally wrote this patch, because while working on some other
    patch, I spotted a regression in the
    gdb.multi/multi-target-no-resumed.exp.exp testcase.  Debugging the
    issue, I realized that the problem was related to how I was using
    previous_inferior_ptid to look up the thread the user had last
    selected.  The problem is that previous_inferior_ptid alone doesn't
    tell you which target that ptid is from, and I was just always using
    the current target, which was incorrect.  Two different targets may
    have threads with the same ptid.
    
    I decided to fix this by replacing previous_inferior_ptid with a
    strong reference to the thread, called previous_thread.
    
    I have since found a new motivation for this change -- I would like to
    tweak "info program" to not rely on get_last_target_status returning a
    ptid that still exists in the thread list.  With both the follow_fork
    changes later in this series, and the step-over-thread-exit changes,
    that can happen, as we'll delete threads and not clear the last
    waitstatus.
    
    A new update_previous_thread function is added that can be used to
    update previous_thread from inferior_ptid.  This must be called in
    several places that really want to get rid of previous_thread thread,
    and reset the thread id counter, otherwise we get regressions like
    these:
    
      (gdb) info threads -gid
        Id   GId  Target Id                               Frame
     - * 1    1    Thread 2974541.2974541 "tids-gid-reset" main () at src/gdb/testsuite/gdb.multi/tids-gid-reset.c:21
     - (gdb) PASS: gdb.multi/tids-gid-reset.exp: single-inferior: after restart: info threads -gid
     + * 1    2    Thread 2958361.2958361 "tids-gid-reset" main () at src/gdb/testsuite/gdb.multi/tids-gid-reset.c:21
     + (gdb) FAIL: gdb.multi/tids-gid-reset.exp: single-inferior: after restart: info threads -gid
    
    and:
    
      Core was generated by `build/gdb/testsuite/outputs/gdb.reverse/sigall-precsave/si'.
      Program terminated with signal SIGTRAP, Trace/breakpoint trap.
      #0  gen_ABRT () at src/gdb/testsuite/gdb.reverse/sigall-reverse.c:398
      398      kill (getpid (), SIGABRT);
     +[Current thread is 1 (LWP 2662066)]
      Restored records from core file build/gdb/testsuite/outputs/gdb.reverse/sigall-precsave/sigall.precsave.
      #0  gen_ABRT () at src/gdb/testsuite/gdb.reverse/sigall-reverse.c:398
      398      kill (getpid (), SIGABRT);
    
      continue
      Continuing.
    
     -Program received signal SIGABRT, Aborted.
     +Thread 1 received signal SIGABRT, Aborted.
      0x00007ffff7dfd55b in kill () at ../sysdeps/unix/syscall-template.S:78
      78     ../sysdeps/unix/syscall-template.S: No such file or directory.
     -(gdb) PASS: gdb.reverse/sigall-precsave.exp: sig-test-1: get signal ABRT
     +(gdb) FAIL: gdb.reverse/sigall-precsave.exp: sig-test-1: get signal ABRT
    
    I.e., GDB was failing to restart the thread counter back to 1, because
    the previous_thread thread was being help due to the strong reference.
    
    Tested on GNU/Linux native, gdbserver and gdbserver + "maint set
    target-non-stop on".
    
    gdb/ChangeLog:
    yyyy-mm-dd  Pedro Alves  <pedro@palves.net>
    
            * infcmd.c (kill_command, detach_command, disconnect_command):
            Call update_previous_thread.
            * infrun.c (previous_inferior_ptid): Delete.
            (previous_thread): New.
            (update_previous_thread): New.
            (proceed, init_wait_for_inferior): Call update_previous_thread.
            (normal_stop): Adjust to compare previous_thread and
            inferior_thread.  Call update_previous_thread.
            * infrun.h (update_previous_thread): Declare.
            * target.c (target_pre_inferior, target_preopen): Call
            update_previous_thread.
    
    Change-Id: I42779a1ee51a996fa1e8f6e1525c6605dbfd42c7

Diff:
---
 gdb/infcmd.c |  5 +++++
 gdb/infrun.c | 43 ++++++++++++++++++++++++++++---------------
 gdb/infrun.h |  4 ++++
 gdb/target.c |  5 +++++
 4 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a851fe1f8c8..76453402c93 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2451,6 +2451,8 @@ kill_command (const char *arg, int from_tty)
   target_kill ();
   bfd_cache_close_all ();
 
+  update_previous_thread ();
+
   if (print_inferior_events)
     gdb_printf (_("[Inferior %d (%s) killed]\n"),
 		infnum, pid_str.c_str ());
@@ -2811,6 +2813,8 @@ detach_command (const char *args, int from_tty)
 
   target_detach (current_inferior (), from_tty);
 
+  update_previous_thread ();
+
   /* The current inferior process was just detached successfully.  Get
      rid of breakpoints that no longer make sense.  Note we don't do
      this within target_detach because that is also used when
@@ -2849,6 +2853,7 @@ disconnect_command (const char *args, int from_tty)
   target_disconnect (args, from_tty);
   no_shared_libraries (nullptr, from_tty);
   init_thread_list ();
+  update_previous_thread ();
   if (deprecated_detach_hook)
     deprecated_detach_hook ();
 }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3629db0443a..8983a1024eb 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -152,8 +152,18 @@ show_step_stop_if_no_debug (struct ui_file *file, int from_tty,
 /* proceed and normal_stop use this to notify the user when the
    inferior stopped in a different thread than it had been running
    in.  */
+static thread_info_ref previous_thread;
 
-static ptid_t previous_inferior_ptid;
+/* See infrun.h.  */
+
+void
+update_previous_thread ()
+{
+  if (inferior_ptid == null_ptid)
+    previous_thread = nullptr;
+  else
+    previous_thread = thread_info_ref::new_reference (inferior_thread ());
+}
 
 /* If set (default for legacy reasons), when following a fork, GDB
    will detach from one of the fork branches, child or parent.
@@ -3151,7 +3161,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
     }
 
   /* We'll update this if & when we switch to a new thread.  */
-  previous_inferior_ptid = inferior_ptid;
+  update_previous_thread ();
 
   regcache = get_current_regcache ();
   gdbarch = regcache->arch ();
@@ -3434,7 +3444,7 @@ init_wait_for_inferior (void)
 
   nullify_last_target_wait_ptid ();
 
-  previous_inferior_ptid = inferior_ptid;
+  update_previous_thread ();
 }
 
 \f
@@ -8704,21 +8714,24 @@ normal_stop ()
      the current thread back to the thread the user had selected right
      after this event is handled, so we're not really switching, only
      informing of a stop.  */
-  if (!non_stop
-      && previous_inferior_ptid != inferior_ptid
-      && target_has_execution ()
-      && last.kind () != TARGET_WAITKIND_SIGNALLED
-      && last.kind () != TARGET_WAITKIND_EXITED
-      && last.kind () != TARGET_WAITKIND_NO_RESUMED)
+  if (!non_stop)
     {
-      SWITCH_THRU_ALL_UIS ()
+      if ((last.kind () != TARGET_WAITKIND_SIGNALLED
+	   && last.kind () != TARGET_WAITKIND_EXITED
+	   && last.kind () != TARGET_WAITKIND_NO_RESUMED)
+	  && target_has_execution ()
+	  && previous_thread != inferior_thread ())
 	{
-	  target_terminal::ours_for_output ();
-	  gdb_printf (_("[Switching to %s]\n"),
-		      target_pid_to_str (inferior_ptid).c_str ());
-	  annotate_thread_changed ();
+	  SWITCH_THRU_ALL_UIS ()
+	    {
+	      target_terminal::ours_for_output ();
+	      gdb_printf (_("[Switching to %s]\n"),
+			  target_pid_to_str (inferior_ptid).c_str ());
+	      annotate_thread_changed ();
+	    }
 	}
-      previous_inferior_ptid = inferior_ptid;
+
+      update_previous_thread ();
     }
 
   if (last.kind () == TARGET_WAITKIND_NO_RESUMED)
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 43fd1b44f5a..f4f2216e092 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -117,6 +117,10 @@ enum exec_direction_kind
 /* The current execution direction.  */
 extern enum exec_direction_kind execution_direction;
 
+/* Call this to point 'previous_thread' at the thread returned by
+   inferior_thread, or at nullptr, if there's no selected thread.  */
+extern void update_previous_thread ();
+
 extern void start_remote (int from_tty);
 
 /* Clear out all variables saying what to do when inferior is
diff --git a/gdb/target.c b/gdb/target.c
index d0aa8f5cc6c..0cebecfafc3 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2469,6 +2469,8 @@ target_pre_inferior (int from_tty)
 
   current_inferior ()->highest_thread_num = 0;
 
+  update_previous_thread ();
+
   agent_capability_invalidate ();
 }
 
@@ -2497,6 +2499,9 @@ target_preopen (int from_tty)
 	error (_("Program not killed."));
     }
 
+  /* Release reference to old previous thread.  */
+  update_previous_thread ();
+
   /* Calling target_kill may remove the target from the stack.  But if
      it doesn't (which seems like a win for UDI), remove it now.  */
   /* Leave the exec target, though.  The user may be switching from a