Re: [PATCH v4 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history

[Guinevere Larsen <blarsen@redhat.com>]

On 3/12/24 08:34, Markus Metzger wrote:
> When trying to step over a breakpoint at the end of the trace, the
> step-over will fail with no-history.  This does not clear step_over_info
> so a subsequent resume will cause GDB to not resume the thread and expect
> a SIGTRAP to complete the step-over.  This will never come causing GDB to
> hang in the wait-for-event poll.
>
> That step-over failed after actually completing the step.  This is wrong.
> The step-over itself should have failed and the step should not have
> completed.  Fix it by moving the end of execution history check to before
> we are stepping.
>
> This exposes another issue, however.  When completing a step-over at the
> end of the execution history, we implicitly stop replaying that thread.  A
> continue command would resume after the step-over and, since we're no
> longer replaying, would continue recording.
>
> Fix that by recording the replay state in the thread's control state and
> failing with no-history in keep_going if we're switching from replay to
> recording.

I am a little confused by your test case. From the commit message 
description, I expected the following GDB session to have been problematic:

$ gdb record_goto
(gdb) start
49 fun4 ();
(gdb) record btrace
(gdb) break fun1
(gdb) continue
in frame fun1 ()
23 }
(gdb) reverse-next
In frame fun4 ()
41 fun1 ();
(gdb) next
No more reverse-execution history.
23 }
(gdb) next
# problems due to the SIGTRAP from the previous line

(Pardon my hand written gdb session, my new system has an AMD cpu, so I 
seem to be locked out of btrace).

My confusion comes from your test case never explicitly setting any 
breakpoints. I don't see why the inferior would have a sigtrap generated 
to test it. Am I misunderstanding the issue you described?


