Re: [PATCH v5 4/4] gdb, infrun: fix multi-threaded reverse stepping

Guinevere Larsen
2024-04-12T19:50:36Z

On 4/10/24 04:46, Markus Metzger wrote:
> When reverse-stepping a thread that has a pending breakpoint event, the
> thread is not resumed as part of the infcmd function. A first resume
> notices the event and returns without resuming the target.
>
> If the corresponding breakpoint has been deleted, event processing results
> in a second resume that performs the intended stepping action. That
> resume happens after the infcmd function returned and the temporarily
> modified execution direction was restored. We end up resuming in the
> wrong direction.
>
> Store the direction in a thread's control state and change most of
> infrun to take it from there rather than using the global variable.
Change as a whole looks good. Just one question inlined.
> ---
> gdb/gdbthread.h | 10 +++
> gdb/infrun.c | 47 ++++++----
> gdb/infrun.h | 7 --
> .../gdb.btrace/implicit-stop-replaying.exp | 90 +++++++++++++++++++
> 4 files changed, 128 insertions(+), 26 deletions(-)
> create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
>
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 2d6b212cd32..12773c7bb12 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -92,6 +92,13 @@ enum step_over_calls_kind
> STEP_OVER_UNDEBUGGABLE
> };
>
> +/* Reverse execution. */
> +enum exec_direction_kind
> + {
> + EXEC_FORWARD,
> + EXEC_REVERSE
> + };
> +
> /* Inferior thread specific part of `struct infcall_control_state'.
>
> Inferior process counterpart is `struct inferior_control_state'. */
> @@ -179,6 +186,9 @@ struct thread_control_state
>
> /* Whether the thread was replaying when the command was issued. */
> bool is_replaying = false;
> +
> + /* The execution direction when the command was issued. */
> + enum exec_direction_kind execution_direction = EXEC_FORWARD;

I'm a bit unsure of this... Mostly wondering if this makes sense to be a
thread-specific information. Does it make sense for one thread to have a
direction being forward and another backwards? I would think this is not
a good idea (if not impossible).

I think this would make more sense as a member of inferior_control_state
instead.

--
Cheers,
Guinevere Larsen
She/Her/Hers

