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

[Guinevere Larsen <blarsen@redhat.com>]

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"