[binutils-gdb] Make follow_fork not rely on get_last_target_status

[binutils-gdb] Make follow_fork not rely on get_last_target_status

[Pedro Alves]

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

commit 3505d4c4f7e2156837c7bfcbecf0a03b202e7ffb
Author: Pedro Alves <pedro@palves.net>
Date:   Thu Nov 17 18:25:36 2022 +0000

    Make follow_fork not rely on get_last_target_status
    
    Currently, if
    
      - you're in all-stop mode,
      - the inferior last stopped because of a fork catchpoint,
    
    when you next resume the program, gdb checks whether it had last
    stopped for a fork/vfork, and if so,
    
     a) if the current thread is the one that forked, gdb follows the
       parent/child, depending on "set follow-fork" mode.
    
     b) if the current thread is some other thread (because you switched
       threads meanwhile), gdb switches back to that thread, gdb follows
       the parent/child, and stops the resumption command.
    
    There's a problem in b), however -- if you have "set schedule-multiple
    off", which is the default, or "set scheduler-locking on", gdb will
    still switch back to the forking thread, even if you didn't want to
    resume it.  For example, with:
    
      (gdb) catch fork
      (gdb) c
      * thread 1 stops for fork
      (gdb) thread 2
      (gdb) set scheduler-locking on
      (gdb) c
    
    gdb switches back to thread 1, and follows the fork.
    
    Or with:
    
      (gdb) add-inferior -exec prog
      (gdb) inferior 2
      (gdb) start
      (gdb) inferior 1
      (gdb) catch fork
      (gdb) c
      * thread 1.1 stops for fork
      (gdb) inferior 2
      (gdb) set schedule-multiple off # this is the default
      (gdb) c
    
    gdb switches back to thread 1.1, and follows the fork.
    
    Another issue is that, because follow_fork relies on
    get_last_target_status to find the thread that has a pending fork, it
    is possible to confuse it.  For example, "run" or "start" call
    init_wait_for_inferior, which clears the last target status, so this:
    
      (gdb) catch fork
      (gdb) c
      * thread 1 stops for fork
      (gdb) add-inferior -exec prog
      (gdb) inferior 2
      (gdb) start
      (gdb) set follow-fork child
      (gdb) inferior 1
      (gdb) n
    
    ... does not follow to the fork child of inferior 1, because the
    get_last_target_status call in follow_fork doesn't return a
    TARGET_WAITKIND_FORKED.  Thanks to Simon for this example.
    
    All of the above are fixed by this patch.  It changes follow_fork to
    not look at get_last_target_status, but to instead iterate over the
    set of threads that the user is resuming, and find the one that has a
    pending_follow kind of fork/vfork.
    
    gdb.base/foll-fork.exp is augmented to exercise the last "start"
    scenario described above.  The other cases will be exercised in the
    testcase added by the following patch.
    
    Change-Id: Ifcca77e7b2456277387f40660ef06cec2b93b97e

Diff:
---
 gdb/infrun.c                         | 67 ++++++++++++++++++++----------------
 gdb/testsuite/gdb.base/foll-fork.exp | 36 +++++++++++++++++--
 2 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 05c824d2141..04d76895f5e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -706,6 +706,15 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
   return false;
 }
 
+/* Set the last target status as TP having stopped.  */
+
+static void
+set_last_target_status_stopped (thread_info *tp)
+{
+  set_last_target_status (tp->inf->process_target (), tp->ptid,
+			  target_waitstatus {}.set_stopped (GDB_SIGNAL_0));
+}
+
 /* Tell the target to follow the fork we're stopped at.  Returns true
    if the inferior should be resumed; false, if the target for some
    reason decided it's best not to resume.  */
@@ -730,32 +739,33 @@ follow_fork ()
 
   if (!non_stop)
     {
-      process_stratum_target *wait_target;
-      ptid_t wait_ptid;
-      struct target_waitstatus wait_status;
+      thread_info *cur_thr = inferior_thread ();
 
-      /* Get the last target status returned by target_wait().  */
-      get_last_target_status (&wait_target, &wait_ptid, &wait_status);
-
-      /* If not stopped at a fork event, then there's nothing else to
-	 do.  */
-      if (wait_status.kind () != TARGET_WAITKIND_FORKED
-	  && wait_status.kind () != TARGET_WAITKIND_VFORKED)
-	return 1;
+      ptid_t resume_ptid
+	= user_visible_resume_ptid (cur_thr->control.stepping_command);
+      process_stratum_target *resume_target
+	= user_visible_resume_target (resume_ptid);
 
-      /* Check if we switched over from WAIT_PTID, since the event was
-	 reported.  */
-      if (wait_ptid != minus_one_ptid
-	  && (current_inferior ()->process_target () != wait_target
-	      || inferior_ptid != wait_ptid))
+      /* Check if there's a thread that we're about to resume, other
+	 than the current, with an unfollowed fork/vfork.  If so,
+	 switch back to it, to tell the target to follow it (in either
+	 direction).  We'll afterwards refuse to resume, and inform
+	 the user what happened.  */
+      for (thread_info *tp : all_non_exited_threads (resume_target,
+						     resume_ptid))
 	{
-	  /* We did.  Switch back to WAIT_PTID thread, to tell the
-	     target to follow it (in either direction).  We'll
-	     afterwards refuse to resume, and inform the user what
-	     happened.  */
-	  thread_info *wait_thread = find_thread_ptid (wait_target, wait_ptid);
-	  switch_to_thread (wait_thread);
-	  should_resume = false;
+	  if (tp == cur_thr)
+	    continue;
+
+	  if (tp->pending_follow.kind () != TARGET_WAITKIND_SPURIOUS)
+	    {
+	      infrun_debug_printf ("need to follow-fork [%s] first",
+				   tp->ptid.to_string ().c_str ());
+
+	      switch_to_thread (tp);
+	      should_resume = false;
+	      break;
+	    }
 	}
     }
 
