Re: [PATCH v2] gdb/infrun: do not restart a stepped thread if not running (was: gdb/infrun: reset thread control's step info in end_stepping_range)

[Bruno Larsen <blarsen@redhat.com>]

On 20/06/2023 17:24, Tankut Baris Aktemur via Gdb-patches wrote:
> ======
> Changes in V2:
>
> * Changed the thread state check to `(tp->state != THREAD_RUNNING)`.
> * Rebased on the current master.
>
> The latest discussion was
> https://sourceware.org/pipermail/gdb-patches/2023-May/199613.html
>
> ======
>
> Suppose we have two inferiors on an all-stop target with schedule-multi
> set on:
>
>    $ gdb -q
>    (gdb) target extended-remote | gdbserver --multi -
>    Remote debugging using | gdbserver --multi -
>    Remote debugging using stdio
>    (gdb) file /temp/test
>    Reading symbols from /temp/test...
>    (gdb) set remote exec-file /temp/test
>    (gdb) start
>    Temporary breakpoint 1 at 0x115c: file test.c, line 8.
>    Starting program: /temp/test
>    stdin/stdout redirected
>    Process /temp/test created; pid = 864027
>    ...
>
>    Temporary breakpoint 1, main (argc=1, argv=0x7fffffffd218) at test.c:8
>    8         foo();
>    (gdb) add-inferior
>    [New inferior 2]
>    Added inferior 2 on connection 1 (extended-remote | gdbserver --multi -)
>    (gdb) inferior 2
>    [Switching to inferior 2 [<null>] (<noexec>)]
>    (gdb) file /temp/test
>    Reading symbols from /temp/test...
>    (gdb) set remote exec-file /temp/test
>    (gdb) tbreak 2
>    Temporary breakpoint 2 at 0x555555555131: /temp/test.c:2. (2 locations)
>    (gdb) run
>    Starting program: /temp/test
>    stdin/stdout redirected
>    Process /temp/test created; pid = 864430
>    ...
>
>    Thread 2.1 "test" hit Temporary breakpoint 2, foo () at test.c:2
>    2         int a = 42;
>    (gdb) set schedule-multi on
>    (gdb)
>
> At this point, detaching the first inferior works fine:
>
>    (gdb) detach inferiors 1
>    Detaching from program: /temp/test, process 858904
>    Detaching from process 858904
>    [Inferior 1 (process 858904) detached]
>    (gdb) info inferiors
>      Num  Description       Connection                                Executable
>      1    <null>            1 (extended-remote | gdbserver --multi -) /temp/test
>    * 2    process 858925    1 (extended-remote | gdbserver --multi -) /temp/test
>    (gdb)
>
> Let us now repeat exactly the same scenario, but before detaching, we
> make the current thread single-step an instruction:
>
>    ...
>    Thread 2.1 "test" hit Temporary breakpoint 2, foo () at test.c:2
>    2         int a = 42;
>    (gdb) stepi
>    3         int b = 43;
>    (gdb) detach inferiors 1
>    Detaching from program: /temp/test, process 876580
>    Detaching from process 876580
>    gdbserver: Couldn't reap LWP 876580 while detaching: No child processes
>    [Inferior 1 (process 876580) detached]
>    (gdb) 3   int b = 43;
>
> There is a mysterious line info output.  Running the scenario with
> infrun debug logs reveals more information.
>
>    ...
>    Thread 2.1 "test" hit Temporary breakpoint 2, foo () at test.c:2
>    2         int a = 42;
>    (gdb) stepi
>    3         int b = 43;
>    (gdb) set debug infrun on
>    (gdb) detach inferiors 1
>    [infrun] scoped_disable_commit_resumed: reason=detaching
>    Detaching from program: /temp/test, process 872445
>    Detaching from process 872445
>    gdbserver: Couldn't reap LWP 872445 while detaching: No child processes
>    [Inferior 1 (process 872445) detached]
>    [infrun] start_step_over: enter
>      [infrun] start_step_over: stealing global queue of threads to step, length = 0
>      [infrun] operator(): step-over queue now empty
>    [infrun] start_step_over: exit
>    [infrun] restart_stepped_thread: switching back to stepped thread (stepping)
>    [infrun] keep_going_stepped_thread: resuming previously stepped thread
>    [infrun] keep_going_stepped_thread: expected thread advanced also (0x555555555131 -> 0x555555555138)
>    [infrun] clear_step_over_info: clearing step over info
>    [infrun] do_target_resume: resume_ptid=-1.0.0, step=0, sig=GDB_SIGNAL_0
>    [infrun] infrun_async: enable=1
>    [infrun] reset: reason=detaching
>    [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target extended-remote
>    [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target extended-remote
>    (gdb) [infrun] fetch_inferior_event: enter
>      [infrun] scoped_disable_commit_resumed: reason=handling event
>      [infrun] do_target_wait: Found 2 inferiors, starting at #0
>      [infrun] random_pending_event_thread: None found.
>      [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
>      [infrun] print_target_wait_results:   872464.872464.0 [Thread 872464.872464],
>      [infrun] print_target_wait_results:   status->kind = STOPPED, sig = GDB_SIGNAL_TRAP
>      [infrun] handle_inferior_event: status->kind = STOPPED, sig = GDB_SIGNAL_TRAP
>      [infrun] context_switch: Switching context from 0.0.0 to 872464.872464.0
>      [infrun] handle_signal_stop: stop_pc=0x555555555138
>      [infrun] handle_signal_stop: [872464.872464.0] hit its single-step breakpoint
>      [infrun] handle_signal_stop: delayed software breakpoint trap, ignoring
>      [infrun] process_event_stop_test: stepi/nexti
>      [infrun] stop_waiting: stop_waiting
>    3         int b = 43;
>      [infrun] infrun_async: enable=0
>      [infrun] reset: reason=handling event
>      [infrun] maybe_set_commit_resumed_all_targets: not requesting commit-resumed for target extended-remote, no resumed threads
>    [infrun] fetch_inferior_event: exit
>
> GDB attempted to do a step-over for the current thread.  This takes us
> to the commit that introduced restarting step-overs:
>
>    commit 408f66864a1a823591b26420410c982174c239a2
>    Author: Pedro Alves <pedro@palves.net>
>    Date:   Mon Jan 11 20:01:58 2021 +0000
>
>      detach in all-stop with threads running
>
>      A following patch will add a testcase that has a number of threads
>      constantly stepping over a breakpoint, and then has GDB detach the
>      process, while threads are running.  If we have more than one inferior
>      running, and we detach from just one of the inferiors, we expect that
>      the remaining inferior continues running.  However, in all-stop, if
>      GDB needs to pause the target for the detach, nothing is re-resuming
>      the other inferiors after the detach.  "info threads" shows the
>      threads as running, but they really aren't.  This fixes it.
>
> However, the thread that was resumed for step-over in our scenario did
> not have an interrupted step-over; it had completed its stepi already.
> More debugging reveals that the thread is resumed because of the
> following two conditions in `restart_stepped_thread`:
>
>        if (tp->control.trap_expected)
>          {
>            infrun_debug_printf ("switching back to stepped thread (step-over)");
>
>            if (keep_going_stepped_thread (tp))
>              return true;
>          }
>
> and
>
>        if (tp->control.step_range_end)
>          {
>            infrun_debug_printf ("switching back to stepped thread (stepping)");
>
>            if (keep_going_stepped_thread (tp))
>              return true;
>          }
>
> The root cause of the problem is, `restart_stepped_thread` checks for
> the thread state as
>
>      if (tp->state == THREAD_EXITED)
>        continue;
>
> but the thread's state is THREAD_STOPPED.  To fix, we change the state
> check to
>
>      if (tp->state != THREAD_RUNNING)
>
> Additionally, the 'trap_expected' and the 'step_range_end' fields of
> the thread's control remain set even after the "stepi" command
> completes, creating a half-baked internal state that can be misleading
> when debugging.  We address this problem by clearing the control
> fields when stepping completes.  We also add a regression test.
>
> Regression-tested on X86_64 Linux using the default, native-gdbserver,
> and native-extended-gdbserver board files.

Hi Tankut!

Thanks for working on this. I don't see any regressions, but the test 
you added doesn't seem to fail before your change. Does it not work with 
the default options for the testsuite?

-- 
Cheers,
Bruno

> ---
>   gdb/infrun.c                             |  7 ++-
>   gdb/testsuite/gdb.multi/detach-stepi.c   | 30 +++++++++++
>   gdb/testsuite/gdb.multi/detach-stepi.exp | 66 ++++++++++++++++++++++++
>   3 files changed, 101 insertions(+), 2 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.multi/detach-stepi.c
>   create mode 100644 gdb/testsuite/gdb.multi/detach-stepi.exp
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..f40d924dc7a 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7776,7 +7776,7 @@ restart_stepped_thread (process_stratum_target *resume_target,
>   
>     for (thread_info *tp : all_threads_safe ())
>       {
> -      if (tp->state == THREAD_EXITED)
> +      if (tp->state != THREAD_RUNNING)
>   	continue;
>   
>         if (tp->has_pending_waitstatus ())
> @@ -7800,7 +7800,7 @@ restart_stepped_thread (process_stratum_target *resume_target,
>   
>     for (thread_info *tp : all_threads_safe ())
>       {
> -      if (tp->state == THREAD_EXITED)
> +      if (tp->state != THREAD_RUNNING)
>   	continue;
>   
>         if (tp->has_pending_waitstatus ())
> @@ -8523,6 +8523,9 @@ static void
>   end_stepping_range (struct execution_control_state *ecs)
>   {
>     ecs->event_thread->control.stop_step = 1;
> +  ecs->event_thread->control.trap_expected = 0;
> +  ecs->event_thread->control.step_range_start = 0;
> +  ecs->event_thread->control.step_range_end = 0;
>     stop_waiting (ecs);
>   }
>   
> diff --git a/gdb/testsuite/gdb.multi/detach-stepi.c b/gdb/testsuite/gdb.multi/detach-stepi.c
> new file mode 100644
> index 00000000000..d365645fb3f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/detach-stepi.c
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +void
> +a_function ()
> +{
> +  int a = 42;
> +}
> +
> +int
> +main ()
> +{
> +  int b = 43;
> +  a_function ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/detach-stepi.exp b/gdb/testsuite/gdb.multi/detach-stepi.exp
> new file mode 100644
> index 00000000000..28ef8c4f9f7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/detach-stepi.exp
> @@ -0,0 +1,66 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test detaching from an inferior after a thread in another inferior
> +# completes a stepi.  This is a regression test for a bug that was
> +# causing an inadvertent resume of the just-stepped thread.
> +
> +standard_testfile
> +
> +if {[use_gdb_stub]} {
> +    untested "using gdb stub"
> +    return 0
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    return -1
> +}
> +
> +delete_breakpoints
> +
> +# Setup inferior 2.
> +gdb_test "add-inferior" "Added inferior .*" \
> +    "add empty inferior"
> +gdb_test "inferior 2" "Switching to inferior .*" \
> +    "switch to inferior"
> +
> +gdb_load $binfile
> +runto "a_function"
> +gdb_test "info inferiors"
> +
> +# The bug for which this regression test is written appears in
> +# schedule-multi mode.
> +gdb_test_no_output "set schedule-multiple on"
> +
> +# Single-step the thread in Inferior 2, then detach Inferior 1.
> +gdb_test "info threads" ".*" "threads before stepi"
> +gdb_test "stepi"
> +gdb_test "info threads" ".*" "threads after stepi"
> +
> +gdb_test "set debug infrun on"
> +gdb_test_multiple "detach inferior 1" "" {
> +    -re "resuming previously stepped thread.*$gdb_prompt" {
> +	fail $gdb_test_name
> +    }
> +    -re "$gdb_prompt $" {
> +	pass $gdb_test_name
> +    }
> +}