[binutils-gdb] gdb: handle_no_resumed: only update thread list of event target

[binutils-gdb] gdb: handle_no_resumed: only update thread list of event target

[Simon Marchi]

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

commit 1e864019e430f2cac5ab6b822c4e1db39bd62699
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Mon Apr 11 22:11:03 2022 -0400

    gdb: handle_no_resumed: only update thread list of event target
    
    When running:
    
        $ make check TESTS="gdb.multi/multi-re-run.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
    
    I get:
    
        target remote localhost:2347^M
        Remote debugging using localhost:2347^M
        Reading symbols from /lib64/ld-linux-x86-64.so.2...^M
        Reading symbols from /usr/lib/debug//lib/x86_64-linux-gnu/ld-2.31.so...^M
        0x00007ffff7fd0100 in _start () from /lib64/ld-linux-x86-64.so.2^M
        (gdb) continue^M
        Continuing.^M
        Cannot execute this command while the target is running.^M
        Use the "interrupt" command to stop the target^M
        and then try again.^M
        (gdb) FAIL: gdb.multi/multi-re-run.exp: re_run_inf=1: iter=1: runto: run to all_started
    
    The test does:
    
     - Connect to a remote target with inferior 2, continue and stop on the
       all_started function
     - Connect to a separate remote target / GDBserver instance with inferior 1,
       continue and (expect to) stop on the all_started function
    
    The failure seen above happens when trying to continue inferior 1.
    
    What happens is:
    
     - GDB tells inferior 1's remote target to continue
     - We go into fetch_inferior_event, try to get an event at random from
       the targets
     - do_target_wait happens to pick inferior 2's target
     - That target return TARGET_WAITKIND_NO_RESUMED, which makes sense:
       inferior 2 is stopped, that target has no resumed thread
     - handle_no_resumed tries to update the thread list of all targets:
    
       for (auto *target : all_non_exited_process_targets ())
         {
           switch_to_target_no_thread (target);
           update_thread_list ();
         }
    
     - When trying to update the thread list of inferior 1's target, it hits
       the "Cannot execute this command while the target is running" error.
       This target is working in "remote all-stop" mode, and it is currently
       waiting for a stop reply, so it can't send packets to update the
       thread list at this time.
    
    To handle the problem described in the comment in handle_no_resumed, I
    don't think it is necessary to update the thread list of all targets,
    but only the event target.  That comment describes a kind of race
    condition where some target reports a breakpoint hit for a thread and
    then its last remaining resumed thread exits, so sends a "no resumed"
    event.  If we ended up resuming the thread that hit a breakpoint, we
    want to ignore the "no resumed" and carry on.
    
    But I don't really see why we need to update the thread list on the
    other targets.  I can't really articulate this, it's more a gut feeling,
    maybe I just fail to imagine the situation where this is needed.  But
    here is the patch anyway, so we can discuss it.  The patch changes
    handle_no_resumed to only update the thread list of the event target.
    This fixes the test run shown above.
    
    The way I originally tried to fix this was to make
    remote_target::update_thread_list return early if the target is
    currently awaiting a stop reply, since there's no way it can update the
    thread list at that time.  But that felt more like papering over the
    problem.  I then thought that we probably shouldn't be asking the target
    to update the thread list unnecessarily.
    
    Change-Id: Ide3df22b4f556478e155ad1c67ad4b4fe7c26a58

Diff:
---
 gdb/infrun.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 15051403e3c..4e7ca803c79 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5240,12 +5240,7 @@ handle_no_resumed (struct execution_control_state *ecs)
   inferior *curr_inf = current_inferior ();
 
   scoped_restore_current_thread restore_thread;
-
-  for (auto *target : all_non_exited_process_targets ())
-    {
-      switch_to_target_no_thread (target);
-      update_thread_list ();
-    }
+  update_thread_list ();
 
   /* If: