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

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

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

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
---
 gdb/infrun.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c311240b78cd..5aaa5491b16e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5228,12 +5228,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:
 

base-commit: 50192212a72b48de7ae4d87c79d394f4e3461a5b
-- 
2.35.1

Re: [PATCH] gdb: handle_no_resumed: only update thread list of event target

[Simon Marchi]

Ping.

On 2022-04-11 22:11, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> 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
> ---
>  gdb/infrun.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c311240b78cd..5aaa5491b16e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5228,12 +5228,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:
>  
> 
> base-commit: 50192212a72b48de7ae4d87c79d394f4e3461a5b
> -- 
> 2.35.1
> 

Re: [PATCH] gdb: handle_no_resumed: only update thread list of event target

[Pedro Alves]

On 2022-04-12 03:11, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> 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.

I think you are right.

> 
> 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.

Agreed.

Re: [PATCH] gdb: handle_no_resumed: only update thread list of event target

[Simon Marchi]

On 2022-04-22 09:30, Pedro Alves wrote:
>
> On 2022-04-12 03:11, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> 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.
>
> I think you are right.

Ok, will push.  Let's hope we are right :).

Simon