-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> ---
>   gdb/gdbthread.h                        |  3 ++
>   gdb/infrun.c                           | 25 +++++++++++++
>   gdb/record-btrace.c                    | 19 +++++-----
>   gdb/testsuite/gdb.btrace/cont-hang.exp | 43 ++++++++++++++++++++++
>   gdb/testsuite/gdb.btrace/step-hang.exp | 42 ++++++++++++++++++++++
>   gdb/testsuite/gdb.btrace/stepn.exp     | 50 ++++++++++++++++++++++++++
>   6 files changed, 173 insertions(+), 9 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>   create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>   create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
>
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 11c553a99ca..4931a0cc2f1 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -173,6 +173,9 @@ struct thread_control_state
>        command.  This is used to decide whether "set scheduler-locking
>        step" behaves like "on" or "off".  */
>     int stepping_command = 0;
> +
> +  /* Whether the thread was replaying when the command was issued.  */
> +  bool is_replaying = false;
>   };
>   
>   /* Inferior thread specific part of `struct infcall_suspend_state'.  */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index bbb98f6dcdb..f38d96b64df 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3089,6 +3089,8 @@ clear_proceed_status_thread (struct thread_info *tp)
>   
>     /* Discard any remaining commands or status from previous stop.  */
>     bpstat_clear (&tp->control.stop_bpstat);
> +
> +  tp->control.is_replaying = target_record_is_replaying (tp->ptid);
>   }
>   
>   /* Notify the current interpreter and observers that the target is about to
> @@ -8972,6 +8974,29 @@ keep_going_pass_signal (struct execution_control_state *ecs)
>     gdb_assert (ecs->event_thread->ptid == inferior_ptid);
>     gdb_assert (!ecs->event_thread->resumed ());
>   
> +  /* When a thread reaches the end of its execution history, it automatically
> +     stops replaying.  This is so the user doesn't need to explicitly stop it
> +     with a separate command.
> +
> +     We do not want a single command (e.g. continue) to transition from
> +     replaying to recording, though, e.g. when starting from a breakpoint we
> +     needed to step over at the end of the trace.  When we reach the end of the
> +     execution history during stepping, stop with no-history.
> +
> +     The other direction is fine.  When we're at the end of the execution
> +     history, we may reverse-continue to start replaying.  */
> +  if (ecs->event_thread->control.is_replaying
> +      && !target_record_is_replaying (ecs->event_thread->ptid))
> +    {
> +      interps_notify_no_history ();
> +      ecs->ws.set_no_history ();
> +      set_last_target_status (ecs->target, ecs->ptid, ecs->ws);
> +      stop_print_frame = true;
> +      stop_waiting (ecs);
> +      normal_stop ();
> +      return;
> +    }
> +
>     /* Save the pc before execution, to compare with pc after stop.  */
>     ecs->event_thread->prev_pc
>       = regcache_read_pc_protected (get_thread_regcache (ecs->event_thread));
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 6350400c318..ae273fda507 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -2340,6 +2340,16 @@ record_btrace_single_step_forward (struct thread_info *tp)
>     if (replay == NULL)
>       return btrace_step_no_history ();
>   
> +  /* The execution trace contains (and ends with) the current instruction.
> +     This instruction has not been executed, yet, so the trace really ends
> +     one instruction earlier.
> +
> +     We'd fail later on in btrace_insn_next () but we must not trigger
> +     breakpoints as we're not really able to step.  */
> +  btrace_insn_end (&end, btinfo);
> +  if (btrace_insn_cmp (replay, &end) == 0)
> +    return btrace_step_no_history ();
> +
>     /* Check if we're stepping a breakpoint.  */
>     if (record_btrace_replay_at_breakpoint (tp))
>       return btrace_step_stopped ();
> @@ -2362,15 +2372,6 @@ record_btrace_single_step_forward (struct thread_info *tp)
>       }
>     while (btrace_insn_get (replay) == NULL);
>   
> -  /* Determine the end of the instruction trace.  */
> -  btrace_insn_end (&end, btinfo);
> -
> -  /* The execution trace contains (and ends with) the current instruction.
> -     This instruction has not been executed, yet, so the trace really ends
> -     one instruction earlier.  */
> -  if (btrace_insn_cmp (replay, &end) == 0)
> -    return btrace_step_no_history ();
> -
>     return btrace_step_spurious ();
>   }
>   
> diff --git a/gdb/testsuite/gdb.btrace/cont-hang.exp b/gdb/testsuite/gdb.btrace/cont-hang.exp
> new file mode 100644
> index 00000000000..cddcf68b3ab
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/cont-hang.exp
> @@ -0,0 +1,43 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2024 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 that we do not hang when trying to continue over a breakpoint at
> +# the end of the trace.
> +
> +require allow_btrace_tests
> +
> +standard_testfile record_goto.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Trace the call to the test function.
> +gdb_test_no_output "record btrace"
> +gdb_test "next" "main\.3.*"
> +
> +# We need to be replaying, otherwise, we'd just continue recording.
> +gdb_test "reverse-stepi"
> +gdb_test "break"
> +
> +# Continuing will step over the breakpoint and then run into the end of
> +# the execution history.  This ends replay, so we can continue recording.
> +gdb_test "continue" "No more reverse-execution history.*"
> +gdb_continue_to_end
> diff --git a/gdb/testsuite/gdb.btrace/step-hang.exp b/gdb/testsuite/gdb.btrace/step-hang.exp
> new file mode 100644
> index 00000000000..91ea813955d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/step-hang.exp
> @@ -0,0 +1,42 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2024 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 that we do not hang when trying to step over a breakpoint at the
> +# end of the trace.
> +
> +require allow_btrace_tests
> +
> +standard_testfile record_goto.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Trace the call to the test function.
> +gdb_test_no_output "record btrace"
> +gdb_test "next" "main\.3.*"
> +
> +# We need to be replaying, otherwise, we'd just continue recording.
> +gdb_test "reverse-stepi"
> +gdb_test "break"
> +
> +# Stepping over the breakpoint ends replaying and we can continue recording.
> +gdb_test "step"  "main\.3.*"
> +gdb_continue_to_end
> diff --git a/gdb/testsuite/gdb.btrace/stepn.exp b/gdb/testsuite/gdb.btrace/stepn.exp
> new file mode 100644
> index 00000000000..4aec90adc65
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/stepn.exp
> @@ -0,0 +1,50 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2024 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 that step n does not start recording when issued while replaying.
> +
> +require allow_btrace_tests
> +
> +standard_testfile record_goto.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Trace the call to the test function.
> +gdb_test_no_output "record btrace"
> +gdb_test "next" "main\.3.*"
> +
> +# Stepping should bring us to the end of the execution history, but should
> +# not resume recording.
> +with_test_prefix "stepi" {
> +    gdb_test "reverse-stepi"
> +    gdb_test "stepi 5" "No more reverse-execution history.*main\.3.*"
> +}
> +
> +with_test_prefix "step" {
> +    gdb_test "reverse-step"
> +    gdb_test "step 5" "No more reverse-execution history.*main\.3.*"
> +}
> +
> +with_test_prefix "next" {
> +    gdb_test "reverse-next"
> +    gdb_test "next 5" "No more reverse-execution history.*main\.3.*"
> +}