> };
>
> /* Inferior thread specific part of `struct infcall_suspend_state'. */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d3738ebbd66..2cdfc4b8fdc 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -95,7 +95,8 @@ static void insert_step_resume_breakpoint_at_caller (const frame_info_ptr &);
>
> static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR);
>
> -static bool maybe_software_singlestep (struct gdbarch *gdbarch);
> +static bool maybe_software_singlestep (const thread_info *tp,
> + gdbarch *gdbarch, CORE_ADDR pc);
>
> static void resume (gdb_signal sig);
>
> @@ -2369,11 +2370,12 @@ bool sched_multi = false;
> GDBARCH the current gdbarch. */
>
> static bool
> -maybe_software_singlestep (struct gdbarch *gdbarch)
> +maybe_software_singlestep (const thread_info *tp, gdbarch *gdbarch,
> + CORE_ADDR pc)
> {
> bool hw_step = true;
>
> - if (execution_direction == EXEC_FORWARD
> + if (tp->control.execution_direction == EXEC_FORWARD
> && gdbarch_software_single_step_p (gdbarch))
> hw_step = !insert_single_step_breakpoints (gdbarch);
>
> @@ -2532,6 +2534,10 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
> /* Install inferior's terminal modes. */
> target_terminal::inferior ();
>
> + scoped_restore save_exec_dir
> + = make_scoped_restore (&execution_direction,
> + tp->control.execution_direction);
> +
> /* Avoid confusing the next resume, if the next stop/resume
> happens to apply to another thread. */
> tp->set_stop_signal (GDB_SIGNAL_0);
> @@ -2792,6 +2798,7 @@ resume_1 (enum gdb_signal sig)
> insert_breakpoints ();
>
> resume_ptid = internal_resume_ptid (user_step);
> +
> do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
> tp->set_resumed (true);
> return;
> @@ -2841,7 +2848,7 @@ resume_1 (enum gdb_signal sig)
> set_step_over_info (aspace, regcache_read_pc (regcache), 0,
> tp->global_num);
>
> - step = maybe_software_singlestep (gdbarch);
> + step = maybe_software_singlestep (tp, gdbarch, pc);
>
> insert_breakpoints ();
> }
> @@ -2860,7 +2867,7 @@ resume_1 (enum gdb_signal sig)
>
> /* Do we need to do it the hard way, w/temp breakpoints? */
> else if (step)
> - step = maybe_software_singlestep (gdbarch);
> + step = maybe_software_singlestep (tp, gdbarch, pc);
>
> /* Currently, our software single-step implementation leads to different
> results than hardware single-stepping in one situation: when stepping
> @@ -2931,7 +2938,7 @@ resume_1 (enum gdb_signal sig)
> else
> resume_ptid = internal_resume_ptid (user_step);
>
> - if (execution_direction != EXEC_REVERSE
> + if (tp->control.execution_direction != EXEC_REVERSE
> && step && breakpoint_inserted_here_p (aspace, pc))
> {
> /* There are two cases where we currently need to step a
> @@ -3100,6 +3107,7 @@ clear_proceed_status_thread (struct thread_info *tp)
> bpstat_clear (&tp->control.stop_bpstat);
>
> tp->control.is_replaying = target_record_is_replaying (tp->ptid);
> + tp->control.execution_direction = ::execution_direction;
> }
>
> /* Notify the current interpreter and observers that the target is about to
> @@ -3209,7 +3217,7 @@ schedlock_applies (struct thread_info *tp)
> && tp->control.stepping_command)
> || (scheduler_mode == schedlock_replay
> && target_record_will_replay (minus_one_ptid,
> - execution_direction)));
> + tp->control.execution_direction)));
> }
>
> /* When FORCE_P is false, set process_stratum_target::COMMIT_RESUMED_STATE
> @@ -3646,7 +3654,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
> if (cur_thr->stop_pc_p ()
> && pc == cur_thr->stop_pc ()
> && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here
> - && execution_direction != EXEC_REVERSE)
> + && cur_thr->control.execution_direction != EXEC_REVERSE)
> /* There is a breakpoint at the address we will resume at,
> step one instruction before inserting breakpoints so that
> we do not stop right away (and report a second hit at this
> @@ -4962,7 +4970,7 @@ adjust_pc_after_break (struct thread_info *thread,
> breakpoint at PC - 1. We'd then report a hit on B1, although
> INSN1 hadn't been de-executed yet. Doing nothing is the correct
> behaviour. */
> - if (execution_direction == EXEC_REVERSE)
> + if (thread->control.execution_direction == EXEC_REVERSE)
> return;
>
> /* If the target can tell whether the thread hit a SW breakpoint,
> @@ -7561,7 +7569,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>
> delete_step_resume_breakpoint (ecs->event_thread);
> if (ecs->event_thread->control.proceed_to_finish
> - && execution_direction == EXEC_REVERSE)
> + && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
> {
> struct thread_info *tp = ecs->event_thread;
>
> @@ -7576,7 +7584,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> }
> fill_in_stop_func (gdbarch, ecs);
> if (ecs->event_thread->stop_pc () == ecs->stop_func_start
> - && execution_direction == EXEC_REVERSE)
> + && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
> {
> /* We are stepping over a function call in reverse, and just
> hit the step-resume breakpoint at the start address of
> @@ -7701,7 +7709,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>
> if (pc_in_thread_step_range (ecs->event_thread->stop_pc (),
> ecs->event_thread)
> - && (execution_direction != EXEC_REVERSE
> + && (ecs->event_thread->control.execution_direction != EXEC_REVERSE
> || *curr_frame_id == original_frame_id))
> {
> infrun_debug_printf
> @@ -7720,7 +7728,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> if (stop_pc == ecs->event_thread->control.step_range_start
> && stop_pc != ecs->stop_func_start
> - && execution_direction == EXEC_REVERSE)
> + && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
> end_stepping_range (ecs);
> else
> keep_going (ecs);
> @@ -7742,7 +7750,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> backward through the trampoline code, and that's handled further
> down, so there is nothing for us to do here. */
>
> - if (execution_direction != EXEC_REVERSE
> + if (ecs->event_thread->control.execution_direction != EXEC_REVERSE
> && ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE
> && in_solib_dynsym_resolve_code (ecs->event_thread->stop_pc ())
> && (ecs->event_thread->control.step_start_function == nullptr
> @@ -7891,7 +7899,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>
> /* Reverse stepping through solib trampolines. */
>
> - if (execution_direction == EXEC_REVERSE
> + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE
> && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE
> && (gdbarch_skip_trampoline_code (gdbarch, frame, stop_pc)
> || (ecs->stop_func_start == 0
> @@ -7919,7 +7927,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> stepped into (backwards), and continue to there. When we
> get there, we'll need to single-step back to the caller. */
>
> - if (execution_direction == EXEC_REVERSE)
> + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
> {
> /* If we're already at the start of the function, we've either
> just stepped backward into a single instruction function,
> @@ -7982,7 +7990,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> tmp_sal)
> && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
> {
> - if (execution_direction == EXEC_REVERSE)
> + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
> handle_step_into_function_backward (gdbarch, ecs);
> else
> handle_step_into_function (gdbarch, ecs);
> @@ -8000,7 +8008,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> return;
> }
>
> - if (execution_direction == EXEC_REVERSE)
> + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
> {
> /* If we're already at the start of the function, we've either just
> stepped backward into a single instruction function without line
> @@ -8029,7 +8037,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>
> /* Reverse stepping through solib trampolines. */
>
> - if (execution_direction == EXEC_REVERSE
> + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE
> && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE)
> {
> CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> @@ -8612,6 +8620,7 @@ keep_going_stepped_thread (struct thread_info *tp)
>
> tp->set_resumed (true);
> resume_ptid = internal_resume_ptid (tp->control.stepping_command);
> +
> do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
> }
> else
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index 5f83ca2b4c3..c3d27c3dea3 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -107,13 +107,6 @@ extern bool disable_randomization;
> current location. */
> extern ULONGEST get_stop_id (void);
>
> -/* Reverse execution. */
> -enum exec_direction_kind
> - {
> - EXEC_FORWARD,
> - EXEC_REVERSE
> - };
> -
> /* The current execution direction. */
> extern enum exec_direction_kind execution_direction;
>
> diff --git a/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
> new file mode 100644
> index 00000000000..20240da1dc1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
> @@ -0,0 +1,90 @@
> +# 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 stop replaying other threads when stepping a thread at the
> +# end of its execution history.
> +#
> +# This is similar to the last test in multi-thread-step.exp, except that
> +# we reverse-step instead of record goto begin to start replaying and we
> +# step instead of continuing.
> +#
> +# This triggered a bug where GDB confused the execution direction and kept
> +# stepping both threads backwards instead of forwards.
> +
> +require allow_btrace_tests
> +
> +standard_testfile multi-thread-step.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile \
> + {debug pthreads}] {
> + return -1
> +}
> +
> +if ![runto_main] {
> + return -1
> +}
> +
> +# Set up breakpoints.
> +set bp_1 [gdb_get_line_number "bp.1" $srcfile]
> +set bp_2 [gdb_get_line_number "bp.2" $srcfile]
> +
> +# Trace the code between the two breakpoints.
> +gdb_breakpoint $srcfile:$bp_1
> +gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*"
> +
> +# Make sure GDB knows about the new thread.
> +gdb_test "info threads"
> +gdb_test_no_output "record btrace"
> +
> +# We have two threads at or close to bp.1 but handled only one stop event.
> +# Remove the breakpoint so we do not need to deal with the 2nd event.
> +delete_breakpoints
> +gdb_breakpoint $srcfile:$bp_2
> +gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*"
> +
> +# Determine the thread that reported the breakpoint.
> +set thread "bad"
> +gdb_test_multiple "thread" "thread" {
> + -re -wrap "Current thread is \($decimal\).*" {
> + set thread $expect_out(1,string)
> + }
> +}
> +
> +# Determine the other thread.
> +set other "bad"
> +if { $thread == 1 } {
> + set other 2
> +} elseif { $thread == 2 } {
> + set other 1
> +}
> +
> +# This test only works for scheduler-locking 'replay'.
> +gdb_test_no_output "set scheduler-locking replay"
> +
> +# Remove breakpoints or we might run into it right away.
> +delete_breakpoints
> +
> +# Start replaying the other thread.
> +gdb_test "thread apply $other reverse-stepi"
> +gdb_test "thread apply $other info record" "Replay in progress.*" \
> + "other thread is replaying"
> +
> +# Step the thread that reported the breakpoint, which is not replaying.
> +gdb_test "next" "return arg;.*"
> +
> +# The other thread stopped replaying.
> +gdb_test "thread apply $other info record" "Recorded \[^\\\r\\\n\]*" \
> + "other thread stopped replaying"