[binutils-gdb] gdb: prune inferiors at end of fetch_inferior_event, fix intermittent failure of gdb.threads/fork-pl

[binutils-gdb] gdb: prune inferiors at end of fetch_inferior_event, fix intermittent failure of gdb.threads/fork-pl

[Simon Marchi]

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

commit 152a17495663ae9099d5efdc38322c9ad348014e
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Fri Apr 22 13:40:57 2022 -0400

    gdb: prune inferiors at end of fetch_inferior_event, fix intermittent failure of gdb.threads/fork-plus-threads.exp
    
    This test sometimes fail like this:
    
        info threads^M
          Id   Target Id         Frame ^M
          11.12 process 2270719   Couldn't get registers: No such process.^M
        (gdb) FAIL: gdb.threads/fork-plus-threads.exp: detach-on-fork=off: no threads left
        [Inferior 11 (process 2270719) exited normally]^M
        info inferiors^M
          Num  Description       Connection           Executable        ^M
        * 1    <null>                                 /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/fork-plus-threads/fork-plus-threads ^M
          11   <null>                                 /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/fork-plus-threads/fork-plus-threads ^M
        (gdb) FAIL: gdb.threads/fork-plus-threads.exp: detach-on-fork=off: only inferior 1 left (the program exited)
    
    I can get it to fail quite reliably by pinning it to a core:
    
      $ taskset -c 5 make check TESTS="gdb.threads/fork-plus-threads.exp"
    
    The previous attempt at fixing this was:
    
      https://sourceware.org/pipermail/gdb-patches/2021-October/182846.html
    
    What we see is part due to a possible unfortunate ordering of events
    given by the kernel, and what could be considered a bug in GDB.
    
    The test program makes a number of forks, waits them all, then exits.
    Most of the time, GDB will get and process the exit event for inferior 1
    after the exit events of all the children.  But this is not guaranteed.
    After the last child exits and is waited by the parent, the parent can
    exit quickly, such that GDB collects from the kernel the exit events for
    the parent and that child at the same time.  It then chooses one event
    at random, which can be the event for the parent.  This will result in
    the parent appearing to exit before its child.  There's not much we can
    do about it, so I think we have to adjust the test to cope.
    
    After expect has seen the "exited normally" notification for inferior 1,
    it immediately does an "info thread" that it expects to come back empty.
    But at this point, GDB might not have processed inferior 11's (the last
    child) exit event, so it will look like there is still a thread.  Of
    course that thread is dead, we just don't know it yet.  But that makes
    the "no thread" test fail.  If the test waited just a bit more for the
    "exited normally" notification for inferior 11, then the list of threads
    would be empty.
    
    So, first change, make the test collect all the "exited normally"
    notifications for all inferiors before proceeding, that should ensure we
    see an empty thread list.  That would fix the first FAIL above.
    
    However, we would still have the second FAIL, as we expect inferior 11
    to not be there, it should have been deleted automatically.  Inferior 11
    is normally deleted when prune_inferiors is called.  That is called by
    normal_stop, which is only called by fetch_inferior_event only if the
    event thread completed an execution command FSM (thread_fsm).  But the
    FSM for the continue command completed when inferior 1 exited.  At that
    point inferior 11 was not prunable, as it still had a thread.  When
    inferior 11 exits, prune_inferiors is not called.
    
    I think that can be considered a GDB bug.  From the user point of view,
    there's no reason why in one case inferior 11 would be deleted and not
    in the other case.
    
    This patch makes the somewhat naive change to call prune_inferiors in
    fetch_inferior_event, so that it is called in this case.  It is placed
    at this particular point in the function so that it is called after the
    user inferior / thread selection is restored.  If it was called before
    that, inferior 11 wouldn't be pruned, because it would still be the
    current inferior.
    
    Change-Id: I48a15d118f30b1c72c528a9f805ed4974170484a
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26272

Diff:
---
 gdb/infrun.c                                    | 17 +++++++++----
 gdb/testsuite/gdb.threads/fork-plus-threads.exp | 33 +++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c311240b78c..15051403e3c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4278,6 +4278,18 @@ fetch_inferior_event ()
        reinstalled here.  */
   }
 
+  /* Handling this event might have caused some inferiors to become prunable.
+     For example, the exit of an inferior that was automatically added.  Try
+     to get rid of them.  Keeping those around slows down things linearly.
+
+     Note that this never removes the current inferior.  Therefore, call this
+     after RESTORE_THREAD went out of scope, in case the event inferior (which was
+     temporarily made the current inferior) is meant to be deleted.
+
+     Call this before all_uis_check_sync_execution_done, so that notifications about
+     removed inferiors appear before the prompt.  */
+  prune_inferiors ();
+
   /* If a UI was in sync execution mode, and now isn't, restore its
      prompt (a synchronous execution command has finished, and we're
      ready for input).  */
@@ -8769,11 +8781,6 @@ normal_stop (void)
 	breakpoint_auto_delete (inferior_thread ()->control.stop_bpstat);
     }
 
-  /* Try to get rid of automatically added inferiors that are no
-     longer needed.  Keeping those around slows down things linearly.
-     Note that this never removes the current inferior.  */
-  prune_inferiors ();
-
   return 0;
 }
 \f
diff --git a/gdb/testsuite/gdb.threads/fork-plus-threads.exp b/gdb/testsuite/gdb.threads/fork-plus-threads.exp
index bc09c89e69b..26fbef72941 100644
--- a/gdb/testsuite/gdb.threads/fork-plus-threads.exp
+++ b/gdb/testsuite/gdb.threads/fork-plus-threads.exp
@@ -76,6 +76,16 @@ proc do_test { detach-on-fork } {
     set saw_cannot_remove_breakpoints 0
     set saw_thread_stopped 0
 
+    set expected_num_inferior_exits [expr ${detach-on-fork} == "off" ? 11 : 1]
+
+    # Flags indicating if we have see the exit for each inferior.
+    for {set i 1} {$i <= $expected_num_inferior_exits} {incr i} {
+	set inferior_exits_seen($i) 0
+    }
+
+    # Number of inferior exits seen so far.
+    set num_inferior_exits_seen 0
+
     set test "inferior 1 exited"
     gdb_test_multiple "" $test {
 	-re "Cannot remove breakpoints" {
@@ -94,11 +104,30 @@ proc do_test { detach-on-fork } {
 	    # Avoid timeout with check-read1
 	    exp_continue
 	}
-	-re "Inferior 1 \(\[^\r\n\]+\) exited normally" {
-	    pass $test
+	-re "Inferior ($::decimal) \(\[^\r\n\]+\) exited normally" {
+	    set infnum $expect_out(1,string)
+	    incr num_inferior_exits_seen
+	    incr inferior_exits_seen($infnum) 1
+
+	    if { $num_inferior_exits_seen == $expected_num_inferior_exits } {
+		pass $test
+	    } else {
+		exp_continue
+	    }
 	}
     }
 
+    # Verify that we got all the inferior exits we expected.
+    set num_ok_exits 0
+    for {set i 1} {$i <= $expected_num_inferior_exits} {incr i} {
+	if { $inferior_exits_seen($i) == 1 } {
+	    incr num_ok_exits
+	}
+    }
+
+    gdb_assert { $num_ok_exits == $expected_num_inferior_exits } \
+	"seen all expected inferior exits"
+
     gdb_assert !$saw_cannot_remove_breakpoints \
 	"no failure to remove breakpoints"
     gdb_assert !$saw_thread_stopped \