@@ -819,21 +829,16 @@ follow_fork ()
 	  }
 	else
 	  {
-	    /* This makes sure we don't try to apply the "Switched
-	       over from WAIT_PID" logic above.  */
-	    nullify_last_target_wait_ptid ();
-
 	    /* If we followed the child, switch to it...  */
 	    if (follow_child)
 	      {
-		thread_info *child_thr = find_thread_ptid (parent_targ, child);
-		switch_to_thread (child_thr);
+		tp = find_thread_ptid (parent_targ, child);
+		switch_to_thread (tp);
 
 		/* ... and preserve the stepping state, in case the
 		   user was stepping over the fork call.  */
 		if (should_resume)
 		  {
-		    tp = inferior_thread ();
 		    tp->control.step_resume_breakpoint
 		      = step_resume_breakpoint;
 		    tp->control.step_range_start = step_range_start;
@@ -872,6 +877,8 @@ follow_fork ()
       break;
     }
 
+  if (!should_resume)
+    set_last_target_status_stopped (tp);
   return should_resume;
 }
 
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp b/gdb/testsuite/gdb.base/foll-fork.exp
index 51dfcce7ff5..df5dab056c2 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -209,15 +209,22 @@ set reading_in_symbols_re {(?:\r\nReading in symbols for [^\r\n]*)?}
 # Test the ability to catch a fork, specify that the child be
 # followed, and continue.  Make the catchpoint permanent.
 
-proc_with_prefix catch_fork_child_follow {} {
+proc_with_prefix catch_fork_child_follow {second_inferior} {
     global gdb_prompt
     global srcfile
     global reading_in_symbols_re
 
+    if { $second_inferior && [use_gdb_stub] } {
+	return
+    }
+
     if { ![setup] } {
 	return
     }
 
+    # Get rid of the breakpoint at "main".
+    delete_breakpoints
+
     set bp_after_fork [gdb_get_line_number "set breakpoint here"]
 
     gdb_test "catch fork" \
@@ -246,6 +253,29 @@ proc_with_prefix catch_fork_child_follow {} {
 	"Temporary breakpoint.*, line $bp_after_fork.*" \
 	"set follow-fork child, tbreak"
 
+    if {$second_inferior} {
+	gdb_test "add-inferior" "Added inferior 2.*" "add inferior 2"
+
+	gdb_test "inferior 2" "Switching to inferior 2.*"
+
+	gdb_load $::binfile
+
+	# Start it.  This should not affect inferior 1, given "set
+	# schedule-multiple off" (default).  GDB used to have a bug
+	# where "start" would clear the pending follow fork
+	# information of inferior 1.
+	gdb_test "start" "Starting program.*Temporary breakpoint .*"
+
+	gdb_test "inferior 1" "Switching to inferior 1.*"
+
+	# Verify that the catchpoint is still mentioned in an "info
+	# breakpoints", and further that the catchpoint still shows
+	# the captured process id.
+	gdb_test "info breakpoints" \
+	    ".*catchpoint.*keep y.*fork, process.*" \
+	    "info breakpoints, after starting second inferior"
+    }
+
     set expected_re "\\\[Attaching after.* fork to.*\\\[Detaching after fork from"
     append expected_re ".* at .*$bp_after_fork.*"
     gdb_test "continue" $expected_re "set follow-fork child, hit tbreak"
@@ -428,6 +458,8 @@ foreach_with_prefix follow-fork-mode {"parent" "child"} {
 
 # Catchpoint tests.
 
-catch_fork_child_follow
+foreach_with_prefix second_inferior {false true} {
+    catch_fork_child_follow $second_inferior
+}
 catch_fork_unpatch_child
 tcatch_fork_parent_follow