[1/2] fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp

  GDB maintainers:

This patch fixes the issues with the reverse-finish command on X86. 
The reverse-finish command now correctly stops at the first instruction
in the source code line of the caller.  It now only requires a single
reverse-step or reverse-next instruction to get back to the previous
source code line.

It also adds a new testcase, gdb.reverse/finish-reverse-next.exp, and
updates several existing testcases. 

Please let me know if you have any comments on the patch.  Thanks.

                    Carl 

--------------------------------------------------------------
X86: reverse-finish fix

Currently on X86, when executing the finish command in reverse, gdb does a
single step from the first instruction in the callee to get back to the
caller.  GDB stops on the last instruction in the source code line where
the call was made.  When stopped at the last instruction of the source code
line, a reverse next or step command will stop at the first instruction
of the same source code line thus requiring two step/next commands to
reach the previous source code line.  It should only require one step/next
command to reach the previous source code line.

By contrast, a reverse next or step command from the first line in a
function stops at the first instruction in the source code line where the
call was made.

This patch fixes the reverse finish command so it will stop at the first
instruction of the source line where the function call was made.  The
behavior on X86 for the reverse-finish command now matches doing a
reverse-next from the beginning of the function.

The proceed_to_finish flag in struct thread_control_state is no longer
used.  This patch removes the declaration, initialization and setting of
the flag.

This patch requires a number of regression tests to be updated.  Test
gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
tests gdb.reverse/amd64-ailcall-reverse.exp and
gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
result.

This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
reverse-finish command when returning from the entry point and from the
body of the function.

The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp
was moved to lib/gdb.exp and renamed cmd_until.

The patch has been tested on X86 and PowerPC to verify no additional
regression failures occured.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29927
---
 gdb/gdbthread.h                               |   4 -
 gdb/infcall.c                                 |   3 -
 gdb/infcmd.c                                  |  32 +++---
 gdb/infrun.c                                  |  41 +++----
 gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
 .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
 .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
 .../gdb.reverse/finish-reverse-next.exp       | 108 ++++++++++++++++++
 gdb/testsuite/gdb.reverse/finish-reverse.exp  |   5 +
 .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
 .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++------
 gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
 gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
 gdb/testsuite/lib/gdb.exp                     |  33 ++++++
 14 files changed, 240 insertions(+), 106 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp
  

On 1/11/23 19:27, Carl Love via Gdb-patches wrote:
> This patch requires a number of regression tests to be updated.  Test
> gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
> previous line.

Hi Carl,

FYI, I stumbled upon a FAIL (not related to reverse-finish) in 
gdb.reverse/step-reverse.exp, see PR29962 ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=29962 ) and wrote a 
tentative patch for it  ( 
https://sourceware.org/bugzilla/attachment.cgi?id=14588 ), and ended up 
making the same fix in the gdb.mi/mi-reverse.exp test-case.

I'll test the patch a bit further before posting it.

Thanks,
- Tom
  

Tom:



On Thu, 2023-01-12 at 17:56 +0100, Tom de Vries wrote:
> On 1/11/23 19:27, Carl Love via Gdb-patches wrote:
> > This patch requires a number of regression tests to be
> > updated.  Test
> > gdb.mi/mi-reverse.exp no longer needs to execute two steps to get
> > to the
> > previous line.
> 
> Hi Carl,
> 
> FYI, I stumbled upon a FAIL (not related to reverse-finish) in 
> gdb.reverse/step-reverse.exp, see PR29962 ( 
> https://sourceware.org/bugzilla/show_bug.cgi?id=29962  ) and wrote
> a 
> tentative patch for it  ( 
> https://sourceware.org/bugzilla/attachment.cgi?id=14588  ), and ended
> up 
> making the same fix in the gdb.mi/mi-reverse.exp test-case.
> 
> I'll test the patch a bit further before posting it.

On PPC64 there are also issues with reverse stepping over a line that
has multiple executable statements in it.  Not sure if this is related
to what you are seeing.  

There is a patch that I have worked on with Luis Machado to fix.  We
have posted versions of it to the mailing list.  The subject line is:

Fix reverse stepping multiple contiguous PC ranges over the line table

which might be relevant.  Unfortunately, work on this patch has been
stalled for awhile.  The patch fixes 5 failures in  gdb.reverse/solib-
precsave.exp and 5 failures in gdb.reverse/solib-reverse.exp.  

You might want to take a look at it to see if it helps.

                            Carl
  

On 11/01/2023 19:27, Carl Love via Gdb-patches wrote:
> GDB maintainers:
>
> This patch fixes the issues with the reverse-finish command on X86.
> The reverse-finish command now correctly stops at the first instruction
> in the source code line of the caller.  It now only requires a single
> reverse-step or reverse-next instruction to get back to the previous
> source code line.
>
> It also adds a new testcase, gdb.reverse/finish-reverse-next.exp, and
> updates several existing testcases.
>
> Please let me know if you have any comments on the patch.  Thanks.
Thanks for looking at this, this is a nice change. I just have a couple 
of comments, mostly related to the testsuite side.
>                      Carl
>
> --------------------------------------------------------------
> X86: reverse-finish fix
>
> Currently on X86, when executing the finish command in reverse, gdb does a
> single step from the first instruction in the callee to get back to the
> caller.  GDB stops on the last instruction in the source code line where
> the call was made.  When stopped at the last instruction of the source code
> line, a reverse next or step command will stop at the first instruction
> of the same source code line thus requiring two step/next commands to
> reach the previous source code line.  It should only require one step/next
> command to reach the previous source code line.
>
> By contrast, a reverse next or step command from the first line in a
> function stops at the first instruction in the source code line where the
> call was made.
>
> This patch fixes the reverse finish command so it will stop at the first
> instruction of the source line where the function call was made.  The
> behavior on X86 for the reverse-finish command now matches doing a
> reverse-next from the beginning of the function.
>
> The proceed_to_finish flag in struct thread_control_state is no longer
> used.  This patch removes the declaration, initialization and setting of
> the flag.
>
> This patch requires a number of regression tests to be updated.  Test
> gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
> previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
> and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
> tests gdb.reverse/amd64-ailcall-reverse.exp and
s/ailcall/tailcall
> gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
> result.
>
> This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
> reverse-finish command when returning from the entry point and from the
> body of the function.
>
> The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp
> was moved to lib/gdb.exp and renamed cmd_until.
I'm not a big fan of the name cmd_until, because it sounded to me like 
you were testing the GDB command until. I think repeat_cmd_until or 
repeat_until would avoid this possible confusion.
>
> The patch has been tested on X86 and PowerPC to verify no additional
> regression failures occured.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29927
If you add record/29927 somewhere along the text of your commit message, 
there is some automation that will comment on the bugzilla bug 
specifying this commit. Might be worth doing for future reference.
> ---
>   gdb/gdbthread.h                               |   4 -
>   gdb/infcall.c                                 |   3 -
>   gdb/infcmd.c                                  |  32 +++---
>   gdb/infrun.c                                  |  41 +++----
>   gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
>   .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
>   .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
>   .../gdb.reverse/finish-reverse-next.exp       | 108 ++++++++++++++++++
>   gdb/testsuite/gdb.reverse/finish-reverse.exp  |   5 +
>   .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
>   .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++------
>   gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
>   gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
>   gdb/testsuite/lib/gdb.exp                     |  33 ++++++
>   14 files changed, 240 insertions(+), 106 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
>   create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp
>
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 11d69fceab0..e4edff2d621 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -150,10 +150,6 @@ struct thread_control_state
>        the finished single step.  */
>     int trap_expected = 0;
>   
> -  /* Nonzero if the thread is being proceeded for a "finish" command
> -     or a similar situation when return value should be printed.  */
> -  int proceed_to_finish = 0;
> -
>     /* Nonzero if the thread is being proceeded for an inferior function
>        call.  */
>     int in_infcall = 0;
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index e09904f9a35..116605c43ef 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -625,9 +625,6 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
>   
>     disable_watchpoints_before_interactive_call_start ();
>   
> -  /* We want to print return value, please...  */
> -  call_thread->control.proceed_to_finish = 1;
> -
>     try
>       {
>         /* Infcalls run synchronously, in the foreground.  */
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 0497ad05091..9c42efeae8d 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm *sm)
>   
>     sal = find_pc_line (func_addr, 0);
>   
> -  tp->control.proceed_to_finish = 1;
> -  /* Special case: if we're sitting at the function entry point,
> -     then all we need to do is take a reverse singlestep.  We
> -     don't need to set a breakpoint, and indeed it would do us
> -     no good to do so.
> -
> -     Note that this can only happen at frame #0, since there's
> -     no way that a function up the stack can have a return address
> -     that's equal to its entry point.  */
> +  frame_info_ptr frame = get_selected_frame (nullptr);
>   
>     if (sal.pc != pc)
>       {
> -      frame_info_ptr frame = get_selected_frame (nullptr);
>         struct gdbarch *gdbarch = get_frame_arch (frame);
>   
>         /* Set a step-resume at the function's entry point.  Once that's
> @@ -1743,16 +1734,22 @@ finish_backward (struct finish_command_fsm *sm)
>         sr_sal.pspace = get_frame_program_space (frame);
>         insert_step_resume_breakpoint_at_sal (gdbarch,
>   					    sr_sal, null_frame_id);
> -
> -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>       }
>     else
>       {
> -      /* We're almost there -- we just need to back up by one more
> -	 single-step.  */
> -      tp->control.step_range_start = tp->control.step_range_end = 1;
> -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> +      /* We are exactly at the function entry point.  Note that this
> +	 can only happen at frame #0.
> +
> +	 When setting a step range, need to call set_step_info
> +	 to setup the current_line/symtab fields as well.  */
> +      set_step_info (tp, frame, find_pc_line (pc, 0));
> +
> +      /* Return using a step range so we will keep stepping back
> +	 to the first instruction in the source code line.  */
> +      tp->control.step_range_start = sal.pc;
> +      tp->control.step_range_end = sal.pc;
>       }
> +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>   }
>   
>   /* finish_forward -- helper function for finish_command.  FRAME is the
> @@ -1778,9 +1775,6 @@ finish_forward (struct finish_command_fsm *sm, frame_info_ptr frame)
>   
>     set_longjmp_breakpoint (tp, frame_id);
>   
> -  /* We want to print return value, please...  */
> -  tp->control.proceed_to_finish = 1;
> -
>     proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>   }
>   
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 181d961d80d..8ed538ea9ec 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct thread_info *tp)
>   
>     tp->control.stop_step = 0;
>   
> -  tp->control.proceed_to_finish = 0;
> -
>     tp->control.stepping_command = 0;
>   
>     /* Discard any remaining commands or status from previous stop.  */
> @@ -6737,31 +6735,28 @@ process_event_stop_test (struct execution_control_state *ecs)
>   
>       case BPSTAT_WHAT_STEP_RESUME:
>         infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
> -
>         delete_step_resume_breakpoint (ecs->event_thread);
> -      if (ecs->event_thread->control.proceed_to_finish
> -	  && execution_direction == EXEC_REVERSE)
> +      fill_in_stop_func (gdbarch, ecs);
> +
> +      if (execution_direction == EXEC_REVERSE
> +	  && ecs->event_thread->stop_pc () == ecs->stop_func_start)
Is there any reason to invert the order of checks here? The second if 
clause is the same and keeping that would make the changes easier to parse.
>   	{
>   	  struct thread_info *tp = ecs->event_thread;
> +	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), 0);
>   
> -	  /* We are finishing a function in reverse, and just hit the
> -	     step-resume breakpoint at the start address of the
> -	     function, and we're almost there -- just need to back up
> -	     by one more single-step, which should take us back to the
> -	     function call.  */
> -	  tp->control.step_range_start = tp->control.step_range_end = 1;
> -	  keep_going (ecs);
> -	  return;
> -	}
> -      fill_in_stop_func (gdbarch, ecs);
> -      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
> -	  && 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
> -	     the function.  Go back to single-stepping, which should
> -	     take us back to the function call.  */
> -	  ecs->event_thread->stepping_over_breakpoint = 1;
> +	  /* When setting a step range, need to call set_step_info
> +	     to setup the current_line/symtab fields as well.  */
> +	  set_step_info (tp, frame, stop_pc_sal);
> +
> +	  /* We are finishing a function in reverse or stepping over a function
> +	     call in reverse, and just hit the step-resume breakpoint at the
> +	     start address of the function, and we're almost there -- just need
> +	     to back up to the function call.
> +
> +	     Return using a step range so we will keep stepping back to the
> +	     first instruction in the source code line.  */
> +	  tp->control.step_range_start = ecs->stop_func_start;
> +	  tp->control.step_range_end = ecs->stop_func_start;
>   	  keep_going (ecs);
>   	  return;
>   	}
> diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
> index d631beb17c8..30635ab1754 100644
> --- a/gdb/testsuite/gdb.mi/mi-reverse.exp
> +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
> @@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} {
>   	"basics.c" $line_main_callme_1 "" \
>   	"reverse finish from callme"
>   
> -    # Test exec-reverse-next
> -    #   It takes two steps to get back to the previous line,
> -    #   as the first step moves us to the start of the current line,
> -    #   and the one after that moves back to the previous line.
> -
> -    mi_execute_to "exec-next --reverse 2" \
> +    mi_execute_to "exec-next --reverse" \
>    	"end-stepping-range" "main" "" \
>    	"basics.c" $line_main_hello "" \
> - 	"reverse next to get over the call to do_nothing"
> +	"reverse next to get over the call to do_nothing"
>   
>       # Test exec-reverse-step
>   
> diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> index 52a87faabf7..9964b4f8e4b 100644
> --- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> @@ -44,6 +44,5 @@ if [supports_process_record] {
>   gdb_test "next" {f \(\);} "next to f"
>   gdb_test "next" {v = 3;} "next to v = 3"
>   
> -# FAIL was:
> -# 29        g ();
> -gdb_test "reverse-next" {f \(\);}
> +# Reverse step back into f ().  Puts us at call to g () in function f ().
> +gdb_test "reverse-next" {g \(\);}
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> new file mode 100644
> index 00000000000..42e41b5a2e0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> @@ -0,0 +1,48 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2012-2022 Free Software Foundation, Inc.
copyright year should be 2023.
> +
> +   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/>.  */
> +
> +/* The reverse finish command should return from a function and stop on
> +   the first instruction of the source line where the function call is made.
> +   Specifically, the behavior should match doing a reverse next from the
> +   first instruction in the function.  GDB should only require one reverse
> +   step or next statement to reach the previous source code line.
> +
> +   This test verifies the fix for gdb bugzilla:
> +
> +   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> +*/
> +
> +int
> +function1 (int a, int b)   // FUNCTION1
> +{
> +  int ret = 0;
> +
> +  ret = a + b;
> +  return ret;
> +}
> +
> +int
> +main(int argc, char* argv[])
> +{
> +  int a, b;
> +
> +  a = 1;
> +  b = 5;
> +
> +  function1 (a, b);   // CALL FUNCTION
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> new file mode 100644
> index 00000000000..7880de10ffc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -0,0 +1,108 @@
> +# Copyright 2008-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/>.  */
> +
> +# This file is part of the GDB testsuite.  It tests reverse stepping.
> +# Lots of code borrowed from "step-test.exp".
> +
> +# The reverse finish command should return from a function and stop on
> +# the first instruction of the source line where the function call is made.
> +# Specifically, the behavior should match doing a reverse next from the
> +# first instruction in the function.  GDB should only take one reverse step
> +# or next statement to reach the previous source code line.
> +
> +# This testcase verifies the reverse-finish command stops at the first
> +# instruction in the source code line where the function was called.  There
> +# are two scenarios that must be checked:
> +#   1) gdb is at the entry point instruction for the function
> +#   2) gdb is in the body of the function.

While testing locally, I ran into a bug with reverse finish at the 
epilogue of the function, that your patch also fixed. It would be nice 
if the test extended that. And since the bug was that GDB stopped 
responding and even ctrl+c did nothing, I would suggest adding it as the 
last test.

> +
> +# This test verifies the fix for gdb bugzilla:
> +#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> +
> +if ![supports_reverse] {
> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +runto_main
> +set target_remote [gdb_is_target_remote]
> +
> +if [supports_process_record] {
> +    # Activate process record/replay.
> +    gdb_test_no_output "record" "turn on process record for test1"
> +}
> +
> +
> +### TEST 1: reverse finish from the entry point instruction in
> +### function1.
> +
> +# Set breakpoint at call to function1 in main.
> +set FUNCTION_test  [gdb_get_line_number "CALL FUNCTION" $srcfile]
> +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at .*" \
> +    "set breakpoint on function1 call to stepi into function"

There is a proc in lib/gdb.exp called gdb_breakpoint which couldsimplify 
this gdb_test to

gdb_breakpoint $srcfile:$FUNCTION_test temporary

And would remove the need for the delete_breakpoints call later.

> +
> +# Continue to break point at function1 call in main.
> +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*" \
> +    "stopped at function1 entry point instruction to stepi into function"
You can use gdb_continue_to_breakpoint here instead.
> +
> +# stepi until we see "{" indicating we entered function1
> +cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
> +
> +delete_breakpoints
> +
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
> +    "reverse-finish function1 "
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line.  It should only take one reverse next command to get to the previous
> +# source line.   If GDB stops at the last instruction in the source code line
> +# it will take two reverse next instructions to get to the previous source
> +# line.
> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
> +
> +# Clear the recorded log.
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test1"
> +gdb_test_no_output "record" "turn on process record for test2"
> +
> +
> +### TEST 2: reverse finish from the body of function1.
> +
> +# Set breakpoint at call to function1 in main.
> +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at .*" \
> +    "set breakpoint on function1 call to step into body of function"
> +
> +# Continue to break point at function1 call in main.
> +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*" \
> +    "stopped at function1 entry point instruction to step to body of function"
> +
> +delete_breakpoints
> +
> +# do a step instruction to get to the body of the function
> +gdb_test "step" ".*int ret = 0;.*" "step test 1"
> +
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
> +    "reverse-finish function1 call from function body"
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line.  It should only take one reverse next command to get to the previous
> +# source line.
> +gdb_test "reverse-next" ".*b = 5;.*" \
> +    "reverse next at b = 5, from function body"
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse.exp b/gdb/testsuite/gdb.reverse/finish-reverse.exp
> index 01ba309420c..a05cb81892a 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse.exp
> @@ -16,6 +16,11 @@
>   # This file is part of the GDB testsuite.  It tests 'finish' with
>   # reverse debugging.
>   
> +# This test verifies the fix for gdb bugzilla:
> +
> +# https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> +
> +

Is this comment a left over from an earlier version?

I actually wonder if the whole new test is needed, or if you can just 
add a couple of new tests to finish-reverse.exp; is there any reason you 
went with the new test instead?

>   if ![supports_reverse] {
>       return
>   }
> diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> index 1ca7c2ce559..eb03051625a 100644
> --- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> @@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3"
>   # {
>   gdb_test "reverse-step" {nodebug \(\);}
>   
> -# FAIL was:
> -# No more reverse-execution history.
> -# {
> -gdb_test "reverse-next" {f \(\);}
> +gdb_test "reverse-next" {g \(\);}
> diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> index ad637899e5b..1928cdda217 100644
> --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> @@ -39,39 +39,6 @@ if { ![runto_main] } {
>       return -1
>   }
>   
> -# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
> -#
> -#  COMMAND is a stepping command
> -#  CURRENT is a string matching the current location
> -#  TARGET  is a string matching the target location
> -#  TEST    is the test name
> -#
> -# The function issues repeated COMMANDs as long as the location matches
> -# CURRENT up to a maximum of 100 steps.
> -#
> -# TEST passes if the resulting location matches TARGET and fails
> -# otherwise.
> -#
> -proc step_until { command current target test } {
> -    global gdb_prompt
> -
> -    set count 0
> -    gdb_test_multiple "$command" "$test" {
> -        -re "$current.*$gdb_prompt $" {
> -            incr count
> -            if { $count < 100 } {
> -                send_gdb "$command\n"
> -                exp_continue
> -            } else {
> -                fail "$test"
> -            }
> -        }
> -        -re "$target.*$gdb_prompt $" {
> -            pass "$test"
> -        }
> -    }
> -}
> -
>   gdb_test_no_output "record"
>   gdb_test "next" ".*" "record trace"
>   
> @@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \
>       "reverse-step through thunks and over inc"
>   
>   # We can use instruction stepping to step into thunks.
> -step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
> -step_until "stepi" "indirect_thunk" "inc" \
> +cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
> +cmd_until "stepi" "indirect_thunk" "inc" \
>       "stepi out of call thunk into inc"
>   set alphanum_re "\[a-zA-Z0-9\]"
>   set pic_thunk_re  "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re* \\(\\)"
> -step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
> -step_until "stepi" "return_thunk" "apply" \
> +cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
> +cmd_until "stepi" "return_thunk" "apply" \
>       "stepi out of return thunk back into apply"
>   
> -step_until "reverse-stepi" "apply" "return_thunk" \
> +cmd_until "reverse-stepi" "apply" "return_thunk" \
>       "reverse-stepi into return thunk"
> -step_until "reverse-stepi" "return_thunk" "inc" \
> +cmd_until "reverse-stepi" "return_thunk" "inc" \
>       "reverse-stepi out of return thunk into inc"
> -step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
> +cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
>       "reverse-stepi into call thunk"
> -step_until "reverse-stepi" "indirect_thunk" "apply" \
> +cmd_until "reverse-stepi" "indirect_thunk" "apply" \
>       "reverse-stepi out of call thunk into apply"
> diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp b/gdb/testsuite/gdb.reverse/until-precsave.exp
> index 0c2d7537cd6..777aec94ac1 100644
> --- a/gdb/testsuite/gdb.reverse/until-precsave.exp
> +++ b/gdb/testsuite/gdb.reverse/until-precsave.exp
> @@ -142,7 +142,7 @@ gdb_test "advance marker2" \
>   # Finish out to main scope (backward)
>   
>   gdb_test "finish" \
> -    " in main .*$srcfile:$bp_location20.*" \
> +    "main .*$srcfile:$bp_location20.*" \
This change doesn't seem connected to anything in this patch, is this 
just a cosmetic change or was there some problem?
>       "reverse-finish from marker2"
>   
>   # Advance backward to last line of factorial (outer invocation)
> diff --git a/gdb/testsuite/gdb.reverse/until-reverse.exp b/gdb/testsuite/gdb.reverse/until-reverse.exp
> index 23fc881dbf2..3a05953329f 100644
> --- a/gdb/testsuite/gdb.reverse/until-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/until-reverse.exp
> @@ -113,7 +113,7 @@ gdb_test "advance marker2" \
>   # Finish out to main scope (backward)
>   
>   gdb_test "finish" \
> -    " in main .*$srcfile:$bp_location20.*" \
> +    "main .*$srcfile:$bp_location20.*" \
same here.
>       "reverse-finish from marker2"
>   
>   # Advance backward to last line of factorial (outer invocation)
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index c41d4698d66..25f42eb5510 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9301,6 +9301,39 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
>       }
>   }
>   
> +# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
> +#
> +#  COMMAND is a stepping command
> +#  CURRENT is a string matching the current location
> +#  TARGET  is a string matching the target location
> +#  TEST    is the test name
> +#
> +# The function issues repeated COMMANDs as long as the location matches
> +# CURRENT up to a maximum of 100 steps.
> +#
> +# TEST passes if the resulting location matches TARGET and fails
> +# otherwise.
> +
> +proc cmd_until { command current target test } {
> +    global gdb_prompt
> +
> +    set count 0
> +    gdb_test_multiple "$command" "$test" {
> +	-re "$current.*$gdb_prompt $" {
> +	    incr count
> +	    if { $count < 100 } {
> +		send_gdb "$command\n"
> +		exp_continue
> +	    } else {
> +		fail "$test"
> +	    }
> +	}
> +	-re "$target.*$gdb_prompt $" {
> +	    pass "$test"
> +	}
> +    }
> +}
> +
>   # Check if the compiler emits epilogue information associated
>   # with the closing brace or with the last statement line.
>   #
  

On 11/01/2023 19:27, Carl Love via Gdb-patches wrote:
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 181d961d80d..8ed538ea9ec 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct thread_info *tp)
>   
>     tp->control.stop_step = 0;
>   
> -  tp->control.proceed_to_finish = 0;
> -
>     tp->control.stepping_command = 0;
>   
>     /* Discard any remaining commands or status from previous stop.  */
> @@ -6737,31 +6735,28 @@ process_event_stop_test (struct execution_control_state *ecs)
>   
>       case BPSTAT_WHAT_STEP_RESUME:
Something else that I failed to notice. Since you removed both comments 
that mention that this case is here for reverse finishing, there is no 
good way to figure out what GDB wants to do when this part of the code 
is reached. Adding a comment here mentioning it would fix that.
>         infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
> -
>         delete_step_resume_breakpoint (ecs->event_thread);
> -      if (ecs->event_thread->control.proceed_to_finish
> -	  && execution_direction == EXEC_REVERSE)
> +      fill_in_stop_func (gdbarch, ecs);
> +
> +      if (execution_direction == EXEC_REVERSE
> +	  && ecs->event_thread->stop_pc () == ecs->stop_func_start)
>   	{
>   	  struct thread_info *tp = ecs->event_thread;
> +	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), 0);
>   
> -	  /* We are finishing a function in reverse, and just hit the
> -	     step-resume breakpoint at the start address of the
> -	     function, and we're almost there -- just need to back up
> -	     by one more single-step, which should take us back to the
> -	     function call.  */
> -	  tp->control.step_range_start = tp->control.step_range_end = 1;
> -	  keep_going (ecs);
> -	  return;
> -	}
> -      fill_in_stop_func (gdbarch, ecs);
> -      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
> -	  && 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
> -	     the function.  Go back to single-stepping, which should
> -	     take us back to the function call.  */
> -	  ecs->event_thread->stepping_over_breakpoint = 1;
> +	  /* When setting a step range, need to call set_step_info
> +	     to setup the current_line/symtab fields as well.  */
> +	  set_step_info (tp, frame, stop_pc_sal);
> +
> +	  /* We are finishing a function in reverse or stepping over a function
> +	     call in reverse, and just hit the step-resume breakpoint at the
> +	     start address of the function, and we're almost there -- just need
> +	     to back up to the function call.
> +
> +	     Return using a step range so we will keep stepping back to the
> +	     first instruction in the source code line.  */
> +	  tp->control.step_range_start = ecs->stop_func_start;
> +	  tp->control.step_range_end = ecs->stop_func_start;
>   	  keep_going (ecs);
>   	  return;
>   	}
  

Bruno:

On Fri, 2023-01-13 at 14:33 +0100, Bruno Larsen wrote:
> > +# This file is part of the GDB testsuite.  It tests reverse
> > stepping.
> > +# Lots of code borrowed from "step-test.exp".
> > +
> > +# The reverse finish command should return from a function and
> > stop on
> > +# the first instruction of the source line where the function call
> > is made.
> > +# Specifically, the behavior should match doing a reverse next
> > from the
> > +# first instruction in the function.  GDB should only take one
> > reverse step
> > +# or next statement to reach the previous source code line.
> > +
> > +# This testcase verifies the reverse-finish command stops at the
> > first
> > +# instruction in the source code line where the function was
> > called.  There
> > +# are two scenarios that must be checked:
> > +#   1) gdb is at the entry point instruction for the function
> > +#   2) gdb is in the body of the function.
> 
> While testing locally, I ran into a bug with reverse finish at the 
> epilogue of the function, that your patch also fixed. It would be
> nice 
> if the test extended that. And since the bug was that GDB stopped 
> responding and even ctrl+c did nothing, I would suggest adding it as
> the 
> last test.

I haven't run into the issue that mentioned about GDB not responding in
the epilogue.  I will have to go play with that to see if I can
reproduce it.  If you have any specific instructions on how you ran
into it that would be interesting and helpful.

I have read thru the other comments and will work on them and update
you on those fixes later.  Thanks for the feedback.

                        Carl
  

On 13/01/2023 17:43, Carl Love wrote:
> Bruno:
>
> On Fri, 2023-01-13 at 14:33 +0100, Bruno Larsen wrote:
>>> +# This file is part of the GDB testsuite.  It tests reverse
>>> stepping.
>>> +# Lots of code borrowed from "step-test.exp".
>>> +
>>> +# The reverse finish command should return from a function and
>>> stop on
>>> +# the first instruction of the source line where the function call
>>> is made.
>>> +# Specifically, the behavior should match doing a reverse next
>>> from the
>>> +# first instruction in the function.  GDB should only take one
>>> reverse step
>>> +# or next statement to reach the previous source code line.
>>> +
>>> +# This testcase verifies the reverse-finish command stops at the
>>> first
>>> +# instruction in the source code line where the function was
>>> called.  There
>>> +# are two scenarios that must be checked:
>>> +#   1) gdb is at the entry point instruction for the function
>>> +#   2) gdb is in the body of the function.
>> While testing locally, I ran into a bug with reverse finish at the
>> epilogue of the function, that your patch also fixed. It would be
>> nice
>> if the test extended that. And since the bug was that GDB stopped
>> responding and even ctrl+c did nothing, I would suggest adding it as
>> the
>> last test.
> I haven't run into the issue that mentioned about GDB not responding in
> the epilogue.  I will have to go play with that to see if I can
> reproduce it.  If you have any specific instructions on how you ran
> into it that would be interesting and helpful.

I manually ran gdb.reverse/finish-precsave (setting recording, of 
course), set a temporary breakpoint on void_func, nexted once to be in 
the epilogue and tried to reverse finish. I'm not sure if it was some 
stale file before I recompiled the test, though.

Anyway, sorry if I was unclear, but that is not a regression of your 
patch. Rather, you patch fixed that issue, I just want that test to 
confirm that we don't accidentally regress it.
  

Bruno:

On Fri, 2023-01-13 at 18:04 +0100, Bruno Larsen wrote:
> > I haven't run into the issue that mentioned about GDB not
> > responding in
> > the epilogue.  I will have to go play with that to see if I can
> > reproduce it.  If you have any specific instructions on how you ran
> > into it that would be interesting and helpful.
> 
> I manually ran gdb.reverse/finish-precsave (setting recording, of 
> course), set a temporary breakpoint on void_func, nexted once to be
> in 
> the epilogue and tried to reverse finish. I'm not sure if it was
> some 
> stale file before I recompiled the test, though.
> 
> Anyway, sorry if I was unclear, but that is not a regression of your 
> patch. Rather, you patch fixed that issue, I just want that test to 
> confirm that we don't accidentally regress it.

You were clear that you saw this as an existing issue before applying
my patches, i.e. not a regression due to my patch.  But rather my patch
fixed an existing issue.

I have been playing around on my X86 box trying to reproduce the issue.

I have an X86 box running Ubuntu 22.04.1 LTS, gcc version Ubuntu
11.3.0-1ubuntu1~22.04.

Here is the log of what I did trying to reproduce the issue as you
described:

Note, I started by running 

 make check RUNTESTFLAGS='GDB=/home/carll/bin/gdb gdb.reverse/finish-precsave.exp'

to generate the binary for the test.  I then cd'd to 

~/GDB/build-finish-precsave/gdb/testsuite/outputs/gdb.reverse/finish-precsave

where the binary was saved.  

Then I ran the test.  The following is the results with the path names
to the files shortened to improve readability.

gdb ./finish-precsave
GNU gdb (GDB) 14.0.50.20230111-git

<snip>

Reading symbols from ./finish-precsave...
(gdb) break main
Breakpoint 1 at 0x11d3: file .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/finish-reverse.c, line 95.
(gdb) r
Starting program: .../gdb/testsuite/outputs/gdb.reverse/finish-precsave/finish-precsave 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, main (argc=1, argv=0x7fffffffe798)
    at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/finish-reverse.c:95
95	  for (i = 0; i < sizeof (testval.ffff); i++)
(gdb) record
(gdb) tb void_func
Temporary breakpoint 2 at 0x555555555131: file .../binutils-gdb-finishrecsave/gdb/testsuite/gdb.reverse/finish-reverse.c, line 44.
(gdb) c
Continuing.

Temporary breakpoint 2, void_func ()
    at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/finish-reverse.c:44
44	  void_test = 1;		/* VOID FUNC */
(gdb) next
45	}
(gdb) reverse-finish
Run back to call of #0  void_func ()
    at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/finish-reverse.c:45
0x00005555555551fd in main (argc=1, argv=0x7fffffffe798)
    at ../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/finish-reverse.c:98
98	  void_func (); 				/* call to void_func */
(gdb) reverse-step
98	  void_func (); 				/* call to void_func */
(gdb) q

I have tried stopping in the other functions as well.  The reverse-
finish still seems to work fine.  I also tried setting layout-asm once
I reached the function and then did si to reach various instructions in
the epilog.  Didn't seem to matter which instruction in the function I
was at when I issued the reverse-finish instruction, I was not able to
get gdb to hang.  Unfortunately, I was not able to reproduce the issue
on X86.  I have also tried reproducing the error on PowerPC without 
success.

I created a new test case to do the above tests.  My thought is if we
can get this new test case to reliably fail on your system without my
proposed patches, then we can add it to the new test case in the patch.
The test case passes on my X86 machine.  Of course it fails on my PPC
machine due to the existing reverse finish issues. What do you think?

                            Carl
-------------------------------------------
reverse-finish hang test

---
 .../gdb.reverse/gdb-reverse-finish-hang.exp   | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 gdb/testsuite/gdb.reverse/gdb-reverse-finish-hang.exp

diff --git a/gdb/testsuite/gdb.reverse/gdb-reverse-finish-hang.exp b/gdb/testsuite/gdb.reverse/gdb-reverse-finish-hang.exp
new file mode 100644
index 00000000000..14a3991c9b9
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/gdb-reverse-finish-hang.exp
@@ -0,0 +1,54 @@
+# Copyright 2008-2023 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/>.
+
+# This file is part of the GDB testsuite.  It tests 'finish' with
+# reverse debugging.
+
+if ![supports_reverse] {
+    return
+}
+
+standard_testfile finish-reverse.c
+set precsave [standard_output_file finish.precsave]
+
+if { [prepare_for_testing "failed to prepare" "$testfile" $srcfile] } {
+    return -1
+}
+
+runto_main
+
+if [supports_process_record] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "turn on process record"
+}
+
+# Test reverse finish from void func to see if gdb hangs.
+set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"]
+gdb_test "tbreak void_func" \
+    "Temporary breakpoint $decimal at .*$srcfile, line $breakloc\." \
+    "set breakpoint on void_func"
+gdb_continue_to_breakpoint "void_func" ".*$srcfile:$breakloc.*"
+
+# Step into epilogue of void_func.
+gdb_test "step" ".*}.*" "step to epilog"
+
+# Test to see if gdb hangs when doing a reverse-finish from the epilogue.
+gdb_test "reverse-finish" "$decimal.*void_func.*" \
+    "return to caller of void_func ()" 
+
+# Do reverse-next, should stay on void_func function due to existing bug
+# in reverse-finish.
+gdb_test "reverse-next" "$decimal.*void_func.*" \
+    "reverse next in main"
  

On Fri, 2023-01-13 at 14:33 +0100, Bruno Larsen wrote:
> On 11/01/2023 19:27, Carl Love via Gdb-patches wrote:
> > GDB maintainers:
> > 
> > This patch fixes the issues with the reverse-finish command on X86.
> > The reverse-finish command now correctly stops at the first
> > instruction
> > in the source code line of the caller.  It now only requires a
> > single
> > reverse-step or reverse-next instruction to get back to the
> > previous
> > source code line.
> > 
> > It also adds a new testcase, gdb.reverse/finish-reverse-next.exp,
> > and
> > updates several existing testcases.
> > 
> > Please let me know if you have any comments on the patch.  Thanks.
> Thanks for looking at this, this is a nice change. I just have a
> couple 
> of comments, mostly related to the testsuite side.
> >                      Carl
> > 
> > --------------------------------------------------------------
> > X86: reverse-finish fix
> > 
> > Currently on X86, when executing the finish command in reverse, gdb
> > does a
> > single step from the first instruction in the callee to get back to
> > the
> > caller.  GDB stops on the last instruction in the source code line
> > where
> > the call was made.  When stopped at the last instruction of the
> > source code
> > line, a reverse next or step command will stop at the first
> > instruction
> > of the same source code line thus requiring two step/next commands
> > to
> > reach the previous source code line.  It should only require one
> > step/next
> > command to reach the previous source code line.
> > 
> > By contrast, a reverse next or step command from the first line in
> > a
> > function stops at the first instruction in the source code line
> > where the
> > call was made.
> > 
> > This patch fixes the reverse finish command so it will stop at the
> > first
> > instruction of the source line where the function call was
> > made.  The
> > behavior on X86 for the reverse-finish command now matches doing a
> > reverse-next from the beginning of the function.
> > 
> > The proceed_to_finish flag in struct thread_control_state is no
> > longer
> > used.  This patch removes the declaration, initialization and
> > setting of
> > the flag.
> > 
> > This patch requires a number of regression tests to be
> > updated.  Test
> > gdb.mi/mi-reverse.exp no longer needs to execute two steps to get
> > to the
> > previous line.  The gdb output for tests gdb.reverse/until-
> > precsave.exp
> > and gdb.reverse/until-reverse.exp changed slightly.  The expected
> > result in
> > tests gdb.reverse/amd64-ailcall-reverse.exp and
> s/ailcall/tailcall

Fixed

> > gdb.reverse/singlejmp-reverse.exp are updated to the correct
> > expected
> > result.
> > 
> > This patch adds a new test gdb.reverse/finish-reverse-next.exp to
> > test the
> > reverse-finish command when returning from the entry point and from
> > the
> > body of the function.
> > 
> > The step_until proceedure in test gdb.reverse/step-indirect-call-
> > thunk.exp
> > was moved to lib/gdb.exp and renamed cmd_until.
> I'm not a big fan of the name cmd_until, because it sounded to me
> like 
> you were testing the GDB command until. I think repeat_cmd_until or 
> repeat_until would avoid this possible confusion.

Changed cmd_until to repeat_cmd_until.

> > The patch has been tested on X86 and PowerPC to verify no
> > additional
> > regression failures occured.
> > 
> > Bug: 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> >  
> If you add record/29927 somewhere along the text of your commit
> message, 
> there is some automation that will comment on the bugzilla bug 
> specifying this commit. Might be worth doing for future reference.

Added.  I realized I had forgotten to do that after I sent the email.  
I added it to both patches.

> > ---
> >   gdb/gdbthread.h                               |   4 -
> >   gdb/infcall.c                                 |   3 -
> >   gdb/infcmd.c                                  |  32 +++---
> >   gdb/infrun.c                                  |  41 +++----
> >   gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
> >   .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
> >   .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
> >   .../gdb.reverse/finish-reverse-next.exp       | 108
> > ++++++++++++++++++
> >   gdb/testsuite/gdb.reverse/finish-reverse.exp  |   5 +
> >   .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
> >   .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++------
> >   gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
> >   gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
> >   gdb/testsuite/lib/gdb.exp                     |  33 ++++++
> >   14 files changed, 240 insertions(+), 106 deletions(-)
> >   create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-
> > next.c
> >   create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-
> > next.exp
> > 
> > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> > index 11d69fceab0..e4edff2d621 100644
> > --- a/gdb/gdbthread.h
> > +++ b/gdb/gdbthread.h
> > @@ -150,10 +150,6 @@ struct thread_control_state
> >        the finished single step.  */
> >     int trap_expected = 0;
> >   
> > -  /* Nonzero if the thread is being proceeded for a "finish"
> > command
> > -     or a similar situation when return value should be
> > printed.  */
> > -  int proceed_to_finish = 0;
> > -
> >     /* Nonzero if the thread is being proceeded for an inferior
> > function
> >        call.  */
> >     int in_infcall = 0;
> > diff --git a/gdb/infcall.c b/gdb/infcall.c
> > index e09904f9a35..116605c43ef 100644
> > --- a/gdb/infcall.c
> > +++ b/gdb/infcall.c
> > @@ -625,9 +625,6 @@ run_inferior_call
> > (std::unique_ptr<call_thread_fsm> sm,
> >   
> >     disable_watchpoints_before_interactive_call_start ();
> >   
> > -  /* We want to print return value, please...  */
> > -  call_thread->control.proceed_to_finish = 1;
> > -
> >     try
> >       {
> >         /* Infcalls run synchronously, in the foreground.  */
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 0497ad05091..9c42efeae8d 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm
> > *sm)
> >   
> >     sal = find_pc_line (func_addr, 0);
> >   
> > -  tp->control.proceed_to_finish = 1;
> > -  /* Special case: if we're sitting at the function entry point,
> > -     then all we need to do is take a reverse singlestep.  We
> > -     don't need to set a breakpoint, and indeed it would do us
> > -     no good to do so.
> > -
> > -     Note that this can only happen at frame #0, since there's
> > -     no way that a function up the stack can have a return address
> > -     that's equal to its entry point.  */
> > +  frame_info_ptr frame = get_selected_frame (nullptr);
> >   
> >     if (sal.pc != pc)
> >       {
> > -      frame_info_ptr frame = get_selected_frame (nullptr);
> >         struct gdbarch *gdbarch = get_frame_arch (frame);
> >   
> >         /* Set a step-resume at the function's entry point.  Once
> > that's
> > @@ -1743,16 +1734,22 @@ finish_backward (struct finish_command_fsm
> > *sm)
> >         sr_sal.pspace = get_frame_program_space (frame);
> >         insert_step_resume_breakpoint_at_sal (gdbarch,
> >   					    sr_sal, null_frame_id);
> > -
> > -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> >       }
> >     else
> >       {
> > -      /* We're almost there -- we just need to back up by one more
> > -	 single-step.  */
> > -      tp->control.step_range_start = tp->control.step_range_end =
> > 1;
> > -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> > +      /* We are exactly at the function entry point.  Note that
> > this
> > +	 can only happen at frame #0.
> > +
> > +	 When setting a step range, need to call set_step_info
> > +	 to setup the current_line/symtab fields as well.  */
> > +      set_step_info (tp, frame, find_pc_line (pc, 0));
> > +
> > +      /* Return using a step range so we will keep stepping back
> > +	 to the first instruction in the source code line.  */
> > +      tp->control.step_range_start = sal.pc;
> > +      tp->control.step_range_end = sal.pc;
> >       }
> > +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> >   }
> >   
> >   /* finish_forward -- helper function for finish_command.  FRAME
> > is the
> > @@ -1778,9 +1775,6 @@ finish_forward (struct finish_command_fsm
> > *sm, frame_info_ptr frame)
> >   
> >     set_longjmp_breakpoint (tp, frame_id);
> >   
> > -  /* We want to print return value, please...  */
> > -  tp->control.proceed_to_finish = 1;
> > -
> >     proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> >   }
> >   
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 181d961d80d..8ed538ea9ec 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct
> > thread_info *tp)
> >   
> >     tp->control.stop_step = 0;
> >   
> > -  tp->control.proceed_to_finish = 0;
> > -
> >     tp->control.stepping_command = 0;
> >   
> >     /* Discard any remaining commands or status from previous
> > stop.  */
> > @@ -6737,31 +6735,28 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >   
> >       case BPSTAT_WHAT_STEP_RESUME:
> >         infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
> > -
> >         delete_step_resume_breakpoint (ecs->event_thread);
> > -      if (ecs->event_thread->control.proceed_to_finish
> > -	  && execution_direction == EXEC_REVERSE)
> > +      fill_in_stop_func (gdbarch, ecs);
> > +
> > +      if (execution_direction == EXEC_REVERSE
> > +	  && ecs->event_thread->stop_pc () == ecs->stop_func_start)
> Is there any reason to invert the order of checks here? The second
> if 
> clause is the same and keeping that would make the changes easier to
> parse.

No, must have inadvertently swizzled it as part of the patch
development.  Per comments for the second patch, PowerPC, the  "cs-
>event_thread->stop_pc () == ecs->stop_func_start" check should be
removed in this patch not the PowerPC patch.  Probably got missed when
I switched the order of the patches.

 Fixed, removed the "ecs->event_thread->stop_pc () == ecs-
>stop_func_start" test here.

> >   	{
> >   	  struct thread_info *tp = ecs->event_thread;
> > +	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (),
> > 0);
> >   
> > -	  /* We are finishing a function in reverse, and just hit the
> > -	     step-resume breakpoint at the start address of the
> > -	     function, and we're almost there -- just need to back up
> > -	     by one more single-step, which should take us back to the
> > -	     function call.  */
> > -	  tp->control.step_range_start = tp->control.step_range_end =
> > 1;
> > -	  keep_going (ecs);
> > -	  return;
> > -	}
> > -      fill_in_stop_func (gdbarch, ecs);
> > -      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
> > -	  && 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
> > -	     the function.  Go back to single-stepping, which should
> > -	     take us back to the function call.  */
> > -	  ecs->event_thread->stepping_over_breakpoint = 1;

The following comment was from the second email.

 >      case BPSTAT_WHAT_STEP_RESUME:> Something else that I failed to
notice. Since you removed both
> comments 
> that mention that this case is here for reverse finishing, there is
> no 
> good way to figure out what GDB wants to do when this part of the
> code 
> is reached. Adding a comment here mentioning it would fix that.
> >          infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");

There were two separate if statements, each with a comment about what
they were for.  Those comments were removed and a new, similar, comment
was added in the single new if statement.  Admittedly, the new comment
is a bit farther into the function and thus easy to miss.  So, I moved
the initial comment about what is going on "We are finishing a function
in reverse or..." up to the beginning of the if statement.  Hopefully
that helps make it quicker/easier for the reader to see what the
purpose of the case statement/if statement.  Please let me know if that
helps address your concerns. 

> > +	  /* When setting a step range, need to call set_step_info
> > +	     to setup the current_line/symtab fields as well.  */
> > +	  set_step_info (tp, frame, stop_pc_sal);
> > +
> > +	  /* We are finishing a function in reverse or stepping over a
> > function
> > +	     call in reverse, and just hit the step-resume breakpoint
> > at the
> > +	     start address of the function, and we're almost there --
> > just need
> > +	     to back up to the function call.
> > +
> > +	     Return using a step range so we will keep stepping back to
> > the
> > +	     first instruction in the source code line.  */
> > +	  tp->control.step_range_start = ecs->stop_func_start;
> > +	  tp->control.step_range_end = ecs->stop_func_start;
> >   	  keep_going (ecs);
> >   	  return;
> >   	}
> > diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp
> > b/gdb/testsuite/gdb.mi/mi-reverse.exp
> > index d631beb17c8..30635ab1754 100644
> > --- a/gdb/testsuite/gdb.mi/mi-reverse.exp
> > +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
> > @@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} {
> >   	"basics.c" $line_main_callme_1 "" \
> >   	"reverse finish from callme"
> >   
> > -    # Test exec-reverse-next
> > -    #   It takes two steps to get back to the previous line,
> > -    #   as the first step moves us to the start of the current
> > line,
> > -    #   and the one after that moves back to the previous line.
> > -
> > -    mi_execute_to "exec-next --reverse 2" \
> > +    mi_execute_to "exec-next --reverse" \
> >    	"end-stepping-range" "main" "" \
> >    	"basics.c" $line_main_hello "" \
> > - 	"reverse next to get over the call to do_nothing"
> > +	"reverse next to get over the call to do_nothing"
> >   
> >       # Test exec-reverse-step
> >   
> > diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> > b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> > index 52a87faabf7..9964b4f8e4b 100644
> > --- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> > +++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> > @@ -44,6 +44,5 @@ if [supports_process_record] {
> >   gdb_test "next" {f \(\);} "next to f"
> >   gdb_test "next" {v = 3;} "next to v = 3"
> >   
> > -# FAIL was:
> > -# 29        g ();
> > -gdb_test "reverse-next" {f \(\);}
> > +# Reverse step back into f ().  Puts us at call to g () in
> > function f ().
> > +gdb_test "reverse-next" {g \(\);}
> > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > new file mode 100644
> > index 00000000000..42e41b5a2e0
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > @@ -0,0 +1,48 @@
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > +   Copyright 2012-2022 Free Software Foundation, Inc.
> copyright year should be 2023.
> > +
> > +   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/
> >  >.  */
> > +
> > +/* The reverse finish command should return from a function and
> > stop on
> > +   the first instruction of the source line where the function
> > call is made.
> > +   Specifically, the behavior should match doing a reverse next
> > from the
> > +   first instruction in the function.  GDB should only require one
> > reverse
> > +   step or next statement to reach the previous source code line.
> > +
> > +   This test verifies the fix for gdb bugzilla:
> > +
> > +   
> > https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> >  
> > +*/
> > +
> > +int
> > +function1 (int a, int b)   // FUNCTION1
> > +{
> > +  int ret = 0;
> > +
> > +  ret = a + b;
> > +  return ret;
> > +}
> > +
> > +int
> > +main(int argc, char* argv[])
> > +{
> > +  int a, b;
> > +
> > +  a = 1;
> > +  b = 5;
> > +
> > +  function1 (a, b);   // CALL FUNCTION
> > +  return 0;
> > +}
> > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > new file mode 100644
> > index 00000000000..7880de10ffc
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > @@ -0,0 +1,108 @@
> > +# Copyright 2008-2022 Free Software Foundation, Inc.

Fixed copyright so it reads 2008-2023.  Fixed in finish-reverse-
next.exp and finish-reverse-next.c.

> > +
> > +# 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/
> >  >.  */
> > +
> > +# This file is part of the GDB testsuite.  It tests reverse
> > stepping.
> > +# Lots of code borrowed from "step-test.exp".
> > +
> > +# The reverse finish command should return from a function and
> > stop on
> > +# the first instruction of the source line where the function call
> > is made.
> > +# Specifically, the behavior should match doing a reverse next
> > from the
> > +# first instruction in the function.  GDB should only take one
> > reverse step
> > +# or next statement to reach the previous source code line.
> > +
> > +# This testcase verifies the reverse-finish command stops at the
> > first
> > +# instruction in the source code line where the function was
> > called.  There
> > +# are two scenarios that must be checked:
> > +#   1) gdb is at the entry point instruction for the function
> > +#   2) gdb is in the body of the function.
> 
> While testing locally, I ran into a bug with reverse finish at the 
> epilogue of the function, that your patch also fixed. It would be
> nice 
> if the test extended that. And since the bug was that GDB stopped 
> responding and even ctrl+c did nothing, I would suggest adding it as
> the 
> last test.

Discussed this additional bug in earlier emails.  Waiting to hear if
the new test I sent reliably exposes the gdb hang that Bruno reported. 
If it does, I will add the new test to the new test case before posting
the updated patch set. Per the discussions, I have not been able to
reproduce the issue on my X86 or PowerPC machines.

> 
> > +
> > +# This test verifies the fix for gdb bugzilla:
> > +#   
> > https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> >  
> > +
> > +if ![supports_reverse] {
> > +    return
> > +}
> > +
> > +standard_testfile
> > +
> > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile]
> > } {
> > +    return -1
> > +}
> > +
> > +runto_main
> > +set target_remote [gdb_is_target_remote]
> > +
> > +if [supports_process_record] {
> > +    # Activate process record/replay.
> > +    gdb_test_no_output "record" "turn on process record for test1"
> > +}
> > +
> > +
> > +### TEST 1: reverse finish from the entry point instruction in
> > +### function1.
> > +
> > +# Set breakpoint at call to function1 in main.
> > +set FUNCTION_test  [gdb_get_line_number "CALL FUNCTION" $srcfile]
> > +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at
> > .*" \his
> > +    "set breakpoint on function1 call to stepi into function"
> 
> There is a proc in lib/gdb.exp called gdb_breakpoint which
> couldsimplify 
> this gdb_test to
> 
> gdb_breakpoint $srcfile:$FUNCTION_test temporary
> 
> And would remove the need for the delete_breakpoints call later.
> 

OK, made the change in both tests.  Made the same change in the PowerPC
patch that adds additional tests.


> > +
> > +# Continue to break point at function1 call in main.
> > +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*"
> > \
> > +    "stopped at function1 entry point instruction to stepi into
> > function"
> You can use gdb_continue_to_breakpoint here instead.

OK, made the change in both tests.  Made the same change in the PowerPC
patch that adds additional tests.

> > +
> > +# stepi until we see "{" indicating we entered function1
> > +cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
> > +
> > +delete_breakpoints
> > +
> > +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL
> > FUNCTION.*" \
> > +    "reverse-finish function1 "
> > +
> > +# Check to make sure we stopped at the first instruction in the
> > source code
> > +# line.  It should only take one reverse next command to get to
> > the previous
> > +# source line.   If GDB stops at the last instruction in the
> > source code line
> > +# it will take two reverse next instructions to get to the
> > previous source
> > +# line.
> > +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call
> > from function"
> > +
> > +# Clear the recorded log.
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test1"
> > +gdb_test_no_output "record" "turn on process record for test2"
> > +
> > +
> > +### TEST 2: reverse finish from the body of function1.
> > +
> > +# Set breakpoint at call to function1 in main.
> > +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at
> > .*" \
> > +    "set breakpoint on function1 call to step into body of
> > function"
> > +
> > +# Continue to break point at function1 call in main.
> > +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*"
> > \
> > +    "stopped at function1 entry point instruction to step to body
> > of function"
> > +
> > +delete_breakpoints
> > +
> > +# do a step instruction to get to the body of the function
> > +gdb_test "step" ".*int ret = 0;.*" "step test 1"
> > +
> > +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL
> > FUNCTION.*" \
> > +    "reverse-finish function1 call from function body"
> > +
> > +# Check to make sure we stopped at the first instruction in the
> > source code
> > +# line.  It should only take one reverse next command to get to
> > the previous
> > +# source line.
> > +gdb_test "reverse-next" ".*b = 5;.*" \
> > +    "reverse next at b = 5, from function body"
> > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse.exp
> > b/gdb/testsuite/gdb.reverse/finish-reverse.exp
> > index 01ba309420c..a05cb81892a 100644
> > --- a/gdb/testsuite/gdb.reverse/finish-reverse.exp
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse.exp
> > @@ -16,6 +16,11 @@
> >   # This file is part of the GDB testsuite.  It tests 'finish' with
> >   # reverse debugging.
> >   
> > +# This test verifies the fix for gdb bugzilla:
> > +
> > +# 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> >  
> > +
> > +
> 
> Is this comment a left over from an earlier version?
> 
> I actually wonder if the whole new test is needed, or if you can
> just 
> add a couple of new tests to finish-reverse.exp; is there any reason
> you 
> went with the new test instead

Yes, it is left over.  Initially I just added an additional test to
finish-reverse.exp for the PowerPC fix.  But as work progressed, I kept
adding more tests for PowerPC then for X86.  I felt that it was better
to have a new test file that was tied to the Bugzilla.  The existing
test file has a different focus from the new tests.  The bugzilla
change didn't get removed from finish-reverse.exp when the tests were
moved to the new file.  We can combine the tests again if that is
preferable?   My preference would be to have separate test files. 
Please let me know if you would prefer a single file and I will merge
them before re-posting the patches. 

> 
> >   if ![supports_reverse] {
> >       return
> >   }
> > diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> > b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> > index 1ca7c2ce559..eb03051625a 100644
> > --- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> > +++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> > @@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3"
> >   # {
> >   gdb_test "reverse-step" {nodebug \(\);}
> >   
> > -# FAIL was:
> > -# No more reverse-execution history.
> > -# {
> > -gdb_test "reverse-next" {f \(\);}
> > +gdb_test "reverse-next" {g \(\);}
> > diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp 
> > b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > index ad637899e5b..1928cdda217 100644
> > --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > @@ -39,39 +39,6 @@ if { ![runto_main] } {
> >       return -1
> >   }
> >   
> > -# Do repeated stepping COMMANDs in order to reach TARGET from
> > CURRENT
> > -#
> > -#  COMMAND is a stepping command
> > -#  CURRENT is a string matching the current location
> > -#  TARGET  is a string matching the target location
> > -#  TEST    is the test name
> > -#
> > -# The function issues repeated COMMANDs as long as the location
> > matches
> > -# CURRENT up to a maximum of 100 steps.
> > -#
> > -# TEST passes if the resulting location matches TARGET and fails
> > -# otherwise.
> > -#
> > -proc step_until { command current target test } {
> > -    global gdb_prompt
> > -
> > -    set count 0
> > -    gdb_test_multiple "$command" "$test" {
> > -        -re "$current.*$gdb_prompt $" {
> > -            incr count
> > -            if { $count < 100 } {
> > -                send_gdb "$command\n"
> > -                exp_continue
> > -            } else {
> > -                fail "$test"
> > -            }
> > -        }
> > -        -re "$target.*$gdb_prompt $" {
> > -            pass "$test"
> > -        }
> > -    }
> > -}
> > -
> >   gdb_test_no_output "record"
> >   gdb_test "next" ".*" "record trace"
> >   
> > @@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \
> >       "reverse-step through thunks and over inc"
> >   
> >   # We can use instruction stepping to step into thunks.
> > -step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call
> > thunk"
> > -step_until "stepi" "indirect_thunk" "inc" \
> > +cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call
> > thunk"
> > +cmd_until "stepi" "indirect_thunk" "inc" \
> >       "stepi out of call thunk into inc"
> >   set alphanum_re "\[a-zA-Z0-9\]"
> >   set pic_thunk_re  "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re*
> > \\(\\)"
> > -step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi
> > into return thunk"
> > -step_until "stepi" "return_thunk" "apply" \
> > +cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into
> > return thunk"
> > +cmd_until "stepi" "return_thunk" "apply" \
> >       "stepi out of return thunk back into apply"
> >   
> > -step_until "reverse-stepi" "apply" "return_thunk" \
> > +cmd_until "reverse-stepi" "apply" "return_thunk" \
> >       "reverse-stepi into return thunk"
> > -step_until "reverse-stepi" "return_thunk" "inc" \
> > +cmd_until "reverse-stepi" "return_thunk" "inc" \
> >       "reverse-stepi out of return thunk into inc"
> > -step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk"
> > \
> > +cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
> >       "reverse-stepi into call thunk"
> > -step_until "reverse-stepi" "indirect_thunk" "apply" \
> > +cmd_until "reverse-stepi" "indirect_thunk" "apply" \
> >       "reverse-stepi out of call thunk into apply"
> > diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp
> > b/gdb/testsuite/gdb.reverse/until-precsave.exp
> > index 0c2d7537cd6..777aec94ac1 100644
> > --- a/gdb/testsuite/gdb.reverse/until-precsave.exp
> > +++ b/gdb/testsuite/gdb.reverse/until-precsave.exp
> > @@ -142,7 +142,7 @@ gdb_test "advance marker2" \
> >   # Finish out to main scope (backward)
> >   
> >   gdb_test "finish" \
> > -    " in main .*$srcfile:$bp_location20.*" \
> > +    "main .*$srcfile:$bp_location20.*" \
> This change doesn't seem connected to anything in this patch, is
> this 
> just a cosmetic change or was there some problem?
> >       "reverse-finish from marker2"
> >   

The output changes due to the functional changes in infrun.c.  Instead
of stepping back one instruction i.e. ecs->event_thread-
>stepping_over_breakpoint = 1 we step back using a range.  Apparently
this causes the gdb output message to change. 

Without the patch the output looks like:
  Run back to call of #0  marker2 (a=43) at.../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/ur1.c:30

  0x0000000010000838 in main (argc=1, argv=0x7fffffffcc58, envp=0x7fffffffcc68) at /.../gdb/testsuite/gdb.reverse/until-reverse.c:48^

With the patch the output looks like:

  Run back to call of #0  marker2 (a=43) at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/ur1.c:30

  main (argc=1, argv=0x7fffffffcc58, envp=0x7fffffffcc68) at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/until-reverse.c:48

Basically, you lose the hex value and "in" with the patch applied. 
This is true in the until-reverse.exp tes, below, as well.  The output
change was mentioned in the commit message as well.


> >   # Advance backward to last line of factorial (outer invocation)
> > diff --git a/gdb/testsuite/gdb.reverse/until-reverse.exp
> > b/gdb/testsuite/gdb.reverse/until-reverse.exp
> > index 23fc881dbf2..3a05953329f 100644
> > --- a/gdb/testsuite/gdb.reverse/until-reverse.exp
> > +++ b/gdb/testsuite/gdb.reverse/until-reverse.exp
> > @@ -113,7 +113,7 @@ gdb_test "advance marker2" \
> >   # Finish out to main scope (backward)
> >   
> >   gdb_test "finish" \
> > -    " in main .*$srcfile:$bp_location20.*" \
> > +    "main .*$srcfile:$bp_location20.*" \
> same here.

Yup, see above.

> >       "reverse-finish from marker2"
> >   
> >   # Advance backward to last line of factorial (outer invocation)
> > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> > index c41d4698d66..25f42eb5510 100644
> > --- a/gdb/testsuite/lib/gdb.exp
> > +++ b/gdb/testsuite/lib/gdb.exp
> > @@ -9301,6 +9301,39 @@ proc gdb_step_until { regexp {test_name ""}
> > {max_steps 10} } {
> >       }
> >   }
> >   
> > +# Do repeated stepping COMMANDs in order to reach TARGET from
> > CURRENT
> > +#
> > +#  COMMAND is a stepping command
> > +#  CURRENT is a string matching the current location
> > +#  TARGET  is a string matching the target location
> > +#  TEST    is the test name
> > +#
> > +# The function issues repeated COMMANDs as long as the location
> > matches
> > +# CURRENT up to a maximum of 100 steps.
> > +#
> > +# TEST passes if the resulting location matches TARGET and fails
> > +# otherwise.
> > +
> > +proc cmd_until { command current target test } {
> > +    global gdb_prompt
> > +
> > +    set count 0
> > +    gdb_test_multiple "$command" "$test" {
> > +	-re "$current.*$gdb_prompt $" {
> > +	    incr count
> > +	    if { $count < 100 } {
> > +		send_gdb "$command\n"
> > +		exp_continue
> > +	    } else {
> > +		fail "$test"
> > +	    }
> > +	}
> > +	-re "$target.*$gdb_prompt $" {
> > +	    pass "$test"
> > +	}
> > +    }
> > +}
> > +
> >   # Check if the compiler emits epilogue information associated
> >   # with the closing brace or with the last statement line.
> >   #
> 
>
  

On 14/01/2023 19:08, Carl Love wrote:
> On Fri, 2023-01-13 at 14:33 +0100, Bruno Larsen wrote:
>> On 11/01/2023 19:27, Carl Love via Gdb-patches wrote:
>>> GDB maintainers:
>>>
>>> This patch fixes the issues with the reverse-finish command on X86.
>>> The reverse-finish command now correctly stops at the first
>>> instruction
>>> in the source code line of the caller.  It now only requires a
>>> single
>>> reverse-step or reverse-next instruction to get back to the
>>> previous
>>> source code line.
>>>
>>> It also adds a new testcase, gdb.reverse/finish-reverse-next.exp,
>>> and
>>> updates several existing testcases.
>>>
>>> Please let me know if you have any comments on the patch.  Thanks.
>> Thanks for looking at this, this is a nice change. I just have a
>> couple
>> of comments, mostly related to the testsuite side.
>>>                       Carl
>>>
>>> --------------------------------------------------------------
>>> X86: reverse-finish fix
>>>
>>> Currently on X86, when executing the finish command in reverse, gdb
>>> does a
>>> single step from the first instruction in the callee to get back to
>>> the
>>> caller.  GDB stops on the last instruction in the source code line
>>> where
>>> the call was made.  When stopped at the last instruction of the
>>> source code
>>> line, a reverse next or step command will stop at the first
>>> instruction
>>> of the same source code line thus requiring two step/next commands
>>> to
>>> reach the previous source code line.  It should only require one
>>> step/next
>>> command to reach the previous source code line.
>>>
>>> By contrast, a reverse next or step command from the first line in
>>> a
>>> function stops at the first instruction in the source code line
>>> where the
>>> call was made.
>>>
>>> This patch fixes the reverse finish command so it will stop at the
>>> first
>>> instruction of the source line where the function call was
>>> made.  The
>>> behavior on X86 for the reverse-finish command now matches doing a
>>> reverse-next from the beginning of the function.
>>>
>>> The proceed_to_finish flag in struct thread_control_state is no
>>> longer
>>> used.  This patch removes the declaration, initialization and
>>> setting of
>>> the flag.
>>>
>>> This patch requires a number of regression tests to be
>>> updated.  Test
>>> gdb.mi/mi-reverse.exp no longer needs to execute two steps to get
>>> to the
>>> previous line.  The gdb output for tests gdb.reverse/until-
>>> precsave.exp
>>> and gdb.reverse/until-reverse.exp changed slightly.  The expected
>>> result in
>>> tests gdb.reverse/amd64-ailcall-reverse.exp and
>> s/ailcall/tailcall
> Fixed
>
>>> gdb.reverse/singlejmp-reverse.exp are updated to the correct
>>> expected
>>> result.
>>>
>>> This patch adds a new test gdb.reverse/finish-reverse-next.exp to
>>> test the
>>> reverse-finish command when returning from the entry point and from
>>> the
>>> body of the function.
>>>
>>> The step_until proceedure in test gdb.reverse/step-indirect-call-
>>> thunk.exp
>>> was moved to lib/gdb.exp and renamed cmd_until.
>> I'm not a big fan of the name cmd_until, because it sounded to me
>> like
>> you were testing the GDB command until. I think repeat_cmd_until or
>> repeat_until would avoid this possible confusion.
> Changed cmd_until to repeat_cmd_until.
>
>>> The patch has been tested on X86 and PowerPC to verify no
>>> additional
>>> regression failures occured.
>>>
>>> Bug:
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927
>>>   
>> If you add record/29927 somewhere along the text of your commit
>> message,
>> there is some automation that will comment on the bugzilla bug
>> specifying this commit. Might be worth doing for future reference.
> Added.  I realized I had forgotten to do that after I sent the email.
> I added it to both patches.
>
>>> ---
>>>    gdb/gdbthread.h                               |   4 -
>>>    gdb/infcall.c                                 |   3 -
>>>    gdb/infcmd.c                                  |  32 +++---
>>>    gdb/infrun.c                                  |  41 +++----
>>>    gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
>>>    .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
>>>    .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
>>>    .../gdb.reverse/finish-reverse-next.exp       | 108
>>> ++++++++++++++++++
>>>    gdb/testsuite/gdb.reverse/finish-reverse.exp  |   5 +
>>>    .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
>>>    .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++------
>>>    gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
>>>    gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
>>>    gdb/testsuite/lib/gdb.exp                     |  33 ++++++
>>>    14 files changed, 240 insertions(+), 106 deletions(-)
>>>    create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-
>>> next.c
>>>    create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-
>>> next.exp
>>>
>>> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
>>> index 11d69fceab0..e4edff2d621 100644
>>> --- a/gdb/gdbthread.h
>>> +++ b/gdb/gdbthread.h
>>> @@ -150,10 +150,6 @@ struct thread_control_state
>>>         the finished single step.  */
>>>      int trap_expected = 0;
>>>    
>>> -  /* Nonzero if the thread is being proceeded for a "finish"
>>> command
>>> -     or a similar situation when return value should be
>>> printed.  */
>>> -  int proceed_to_finish = 0;
>>> -
>>>      /* Nonzero if the thread is being proceeded for an inferior
>>> function
>>>         call.  */
>>>      int in_infcall = 0;
>>> diff --git a/gdb/infcall.c b/gdb/infcall.c
>>> index e09904f9a35..116605c43ef 100644
>>> --- a/gdb/infcall.c
>>> +++ b/gdb/infcall.c
>>> @@ -625,9 +625,6 @@ run_inferior_call
>>> (std::unique_ptr<call_thread_fsm> sm,
>>>    
>>>      disable_watchpoints_before_interactive_call_start ();
>>>    
>>> -  /* We want to print return value, please...  */
>>> -  call_thread->control.proceed_to_finish = 1;
>>> -
>>>      try
>>>        {
>>>          /* Infcalls run synchronously, in the foreground.  */
>>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>>> index 0497ad05091..9c42efeae8d 100644
>>> --- a/gdb/infcmd.c
>>> +++ b/gdb/infcmd.c
>>> @@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm
>>> *sm)
>>>    
>>>      sal = find_pc_line (func_addr, 0);
>>>    
>>> -  tp->control.proceed_to_finish = 1;
>>> -  /* Special case: if we're sitting at the function entry point,
>>> -     then all we need to do is take a reverse singlestep.  We
>>> -     don't need to set a breakpoint, and indeed it would do us
>>> -     no good to do so.
>>> -
>>> -     Note that this can only happen at frame #0, since there's
>>> -     no way that a function up the stack can have a return address
>>> -     that's equal to its entry point.  */
>>> +  frame_info_ptr frame = get_selected_frame (nullptr);
>>>    
>>>      if (sal.pc != pc)
>>>        {
>>> -      frame_info_ptr frame = get_selected_frame (nullptr);
>>>          struct gdbarch *gdbarch = get_frame_arch (frame);
>>>    
>>>          /* Set a step-resume at the function's entry point.  Once
>>> that's
>>> @@ -1743,16 +1734,22 @@ finish_backward (struct finish_command_fsm
>>> *sm)
>>>          sr_sal.pspace = get_frame_program_space (frame);
>>>          insert_step_resume_breakpoint_at_sal (gdbarch,
>>>    					    sr_sal, null_frame_id);
>>> -
>>> -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>>>        }
>>>      else
>>>        {
>>> -      /* We're almost there -- we just need to back up by one more
>>> -	 single-step.  */
>>> -      tp->control.step_range_start = tp->control.step_range_end =
>>> 1;
>>> -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>>> +      /* We are exactly at the function entry point.  Note that
>>> this
>>> +	 can only happen at frame #0.
>>> +
>>> +	 When setting a step range, need to call set_step_info
>>> +	 to setup the current_line/symtab fields as well.  */
>>> +      set_step_info (tp, frame, find_pc_line (pc, 0));
>>> +
>>> +      /* Return using a step range so we will keep stepping back
>>> +	 to the first instruction in the source code line.  */
>>> +      tp->control.step_range_start = sal.pc;
>>> +      tp->control.step_range_end = sal.pc;
>>>        }
>>> +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>>>    }
>>>    
>>>    /* finish_forward -- helper function for finish_command.  FRAME
>>> is the
>>> @@ -1778,9 +1775,6 @@ finish_forward (struct finish_command_fsm
>>> *sm, frame_info_ptr frame)
>>>    
>>>      set_longjmp_breakpoint (tp, frame_id);
>>>    
>>> -  /* We want to print return value, please...  */
>>> -  tp->control.proceed_to_finish = 1;
>>> -
>>>      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>>>    }
>>>    
>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>> index 181d961d80d..8ed538ea9ec 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct
>>> thread_info *tp)
>>>    
>>>      tp->control.stop_step = 0;
>>>    
>>> -  tp->control.proceed_to_finish = 0;
>>> -
>>>      tp->control.stepping_command = 0;
>>>    
>>>      /* Discard any remaining commands or status from previous
>>> stop.  */
>>> @@ -6737,31 +6735,28 @@ process_event_stop_test (struct
>>> execution_control_state *ecs)
>>>    
>>>        case BPSTAT_WHAT_STEP_RESUME:
>>>          infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>>> -
>>>          delete_step_resume_breakpoint (ecs->event_thread);
>>> -      if (ecs->event_thread->control.proceed_to_finish
>>> -	  && execution_direction == EXEC_REVERSE)
>>> +      fill_in_stop_func (gdbarch, ecs);
>>> +
>>> +      if (execution_direction == EXEC_REVERSE
>>> +	  && ecs->event_thread->stop_pc () == ecs->stop_func_start)
>> Is there any reason to invert the order of checks here? The second
>> if
>> clause is the same and keeping that would make the changes easier to
>> parse.
> No, must have inadvertently swizzled it as part of the patch
> development.  Per comments for the second patch, PowerPC, the  "cs-
>> event_thread->stop_pc () == ecs->stop_func_start" check should be
> removed in this patch not the PowerPC patch.  Probably got missed when
> I switched the order of the patches.
>
>   Fixed, removed the "ecs->event_thread->stop_pc () == ecs-
>> stop_func_start" test here.
>>>    	{
>>>    	  struct thread_info *tp = ecs->event_thread;
>>> +	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (),
>>> 0);
>>>    
>>> -	  /* We are finishing a function in reverse, and just hit the
>>> -	     step-resume breakpoint at the start address of the
>>> -	     function, and we're almost there -- just need to back up
>>> -	     by one more single-step, which should take us back to the
>>> -	     function call.  */
>>> -	  tp->control.step_range_start = tp->control.step_range_end =
>>> 1;
>>> -	  keep_going (ecs);
>>> -	  return;
>>> -	}
>>> -      fill_in_stop_func (gdbarch, ecs);
>>> -      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
>>> -	  && 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
>>> -	     the function.  Go back to single-stepping, which should
>>> -	     take us back to the function call.  */
>>> -	  ecs->event_thread->stepping_over_breakpoint = 1;
> The following comment was from the second email.
>
>   >      case BPSTAT_WHAT_STEP_RESUME:> Something else that I failed to
> notice. Since you removed both
>> comments
>> that mention that this case is here for reverse finishing, there is
>> no
>> good way to figure out what GDB wants to do when this part of the
>> code
>> is reached. Adding a comment here mentioning it would fix that.
>>>           infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
> There were two separate if statements, each with a comment about what
> they were for.  Those comments were removed and a new, similar, comment
> was added in the single new if statement.  Admittedly, the new comment
> is a bit farther into the function and thus easy to miss.  So, I moved
> the initial comment about what is going on "We are finishing a function
> in reverse or..." up to the beginning of the if statement.  Hopefully
> that helps make it quicker/easier for the reader to see what the
> purpose of the case statement/if statement.  Please let me know if that
> helps address your concerns.
Yeah, this works. It is mostly so that we don't end up with a comment 
kinda far away or in a situation where it's hard to understand the point 
of this case statement.
>
>>> +	  /* When setting a step range, need to call set_step_info
>>> +	     to setup the current_line/symtab fields as well.  */
>>> +	  set_step_info (tp, frame, stop_pc_sal);
>>> +
>>> +	  /* We are finishing a function in reverse or stepping over a
>>> function
>>> +	     call in reverse, and just hit the step-resume breakpoint
>>> at the
>>> +	     start address of the function, and we're almost there --
>>> just need
>>> +	     to back up to the function call.
>>> +
>>> +	     Return using a step range so we will keep stepping back to
>>> the
>>> +	     first instruction in the source code line.  */
>>> +	  tp->control.step_range_start = ecs->stop_func_start;
>>> +	  tp->control.step_range_end = ecs->stop_func_start;
>>>    	  keep_going (ecs);
>>>    	  return;
>>>    	}
>>> diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp
>>> b/gdb/testsuite/gdb.mi/mi-reverse.exp
>>> index d631beb17c8..30635ab1754 100644
>>> --- a/gdb/testsuite/gdb.mi/mi-reverse.exp
>>> +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
>>> @@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} {
>>>    	"basics.c" $line_main_callme_1 "" \
>>>    	"reverse finish from callme"
>>>    
>>> -    # Test exec-reverse-next
>>> -    #   It takes two steps to get back to the previous line,
>>> -    #   as the first step moves us to the start of the current
>>> line,
>>> -    #   and the one after that moves back to the previous line.
>>> -
>>> -    mi_execute_to "exec-next --reverse 2" \
>>> +    mi_execute_to "exec-next --reverse" \
>>>     	"end-stepping-range" "main" "" \
>>>     	"basics.c" $line_main_hello "" \
>>> - 	"reverse next to get over the call to do_nothing"
>>> +	"reverse next to get over the call to do_nothing"
>>>    
>>>        # Test exec-reverse-step
>>>    
>>> diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
>>> b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
>>> index 52a87faabf7..9964b4f8e4b 100644
>>> --- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
>>> +++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
>>> @@ -44,6 +44,5 @@ if [supports_process_record] {
>>>    gdb_test "next" {f \(\);} "next to f"
>>>    gdb_test "next" {v = 3;} "next to v = 3"
>>>    
>>> -# FAIL was:
>>> -# 29        g ();
>>> -gdb_test "reverse-next" {f \(\);}
>>> +# Reverse step back into f ().  Puts us at call to g () in
>>> function f ().
>>> +gdb_test "reverse-next" {g \(\);}
>>> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
>>> b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
>>> new file mode 100644
>>> index 00000000000..42e41b5a2e0
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
>>> @@ -0,0 +1,48 @@
>>> +/* This testcase is part of GDB, the GNU debugger.
>>> +
>>> +   Copyright 2012-2022 Free Software Foundation, Inc.
>> copyright year should be 2023.
>>> +
>>> +   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/
>>>   >.  */
>>> +
>>> +/* The reverse finish command should return from a function and
>>> stop on
>>> +   the first instruction of the source line where the function
>>> call is made.
>>> +   Specifically, the behavior should match doing a reverse next
>>> from the
>>> +   first instruction in the function.  GDB should only require one
>>> reverse
>>> +   step or next statement to reach the previous source code line.
>>> +
>>> +   This test verifies the fix for gdb bugzilla:
>>> +
>>> +
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927
>>>   
>>> +*/
>>> +
>>> +int
>>> +function1 (int a, int b)   // FUNCTION1
>>> +{
>>> +  int ret = 0;
>>> +
>>> +  ret = a + b;
>>> +  return ret;
>>> +}
>>> +
>>> +int
>>> +main(int argc, char* argv[])
>>> +{
>>> +  int a, b;
>>> +
>>> +  a = 1;
>>> +  b = 5;
>>> +
>>> +  function1 (a, b);   // CALL FUNCTION
>>> +  return 0;
>>> +}
>>> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
>>> b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
>>> new file mode 100644
>>> index 00000000000..7880de10ffc
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
>>> @@ -0,0 +1,108 @@
>>> +# Copyright 2008-2022 Free Software Foundation, Inc.
> Fixed copyright so it reads 2008-2023.  Fixed in finish-reverse-
> next.exp and finish-reverse-next.c.
>
>>> +
>>> +# 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/
>>>   >.  */
>>> +
>>> +# This file is part of the GDB testsuite.  It tests reverse
>>> stepping.
>>> +# Lots of code borrowed from "step-test.exp".
>>> +
>>> +# The reverse finish command should return from a function and
>>> stop on
>>> +# the first instruction of the source line where the function call
>>> is made.
>>> +# Specifically, the behavior should match doing a reverse next
>>> from the
>>> +# first instruction in the function.  GDB should only take one
>>> reverse step
>>> +# or next statement to reach the previous source code line.
>>> +
>>> +# This testcase verifies the reverse-finish command stops at the
>>> first
>>> +# instruction in the source code line where the function was
>>> called.  There
>>> +# are two scenarios that must be checked:
>>> +#   1) gdb is at the entry point instruction for the function
>>> +#   2) gdb is in the body of the function.
>> While testing locally, I ran into a bug with reverse finish at the
>> epilogue of the function, that your patch also fixed. It would be
>> nice
>> if the test extended that. And since the bug was that GDB stopped
>> responding and even ctrl+c did nothing, I would suggest adding it as
>> the
>> last test.
> Discussed this additional bug in earlier emails.  Waiting to hear if
> the new test I sent reliably exposes the gdb hang that Bruno reported.
> If it does, I will add the new test to the new test case before posting
> the updated patch set. Per the discussions, I have not been able to
> reproduce the issue on my X86 or PowerPC machines.
I just tried reproducing it again on my end and failed, even my original 
test. It must have been a fluke, maybe I forgot to compile something 
after pulling from upstream.  Thanks for all the thought you put into 
it, though!
>
>>> +
>>> +# This test verifies the fix for gdb bugzilla:
>>> +#
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927
>>>   
>>> +
>>> +if ![supports_reverse] {
>>> +    return
>>> +}
>>> +
>>> +standard_testfile
>>> +
>>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile]
>>> } {
>>> +    return -1
>>> +}
>>> +
>>> +runto_main
>>> +set target_remote [gdb_is_target_remote]
>>> +
>>> +if [supports_process_record] {
>>> +    # Activate process record/replay.
>>> +    gdb_test_no_output "record" "turn on process record for test1"
>>> +}
>>> +
>>> +
>>> +### TEST 1: reverse finish from the entry point instruction in
>>> +### function1.
>>> +
>>> +# Set breakpoint at call to function1 in main.
>>> +set FUNCTION_test  [gdb_get_line_number "CALL FUNCTION" $srcfile]
>>> +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at
>>> .*" \his
>>> +    "set breakpoint on function1 call to stepi into function"
>> There is a proc in lib/gdb.exp called gdb_breakpoint which
>> couldsimplify
>> this gdb_test to
>>
>> gdb_breakpoint $srcfile:$FUNCTION_test temporary
>>
>> And would remove the need for the delete_breakpoints call later.
>>
> OK, made the change in both tests.  Made the same change in the PowerPC
> patch that adds additional tests.
>
>
>>> +
>>> +# Continue to break point at function1 call in main.
>>> +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*"
>>> \
>>> +    "stopped at function1 entry point instruction to stepi into
>>> function"
>> You can use gdb_continue_to_breakpoint here instead.
> OK, made the change in both tests.  Made the same change in the PowerPC
> patch that adds additional tests.
>
>>> +
>>> +# stepi until we see "{" indicating we entered function1
>>> +cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
>>> +
>>> +delete_breakpoints
>>> +
>>> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL
>>> FUNCTION.*" \
>>> +    "reverse-finish function1 "
>>> +
>>> +# Check to make sure we stopped at the first instruction in the
>>> source code
>>> +# line.  It should only take one reverse next command to get to
>>> the previous
>>> +# source line.   If GDB stops at the last instruction in the
>>> source code line
>>> +# it will take two reverse next instructions to get to the
>>> previous source
>>> +# line.
>>> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call
>>> from function"
>>> +
>>> +# Clear the recorded log.
>>> +gdb_test "record stop"  "Process record is stopped.*" \
>>> +    "turn off process record for test1"
>>> +gdb_test_no_output "record" "turn on process record for test2"
>>> +
>>> +
>>> +### TEST 2: reverse finish from the body of function1.
>>> +
>>> +# Set breakpoint at call to functiojust dont get it confused with maftn1 in main.
>>> +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at
>>> .*" \
>>> +    "set breakpoint on function1 call to step into body of
>>> function"
>>> +
>>> +# Continue to break point at function1 call in main.
>>> +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*"
>>> \
>>> +    "stopped at function1 entry point instruction to step to body
>>> of function"
>>> +
>>> +delete_breakpoints
>>> +
>>> +# do a step instruction to get to the body of the function
>>> +gdb_test "step" ".*int ret = 0;.*" "step test 1"
>>> +
>>> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL
>>> FUNCTION.*" \
>>> +    "reverse-finish function1 call from function body"
>>> +
>>> +# Check to make sure we stopped at the first instruction in the
>>> source code
>>> +# line.  It should only take one reverse next command to get to
>>> the previous
>>> +# source line.
>>> +gdb_test "reverse-next" ".*b = 5;.*" \
>>> +    "reverse next at b = 5, from function body"
>>> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse.exp
>>> b/gdb/testsuite/gdb.reverse/finish-reverse.exp
>>> index 01ba309420c..a05cb81892a 100644
>>> --- a/gdb/testsuite/gdb.reverse/finish-reverse.exp
>>> +++ b/gdb/testsuite/gdb.reverse/finish-reverse.exp
>>> @@ -16,6 +16,11 @@
>>>    # This file is part of the GDB testsuite.  It tests 'finish' with
>>>    # reverse debugging.
>>>    
>>> +# This test verifies the fix for gdb bugzilla:
>>> +
>>> +#
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927
>>>   
>>> +
>>> +
>> Is this comment a left over from an earlier version?
>>
>> I actually wonder if the whole new test is needed, or if you can
>> just
>> add a couple of new tests to finish-reverse.exp; is there any reason
>> you
>> went with the new test instead
> Yes, it is left over.  Initially I just added an additional test to
> finish-reverse.exp for the PowerPC fix.  But as work progressed, I kept
> adding more tests for PowerPC then for X86.  I felt that it was better
> to have a new test file that was tied to the Bugzilla.  The existing
> test file has a different focus from the new tests.  The bugzilla
> change didn't get removed from finish-reverse.exp when the tests were
> moved to the new file.  We can combine the tests again if that is
> preferable?   My preference would be to have separate test files.
> Please let me know if you would prefer a single file and I will merge
> them before re-posting the patches.
Oh, ok, the separation makes sense now. I looked only at this patch 
first and asked before looking at patch 2. I'd say its fine, no need to 
merge them.
>
>>>    if ![supports_reverse] {
>>>        return
>>>    }
>>> diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
>>> b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
>>> index 1ca7c2ce559..eb03051625a 100644
>>> --- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
>>> +++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
>>> @@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3"
>>>    # {
>>>    gdb_test "reverse-step" {nodebug \(\);}
>>>    
>>> -# FAIL was:
>>> -# No more reverse-execution history.
>>> -# {
>>> -gdb_test "reverse-next" {f \(\);}
>>> +gdb_test "reverse-next" {g \(\);}
>>> diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>>> b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>>> index ad637899e5b..1928cdda217 100644
>>> --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>>> +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>>> @@ -39,39 +39,6 @@ if { ![runto_main] } {
>>>        return -1
>>>    }
>>>    
>>> -# Do repeated stepping COMMANDs in order to reach TARGET from
>>> CURRENT
>>> -#
>>> -#  COMMAND is a stepping command
>>> -#  CURRENT is a string matching the current location
>>> -#  TARGET  is a string matching the target location
>>> -#  TEST    is the test name
>>> -#
>>> -# The function issues repeated COMMANDs as long as the location
>>> matches
>>> -# CURRENT up to a maximum of 100 steps.
>>> -#
>>> -# TEST passes if the resulting location matches TARGET and fails
>>> -# otherwise.
>>> -#
>>> -proc step_until { command current target test } {
>>> -    global gdb_prompt
>>> -
>>> -    set count 0
>>> -    gdb_test_multiple "$command" "$test" {
>>> -        -re "$current.*$gdb_prompt $" {
>>> -            incr count
>>> -            if { $count < 100 } {
>>> -                send_gdb "$command\n"
>>> -                exp_continue
>>> -            } else {
>>> -                fail "$test"
>>> -            }
>>> -        }
>>> -        -re "$target.*$gdb_prompt $" {
>>> -            pass "$test"
>>> -        }
>>> -    }
>>> -}
>>> -
>>>    gdb_test_no_output "record"
>>>    gdb_test "next" ".*" "record trace"
>>>    
>>> @@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \
>>>        "reverse-step through thunks and over inc"
>>>    
>>>    # We can use instruction stepping to step into thunks.
>>> -step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call
>>> thunk"
>>> -step_until "stepi" "indirect_thunk" "inc" \
>>> +cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call
>>> thunk"
>>> +cmd_until "stepi" "indirect_thunk" "inc" \
>>>        "stepi out of call thunk into inc"
>>>    set alphanum_re "\[a-zA-Z0-9\]"
>>>    set pic_thunk_re  "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re*
>>> \\(\\)"
>>> -step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi
>>> into return thunk"
>>> -step_until "stepi" "return_thunk" "apply" \
>>> +cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into
>>> return thunk"
>>> +cmd_until "stepi" "return_thunk" "apply" \
>>>        "stepi out of return thunk back into apply"
>>>    
>>> -step_until "reverse-stepi" "apply" "return_thunk" \
>>> +cmd_until "reverse-stepi" "apply" "return_thunk" \
>>>        "reverse-stepi into return thunk"
>>> -step_until "reverse-stepi" "return_thunk" "inc" \
>>> +cmd_until "reverse-stepi" "return_thunk" "inc" \
>>>        "reverse-stepi out of return thunk into inc"
>>> -step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk"
>>> \
>>> +cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
>>>        "reverse-stepi into call thunk"
>>> -step_until "reverse-stepi" "indirect_thunk" "apply" \
>>> +cmd_until "reverse-stepi" "indirect_thunk" "apply" \
>>>        "reverse-stepi out of call thunk into apply"
>>> diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp
>>> b/gdb/testsuite/gdb.reverse/until-precsave.exp
>>> index 0c2d7537cd6..777aec94ac1 100644
>>> --- a/gdb/testsuite/gdb.reverse/until-precsave.exp
>>> +++ b/gdb/testsuite/gdb.reverse/until-precsave.exp
>>> @@ -142,7 +142,7 @@ gdb_test "advance marker2" \
>>>    # Finish out to main scope (backward)
>>>    
>>>    gdb_test "finish" \
>>> -    " in main .*$srcfile:$bp_location20.*" \
>>> +    "main .*$srcfile:$bp_location20.*" \
>> This change doesn't seem connected to anything in this patch, is
>> this
>> just a cosmetic change or was there some problem?
>>>        "reverse-finish from marker2"
>>>    
> The output changes due to the functional changes in infrun.c.  Instead
> of stepping back one instruction i.e. ecs->event_thread-
>> stepping_over_breakpoint = 1 we step back using a range.  Apparently
> this causes the gdb output message to change.
>
> Without the patch the output looks like:
>    Run back to call of #0  marker2 (a=43) at.../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/ur1.c:30
>
>    0x0000000010000838 in main (argc=1, argv=0x7fffffffcc58, envp=0x7fffffffcc68) at /.../gdb/testsuite/gdb.reverse/until-reverse.c:48^
>
> With the patch the output looks like:
>
>    Run back to call of #0  marker2 (a=43) at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/ur1.c:30
>
>    main (argc=1, argv=0x7fffffffcc58, envp=0x7fffffffcc68) at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/until-reverse.c:48
>
> Basically, you lose the hex value and "in" with the patch applied.
> This is true in the until-reverse.exp tes, below, as well.  The output
> change was mentioned in the commit message as well.
Oh right, I should have checked the output before asking. Thank you for 
explaining!
  

GDB maintainers:

Version 2: Addressed various comments from Bruno Larsen.

This patch set fixes a couple of issues with the gdb.reverse tests
finish-precsave.exp and finish-reverse.exp.

The first issue is when doing a reverse-finish command from a function,
gdb should stop at the first instruction of the source code line where
the call was made.  The behavior should look the same as doing a
reverse-next from the first line of the function.  Currently gdb stops
at the last instruction in the caller source code line.  Issuing
reverse-step or reverse-next stops at the first instruction in the same
source code line.  It then requires a second reverse step or next
command to reach the previous source code line in the caller.  It
should only require one reverse step or next command to reach the
previous line.

The first patch in this series fixes the above issue on X86.  A number
of additional testcases require updating since the output is slightly
different or the test case no longer needs to issue the two reverse
step/next instructions.  The step_until proceedure in test
gdb.reverse/step-indirect-call-thunk.exp was moved to lib/gdb.exp and
renamed cmd_until.  The proceedure is used to test the reverse-finish
command when returning from the entry point of the function.

The second issue with the reverse-finish command is that on PowerPC the
reverse-finish doesn't stop at the function call.  The issue is PowerPC
uses two entry points.  PowerPC calls the two entry points the local
entry point (LEP) and the global entry point (GEP).  The LEP is
normally used when calling a function.  The GEP is used when the table
of contents (TOC) needs to be setup before continuing execution at the
LEP.  GDB is not handling the two entry points correctly.  The second
patch fixes the reverse-finish behavior on PowerPC.  On systems that
don't use two entry points the LEP and the GEP are the same.

A new testcase is added to verify the reverse-finish command works
correctly for X86 when returning from the body of a function and from
the entry point.  Note, the finish_backward function must handle the
two scenarios slightly differently.

The new testcase is expanded in the PPC patch to add tests for the two
scenarios for a function called via the GEP.  The initial set of tests
added in the X86 patch take care of the function being called via the
LEP on PowerPC.

The patches have been tested on PowerPC and X86 with no new
regressions.

Please let me know if the patches are acceptable for mainline.  Thanks.
  

GDB maintainers:

Version 2: Addressed various comments from Bruno Larsen.

This patch fixes the issues with the reverse-finish command on X86. 
The reverse-finish command now correctly stops at the first instruction
in the source code line of the caller.  It now only requires a single
reverse-step or reverse-next instruction to get back to the previous
source code line.

It also adds a new testcase, gdb.reverse/finish-reverse-next.exp, and
updates several existing testcases. 

Version 2 patch changes have been re-verified on PowerPC and X86 with
no regressions.

Please let me know if you have any comments on the patch.  Thanks.

                    Carl 

--------------------------------------------------------
X86: reverse-finish fix

PR record/29927  - reverse-finish requires two reverse next instructions to
reach previous source line

Currently on X86, when executing the finish command in reverse, gdb does a
single step from the first instruction in the callee to get back to the
caller.  GDB stops on the last instruction in the source code line where
the call was made.  When stopped at the last instruction of the source code
line, a reverse next or step command will stop at the first instruction
of the same source code line thus requiring two step/next commands to
reach the previous source code line.  It should only require one step/next
command to reach the previous source code line.

By contrast, a reverse next or step command from the first line in a
function stops at the first instruction in the source code line where the
call was made.

This patch fixes the reverse finish command so it will stop at the first
instruction of the source line where the function call was made.  The
behavior on X86 for the reverse-finish command now matches doing a
reverse-next from the beginning of the function.

The proceed_to_finish flag in struct thread_control_state is no longer
used.  This patch removes the declaration, initialization and setting of
the flag.

This patch requires a number of regression tests to be updated.  Test
gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
tests gdb.reverse/amd64-failcall-reverse.exp and
gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
result.

This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
reverse-finish command when returning from the entry point and from the
body of the function.

The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp
was moved to lib/gdb.exp and renamed cmd_until.

The patch has been tested on X86 and PowerPC to verify no additional
regression failures occured.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29927
---
 gdb/gdbthread.h                               |   4 -
 gdb/infcall.c                                 |   3 -
 gdb/infcmd.c                                  |  32 +++---
 gdb/infrun.c                                  |  40 +++----
 gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
 .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
 .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
 .../gdb.reverse/finish-reverse-next.exp       | 104 ++++++++++++++++++
 .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
 .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++-------
 gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
 gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
 gdb/testsuite/lib/gdb.exp                     |  33 ++++++
 13 files changed, 230 insertions(+), 106 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 11d69fceab0..e4edff2d621 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -150,10 +150,6 @@ struct thread_control_state
      the finished single step.  */
   int trap_expected = 0;
 
-  /* Nonzero if the thread is being proceeded for a "finish" command
-     or a similar situation when return value should be printed.  */
-  int proceed_to_finish = 0;
-
   /* Nonzero if the thread is being proceeded for an inferior function
      call.  */
   int in_infcall = 0;
diff --git a/gdb/infcall.c b/gdb/infcall.c
index e09904f9a35..116605c43ef 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -625,9 +625,6 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
 
   disable_watchpoints_before_interactive_call_start ();
 
-  /* We want to print return value, please...  */
-  call_thread->control.proceed_to_finish = 1;
-
   try
     {
       /* Infcalls run synchronously, in the foreground.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 0497ad05091..9c42efeae8d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm *sm)
 
   sal = find_pc_line (func_addr, 0);
 
-  tp->control.proceed_to_finish = 1;
-  /* Special case: if we're sitting at the function entry point,
-     then all we need to do is take a reverse singlestep.  We
-     don't need to set a breakpoint, and indeed it would do us
-     no good to do so.
-
-     Note that this can only happen at frame #0, since there's
-     no way that a function up the stack can have a return address
-     that's equal to its entry point.  */
+  frame_info_ptr frame = get_selected_frame (nullptr);
 
   if (sal.pc != pc)
     {
-      frame_info_ptr frame = get_selected_frame (nullptr);
       struct gdbarch *gdbarch = get_frame_arch (frame);
 
       /* Set a step-resume at the function's entry point.  Once that's
@@ -1743,16 +1734,22 @@ finish_backward (struct finish_command_fsm *sm)
       sr_sal.pspace = get_frame_program_space (frame);
       insert_step_resume_breakpoint_at_sal (gdbarch,
 					    sr_sal, null_frame_id);
-
-      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
     }
   else
     {
-      /* We're almost there -- we just need to back up by one more
-	 single-step.  */
-      tp->control.step_range_start = tp->control.step_range_end = 1;
-      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
+      /* We are exactly at the function entry point.  Note that this
+	 can only happen at frame #0.
+
+	 When setting a step range, need to call set_step_info
+	 to setup the current_line/symtab fields as well.  */
+      set_step_info (tp, frame, find_pc_line (pc, 0));
+
+      /* Return using a step range so we will keep stepping back
+	 to the first instruction in the source code line.  */
+      tp->control.step_range_start = sal.pc;
+      tp->control.step_range_end = sal.pc;
     }
+  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
 
 /* finish_forward -- helper function for finish_command.  FRAME is the
@@ -1778,9 +1775,6 @@ finish_forward (struct finish_command_fsm *sm, frame_info_ptr frame)
 
   set_longjmp_breakpoint (tp, frame_id);
 
-  /* We want to print return value, please...  */
-  tp->control.proceed_to_finish = 1;
-
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 181d961d80d..86e5ef1ed12 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct thread_info *tp)
 
   tp->control.stop_step = 0;
 
-  tp->control.proceed_to_finish = 0;
-
   tp->control.stepping_command = 0;
 
   /* Discard any remaining commands or status from previous stop.  */
@@ -6737,31 +6735,27 @@ process_event_stop_test (struct execution_control_state *ecs)
 
     case BPSTAT_WHAT_STEP_RESUME:
       infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
-
       delete_step_resume_breakpoint (ecs->event_thread);
-      if (ecs->event_thread->control.proceed_to_finish
-	  && execution_direction == EXEC_REVERSE)
+      fill_in_stop_func (gdbarch, ecs);
+
+      if (execution_direction == EXEC_REVERSE)
 	{
 	  struct thread_info *tp = ecs->event_thread;
+	  /* We are finishing a function in reverse or stepping over a function
+	     call in reverse, and just hit the step-resume breakpoint at the
+	     start address of the function, and we're almost there -- just need
+	     to back up to the function call.  */
 
-	  /* We are finishing a function in reverse, and just hit the
-	     step-resume breakpoint at the start address of the
-	     function, and we're almost there -- just need to back up
-	     by one more single-step, which should take us back to the
-	     function call.  */
-	  tp->control.step_range_start = tp->control.step_range_end = 1;
-	  keep_going (ecs);
-	  return;
-	}
-      fill_in_stop_func (gdbarch, ecs);
-      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
-	  && 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
-	     the function.  Go back to single-stepping, which should
-	     take us back to the function call.  */
-	  ecs->event_thread->stepping_over_breakpoint = 1;
+	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), 0);
+
+	  /* When setting a step range, need to call set_step_info
+	     to setup the current_line/symtab fields as well.  */
+	  set_step_info (tp, frame, stop_pc_sal);
+
+	  /* Return using a step range so we will keep stepping back to the
+	     first instruction in the source code line.  */
+	  tp->control.step_range_start = ecs->stop_func_start;
+	  tp->control.step_range_end = ecs->stop_func_start;
 	  keep_going (ecs);
 	  return;
 	}
diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
index d631beb17c8..30635ab1754 100644
--- a/gdb/testsuite/gdb.mi/mi-reverse.exp
+++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
@@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} {
 	"basics.c" $line_main_callme_1 "" \
 	"reverse finish from callme"
 
-    # Test exec-reverse-next
-    #   It takes two steps to get back to the previous line,
-    #   as the first step moves us to the start of the current line,
-    #   and the one after that moves back to the previous line.
-
-    mi_execute_to "exec-next --reverse 2" \
+    mi_execute_to "exec-next --reverse" \
  	"end-stepping-range" "main" "" \
  	"basics.c" $line_main_hello "" \
- 	"reverse next to get over the call to do_nothing"
+	"reverse next to get over the call to do_nothing"
 
     # Test exec-reverse-step
 
diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
index 52a87faabf7..9964b4f8e4b 100644
--- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
@@ -44,6 +44,5 @@ if [supports_process_record] {
 gdb_test "next" {f \(\);} "next to f"
 gdb_test "next" {v = 3;} "next to v = 3"
 
-# FAIL was:
-# 29        g ();
-gdb_test "reverse-next" {f \(\);}
+# Reverse step back into f ().  Puts us at call to g () in function f ().
+gdb_test "reverse-next" {g \(\);}
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
new file mode 100644
index 00000000000..f90ecbb93cb
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2023 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/>.  */
+
+/* The reverse finish command should return from a function and stop on
+   the first instruction of the source line where the function call is made.
+   Specifically, the behavior should match doing a reverse next from the
+   first instruction in the function.  GDB should only require one reverse
+   step or next statement to reach the previous source code line.
+
+   This test verifies the fix for gdb bugzilla:
+
+   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+*/
+
+int
+function1 (int a, int b)   // FUNCTION1
+{
+  int ret = 0;
+
+  ret = a + b;
+  return ret;
+}
+
+int
+main(int argc, char* argv[])
+{
+  int a, b;
+
+  a = 1;
+  b = 5;
+
+  function1 (a, b);   // CALL FUNCTION
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
new file mode 100644
index 00000000000..63305c109e1
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -0,0 +1,104 @@
+# Copyright 2008-2023 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/>.  */
+
+# This file is part of the GDB testsuite.  It tests reverse stepping.
+# Lots of code borrowed from "step-test.exp".
+
+# The reverse finish command should return from a function and stop on
+# the first instruction of the source line where the function call is made.
+# Specifically, the behavior should match doing a reverse next from the
+# first instruction in the function.  GDB should only take one reverse step
+# or next statement to reach the previous source code line.
+
+# This testcase verifies the reverse-finish command stops at the first
+# instruction in the source code line where the function was called.  There
+# are two scenarios that must be checked:
+#   1) gdb is at the entry point instruction for the function
+#   2) gdb is in the body of the function.
+
+# This test verifies the fix for gdb bugzilla:
+#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+
+if ![supports_reverse] {
+    return
+}
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+runto_main
+set target_remote [gdb_is_target_remote]
+
+if [supports_process_record] {
+    # Activate process record/replay.
+    gdb_test_no_output "record" "turn on process record for test1"
+}
+
+
+### TEST 1: reverse finish from the entry point instruction in
+### function1.
+
+# Set breakpoint at call to function1 in main.
+set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
+gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into function" \
+    ".*$srcfile:$bp_FUNCTION\r\n.*"
+
+# stepi until we see "{" indicating we entered function1
+repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
+
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
+    "reverse-finish function1 "
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.   If GDB stops at the last instruction in the source code line
+# it will take two reverse next instructions to get to the previous source
+# line.
+gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
+
+# Clear the recorded log.
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test1"
+gdb_test_no_output "record" "turn on process record for test2"
+
+
+### TEST 2: reverse finish from the body of function1.
+
+# Set breakpoint at call to function1 in main.
+gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "at function1 entry point instruction to step to body of function" \
+    ".*$srcfile:$bp_FUNCTION\r\n.*"
+
+# do a step instruction to get to the body of the function
+gdb_test "step" ".*int ret = 0;.*" "step test 1"
+
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
+    "reverse-finish function1 call from function body"
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.
+gdb_test "reverse-next" ".*b = 5;.*" \
+    "reverse next at b = 5, from function body"
diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
index 1ca7c2ce559..eb03051625a 100644
--- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
@@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3"
 # {
 gdb_test "reverse-step" {nodebug \(\);}
 
-# FAIL was:
-# No more reverse-execution history.
-# {
-gdb_test "reverse-next" {f \(\);}
+gdb_test "reverse-next" {g \(\);}
diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
index ad637899e5b..b82e5663bd5 100644
--- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
+++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
@@ -39,39 +39,6 @@ if { ![runto_main] } {
     return -1
 }
 
-# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
-#
-#  COMMAND is a stepping command
-#  CURRENT is a string matching the current location
-#  TARGET  is a string matching the target location
-#  TEST    is the test name
-#
-# The function issues repeated COMMANDs as long as the location matches
-# CURRENT up to a maximum of 100 steps.
-#
-# TEST passes if the resulting location matches TARGET and fails
-# otherwise.
-#
-proc step_until { command current target test } {
-    global gdb_prompt
-
-    set count 0
-    gdb_test_multiple "$command" "$test" {
-        -re "$current.*$gdb_prompt $" {
-            incr count
-            if { $count < 100 } {
-                send_gdb "$command\n"
-                exp_continue
-            } else {
-                fail "$test"
-            }
-        }
-        -re "$target.*$gdb_prompt $" {
-            pass "$test"
-        }
-    }
-}
-
 gdb_test_no_output "record"
 gdb_test "next" ".*" "record trace"
 
@@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \
     "reverse-step through thunks and over inc"
 
 # We can use instruction stepping to step into thunks.
-step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
-step_until "stepi" "indirect_thunk" "inc" \
+repeat_cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
+repeat_cmd_until "stepi" "indirect_thunk" "inc" \
     "stepi out of call thunk into inc"
 set alphanum_re "\[a-zA-Z0-9\]"
 set pic_thunk_re  "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re* \\(\\)"
-step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
-step_until "stepi" "return_thunk" "apply" \
+repeat_cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
+repeat_cmd_until "stepi" "return_thunk" "apply" \
     "stepi out of return thunk back into apply"
 
-step_until "reverse-stepi" "apply" "return_thunk" \
+repeat_cmd_until "reverse-stepi" "apply" "return_thunk" \
     "reverse-stepi into return thunk"
-step_until "reverse-stepi" "return_thunk" "inc" \
+repeat_cmd_until "reverse-stepi" "return_thunk" "inc" \
     "reverse-stepi out of return thunk into inc"
-step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
+repeat_cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
     "reverse-stepi into call thunk"
-step_until "reverse-stepi" "indirect_thunk" "apply" \
+repeat_cmd_until "reverse-stepi" "indirect_thunk" "apply" \
     "reverse-stepi out of call thunk into apply"
diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp b/gdb/testsuite/gdb.reverse/until-precsave.exp
index 0c2d7537cd6..777aec94ac1 100644
--- a/gdb/testsuite/gdb.reverse/until-precsave.exp
+++ b/gdb/testsuite/gdb.reverse/until-precsave.exp
@@ -142,7 +142,7 @@ gdb_test "advance marker2" \
 # Finish out to main scope (backward)
 
 gdb_test "finish" \
-    " in main .*$srcfile:$bp_location20.*" \
+    "main .*$srcfile:$bp_location20.*" \
     "reverse-finish from marker2"
 
 # Advance backward to last line of factorial (outer invocation)
diff --git a/gdb/testsuite/gdb.reverse/until-reverse.exp b/gdb/testsuite/gdb.reverse/until-reverse.exp
index 23fc881dbf2..3a05953329f 100644
--- a/gdb/testsuite/gdb.reverse/until-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/until-reverse.exp
@@ -113,7 +113,7 @@ gdb_test "advance marker2" \
 # Finish out to main scope (backward)
 
 gdb_test "finish" \
-    " in main .*$srcfile:$bp_location20.*" \
+    "main .*$srcfile:$bp_location20.*" \
     "reverse-finish from marker2"
 
 # Advance backward to last line of factorial (outer invocation)
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index c41d4698d66..234c21a04ea 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9301,6 +9301,39 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
     }
 }
 
+# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
+#
+#  COMMAND is a stepping command
+#  CURRENT is a string matching the current location
+#  TARGET  is a string matching the target location
+#  TEST    is the test name
+#
+# The function issues repeated COMMANDs as long as the location matches
+# CURRENT up to a maximum of 100 steps.
+#
+# TEST passes if the resulting location matches TARGET and fails
+# otherwise.
+
+proc repeat_cmd_until { command current target test } {
+    global gdb_prompt
+
+    set count 0
+    gdb_test_multiple "$command" "$test" {
+	-re "$current.*$gdb_prompt $" {
+	    incr count
+	    if { $count < 100 } {
+		send_gdb "$command\n"
+		exp_continue
+	    } else {
+		fail "$test"
+	    }
+	}
+	-re "$target.*$gdb_prompt $" {
+	    pass "$test"
+	}
+    }
+}
+
 # Check if the compiler emits epilogue information associated
 # with the closing brace or with the last statement line.
 #
  

GDB maintainers:

Version 2: Addressed various comments from Bruno Larsen.

This patch fixes the issues with the reverse-finish command on
PowerPC.  The reverse-finish command now correctly stops at the first
instruction in the source code line of the caller.  

The patch adds tests for calling a function via the GEP to the new test
gdb.reverse/finish-reverse-next.exp.

Version 2 patch changes have been re-verified on PowerPC and X86 with
no regressions.

Please let me know if you have any comments on the patch.  Thanks.

                    Carl

---------------------------------------------------------------
PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp

PR record/29927 - reverse-finish requires two reverse next instructions to
reach previous source line

PowerPC uses two entry points called the local entry point (LEP) and the
global entry point (GEP).  Normally the LEP is used when calling a
function.  However, if the table of contents (TOC) value in register 2 is
not valid the GEP is called to setup the TOC before execution continues at
the LEP.  When executing in reverse, the function finish_backward sets the
break point at the alternate entry point (GEP).  However if the forward
execution enters via the normal entry point (LEP), the reverse execution
never sees the break point at the GEP of the function.  Reverse execution
continues until the next break point is encountered or the end of the
recorded log is reached causing gdb to stop at the wrong place.

This patch adds a new address to struct execution_control_state to hold the
address of the alternate function start address, known as the GEP on
PowerPC.  The finish_backwards function is updated.  If the stopping point
is between the two entry points (the LEP and GEP on PowerPC), the stepping
range is set to execute back to the alternate entry point (GEP on PowerPC).
Otherwise, a breakpoint is inserted at the normal entry point (LEP on
PowerPC).

Function process_event_stop_test checks uses a stepping range to stop
execution in the caller at the first instruction of the source code line.
Note, on systems that only support one entry point, the address of the two
entry points are the same.

Test finish-reverse-next.exp is updated to include tests for the
reverse-finish command when the function is entered via the normal entry
point (i.e. the LEP) and the alternate entry point (i.e. the GEP).

The patch has been tested on X86 and PowerPC with no regressions.
---
 gdb/infcmd.c                                  | 40 +++++---
 gdb/infrun.c                                  | 16 +++-
 .../gdb.reverse/finish-reverse-next.c         | 41 +++++++-
 .../gdb.reverse/finish-reverse-next.exp       | 96 ++++++++++++++++---
 4 files changed, 161 insertions(+), 32 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9c42efeae8d..6aaed34b1b6 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1722,22 +1722,25 @@ finish_backward (struct finish_command_fsm *sm)
   sal = find_pc_line (func_addr, 0);
 
   frame_info_ptr frame = get_selected_frame (nullptr);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  CORE_ADDR alt_entry_point = sal.pc;
+  CORE_ADDR entry_point = alt_entry_point;
 
-  if (sal.pc != pc)
+  if (gdbarch_skip_entrypoint_p (gdbarch))
     {
-      struct gdbarch *gdbarch = get_frame_arch (frame);
-
-      /* Set a step-resume at the function's entry point.  Once that's
-	 hit, we'll do one more step backwards.  */
-      symtab_and_line sr_sal;
-      sr_sal.pc = sal.pc;
-      sr_sal.pspace = get_frame_program_space (frame);
-      insert_step_resume_breakpoint_at_sal (gdbarch,
-					    sr_sal, null_frame_id);
+      /* Some architectures, like PowerPC use local and global entry points.
+	 There is only one Entry Point (GEP = LEP) for other architectures.
+	 The GEP is an alternate entry point.  The LEP is the normal entry
+	 point.  The value of entry_point was initialized to the alternate
+	 entry point (GEP).  It will be adjusted if the normal entry point
+	 (LEP) was used.  */
+       entry_point = gdbarch_skip_entrypoint (gdbarch, entry_point);
     }
-  else
+
+  if (alt_entry_point <= pc && pc <= entry_point)
     {
-      /* We are exactly at the function entry point.  Note that this
+      /* We are exactly at the function entry point, or between the entry
+	 point on platforms that have two (like PowerPC).  Note that this
 	 can only happen at frame #0.
 
 	 When setting a step range, need to call set_step_info
@@ -1746,8 +1749,17 @@ finish_backward (struct finish_command_fsm *sm)
 
       /* Return using a step range so we will keep stepping back
 	 to the first instruction in the source code line.  */
-      tp->control.step_range_start = sal.pc;
-      tp->control.step_range_end = sal.pc;
+      tp->control.step_range_start = alt_entry_point;
+      tp->control.step_range_end = entry_point;
+    }
+  else
+    {
+      symtab_and_line sr_sal;
+      /* Set a step-resume at the function's entry point.  */
+      sr_sal.pc = entry_point;
+      sr_sal.pspace = get_frame_program_space (frame);
+      insert_step_resume_breakpoint_at_sal (gdbarch,
+					    sr_sal, null_frame_id);
     }
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 86e5ef1ed12..b69f84824a3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1868,6 +1868,7 @@ struct execution_control_state
 
   struct target_waitstatus ws;
   int stop_func_filled_in = 0;
+  CORE_ADDR stop_func_alt_start = 0;
   CORE_ADDR stop_func_start = 0;
   CORE_ADDR stop_func_end = 0;
   const char *stop_func_name = nullptr;
@@ -4663,6 +4664,12 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 				    &block);
       ecs->stop_func_name = gsi == nullptr ? nullptr : gsi->print_name ();
 
+      /* PowerPC functions have a Local Entry Point and a Global Entry
+	 Point.  There is only one Entry Point (GEP = LEP) for other
+	 architectures.  Save the alternate entry point address (GEP) for
+	 use later.  */
+      ecs->stop_func_alt_start = ecs->stop_func_start;
+
       /* The call to find_pc_partial_function, above, will set
 	 stop_func_start and stop_func_end to the start and end
 	 of the range containing the stop pc.  If this range
@@ -4679,6 +4686,9 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 	    += gdbarch_deprecated_function_start_offset (gdbarch);
 
 	  if (gdbarch_skip_entrypoint_p (gdbarch))
+	    /* The PowerPC architecture uses two entry points.  Stop at the
+	       regular entry point (LEP on PowerPC) initially.  Will setup a
+	       breakpoint for the alternate entry point (GEP) later.  */
 	    ecs->stop_func_start
 	      = gdbarch_skip_entrypoint (gdbarch, ecs->stop_func_start);
 	}
@@ -6754,7 +6764,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  /* Return using a step range so we will keep stepping back to the
 	     first instruction in the source code line.  */
-	  tp->control.step_range_start = ecs->stop_func_start;
+	  tp->control.step_range_start = ecs->stop_func_alt_start;
 	  tp->control.step_range_end = ecs->stop_func_start;
 	  keep_going (ecs);
 	  return;
@@ -6891,8 +6901,10 @@ process_event_stop_test (struct execution_control_state *ecs)
 	 (unless it's the function entry point, in which case
 	 keep going back to the call point).  */
       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
+	  && (stop_pc < ecs->stop_func_alt_start
+	      || stop_pc > ecs->stop_func_start)
 	  && execution_direction == EXEC_REVERSE)
 	end_stepping_range (ecs);
       else
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
index f90ecbb93cb..6bac7c6117a 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
@@ -1,4 +1,4 @@
-/* This testcase is part of GDB, the GNU debugger.
+j/* This testcase is part of GDB, the GNU debugger.
 
    Copyright 2012-2023 Free Software Foundation, Inc.
 
@@ -24,11 +24,37 @@
    This test verifies the fix for gdb bugzilla:
 
    https://sourceware.org/bugzilla/show_bug.cgi?id=29927
-*/
+
+   PowerPC supports two entry points to a function.  The normal entry point
+   is called the local entry point (LEP).  The alternat entry point is called
+   the global entry point (GEP).  The GEP is only used if the table of
+   contents (TOC) value stored in register r2 needs to be setup prior to
+   execution starting at the LEP.  A function call via a function pointer
+   will entry via the GEP.  A normal function call will enter via the LEP.
+
+   This test has been expanded to include tests to verify the reverse-finish
+   command works properly if the function is called via the GEP.  The original
+   test only verified the reverse-finish command for a normal call that used
+   the LEP.  */
 
 int
 function1 (int a, int b)   // FUNCTION1
 {
+  /* The assembly code for this function when compiled for PowerPC is as
+     follows:
+
+     0000000010000758 <function1>:
+     10000758:	02 10 40 3c 	lis     r2,4098        <- GEP
+     1000075c:	00 7f 42 38 	addi    r2,r2,32512
+     10000760:	a6 02 08 7c 	mflr    r0             <- LEP
+     10000764:	10 00 01 f8 	std     r0,16(r1)
+     ....
+
+     When the function is called on PowerPC with function1 (a, b) the call
+     enters at the Local Entry Point (LEP).  When the function is called via
+     a function pointer, the Global Entry Point (GEP) for function1 is used.
+     The GEP sets up register 2 before reaching the LEP.
+  */
   int ret = 0;
 
   ret = a + b;
@@ -39,10 +65,19 @@ int
 main(int argc, char* argv[])
 {
   int a, b;
+  int (*funp) (int, int) = &function1;
+
+  /* Call function via Local Entry Point (LEP).  */
 
   a = 1;
   b = 5;
 
-  function1 (a, b);   // CALL FUNCTION
+  function1 (a, b);   // CALL VIA LEP
+
+  /* Call function via Global Entry Point (GEP).  */
+  a = 10;
+  b = 50;
+
+  funp (a, b);        // CALL VIA GEP
   return 0;
 }
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
index 63305c109e1..240b7214ed2 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -31,6 +31,16 @@
 # This test verifies the fix for gdb bugzilla:
 #   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
 
+# PowerPC supports two entry points to a function.  The normal entry point
+# is called the local entry point (LEP).  The alternat entry point is called
+# the global entry point (GEP).  A function call via a function pointer
+# will entry via the GEP.  A normal function call will enter via the LEP.
+#
+# This test has been expanded to include tests to verify the reverse-finish
+# command works properly if the function is called via the GEP.  The original
+# test only verified the reverse-finish command for a normal call that used
+# the LEP.
+
 if ![supports_reverse] {
     return
 }
@@ -50,30 +60,30 @@ if [supports_process_record] {
 }
 
 
-### TEST 1: reverse finish from the entry point instruction in
-### function1.
+### TEST 1: reverse finish from the entry point instruction (LEP) in
+### function1 when called using the normal entry point (LEP).
 
 # Set breakpoint at call to function1 in main.
-set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
-gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
 
 # Continue to break point at function1 call in main.
 gdb_continue_to_breakpoint \
     "stopped at function1 entry point instruction to stepi into function" \
-    ".*$srcfile:$bp_FUNCTION\r\n.*"
+    ".*$srcfile:$bp_LEP_test\r\n.*"
 
 # stepi until we see "{" indicating we entered function1
-repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
+repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call"
 
-gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
-    "reverse-finish function1 "
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish function1 LEP call from LEP "
 
 # Check to make sure we stopped at the first instruction in the source code
 # line.  It should only take one reverse next command to get to the previous
 # source line.   If GDB stops at the last instruction in the source code line
 # it will take two reverse next instructions to get to the previous source
 # line.
-gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
+gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from LEP"
 
 # Clear the recorded log.
 gdb_test "record stop"  "Process record is stopped.*" \
@@ -84,21 +94,81 @@ gdb_test_no_output "record" "turn on process record for test2"
 ### TEST 2: reverse finish from the body of function1.
 
 # Set breakpoint at call to function1 in main.
-gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
 
 # Continue to break point at function1 call in main.
 gdb_continue_to_breakpoint \
     "at function1 entry point instruction to step to body of function" \
-    ".*$srcfile:$bp_FUNCTION\r\n.*"
+    ".*$srcfile:$bp_LEP_test\r\n.*"
 
 # do a step instruction to get to the body of the function
 gdb_test "step" ".*int ret = 0;.*" "step test 1"
 
-gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
-    "reverse-finish function1 call from function body"
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish function1 LEP call from function body"
 
 # Check to make sure we stopped at the first instruction in the source code
 # line.  It should only take one reverse next command to get to the previous
 # source line.
 gdb_test "reverse-next" ".*b = 5;.*" \
     "reverse next at b = 5, from function body"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test2"
+gdb_test_no_output "record" "turn on process record for test3"
+
+
+### TEST 3: reverse finish from the alternate entry point instruction (GEP) in
+### function1 when called using the alternate entry point (GEP).
+
+# Set breakpoint at call to funp in main.
+set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into function" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# stepi until we see "{" indicating we entered function.
+cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
+
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "function1 GEP call call from GEP"
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.  If GDB stops at the last instruction in the source code line
+# it will take two reverse next instructions to get to the previous source
+# line.
+gdb_test "reverse-next" ".*b = 50;.*" "reverse next at b = 50, call from GEP"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test3"
+gdb_test_no_output "record" "turn on process record for test4"
+
+
+### TEST 4: reverse finish from the body of function 1 when calling using the
+### alternate entrypoint (GEP).
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call.
+gdb_continue_to_breakpoint \
+    "at function1 entry point instruction to step to body of function" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# Step into body of funp, called via GEP.
+gdb_test "step" ".*int ret = 0;.*" "step test 2"
+
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "reverse-finish function1 GEP call, from function body  "
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.  If GDB stops at the last instruction in the source code line
+# it will take two reverse next instructions to get to the previous source
+# line.
+gdb_test "reverse-next" ".*b = 50;.*" \
+    "reverse next at b = 50 from function body"
  

On 16/01/2023 17:37, Carl Love wrote:
> GDB maintainers:
>
> Version 2: Addressed various comments from Bruno Larsen.
>
> This patch fixes the issues with the reverse-finish command on X86.
> The reverse-finish command now correctly stops at the first instruction
> in the source code line of the caller.  It now only requires a single
> reverse-step or reverse-next instruction to get back to the previous
> source code line.
>
> It also adds a new testcase, gdb.reverse/finish-reverse-next.exp, and
> updates several existing testcases.
>
> Version 2 patch changes have been re-verified on PowerPC and X86 with
> no regressions.
>
> Please let me know if you have any comments on the patch.  Thanks.
>
>                      Carl
>
> --------------------------------------------------------
> X86: reverse-finish fix
>
> PR record/29927  - reverse-finish requires two reverse next instructions to
> reach previous source line
>
> Currently on X86, when executing the finish command in reverse, gdb does a
> single step from the first instruction in the callee to get back to the
> caller.  GDB stops on the last instruction in the source code line where
> the call was made.  When stopped at the last instruction of the source code
> line, a reverse next or step command will stop at the first instruction
> of the same source code line thus requiring two step/next commands to
> reach the previous source code line.  It should only require one step/next
> command to reach the previous source code line.
>
> By contrast, a reverse next or step command from the first line in a
> function stops at the first instruction in the source code line where the
> call was made.
>
> This patch fixes the reverse finish command so it will stop at the first
> instruction of the source line where the function call was made.  The
> behavior on X86 for the reverse-finish command now matches doing a
> reverse-next from the beginning of the function.
>
> The proceed_to_finish flag in struct thread_control_state is no longer
> used.  This patch removes the declaration, initialization and setting of
> the flag.
>
> This patch requires a number of regression tests to be updated.  Test
> gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
> previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
> and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
> tests gdb.reverse/amd64-failcall-reverse.exp and
> gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
> result.
>
> This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
> reverse-finish command when returning from the entry point and from the
> body of the function.
>
> The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp
> was moved to lib/gdb.exp and renamed cmd_until.
>
> The patch has been tested on X86 and PowerPC to verify no additional
> regression failures occured.
>
> Bug:https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> ---

Changes look good now and I also dont see any regressions.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
  

On 16/01/2023 17:37, Carl Love wrote:
> GDB maintainers:
>
> Version 2: Addressed various comments from Bruno Larsen.
>
> This patch fixes the issues with the reverse-finish command on
> PowerPC.  The reverse-finish command now correctly stops at the first
> instruction in the source code line of the caller.
>
> The patch adds tests for calling a function via the GEP to the new test
> gdb.reverse/finish-reverse-next.exp.
>
> Version 2 patch changes have been re-verified on PowerPC and X86 with
> no regressions.
>
> Please let me know if you have any comments on the patch.  Thanks.
>
>                      Carl
>
> ---------------------------------------------------------------
> PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp
>
> PR record/29927 - reverse-finish requires two reverse next instructions to
> reach previous source line
>
> PowerPC uses two entry points called the local entry point (LEP) and the
> global entry point (GEP).  Normally the LEP is used when calling a
> function.  However, if the table of contents (TOC) value in register 2 is
> not valid the GEP is called to setup the TOC before execution continues at
> the LEP.  When executing in reverse, the function finish_backward sets the
> break point at the alternate entry point (GEP).  However if the forward
> execution enters via the normal entry point (LEP), the reverse execution
> never sees the break point at the GEP of the function.  Reverse execution
> continues until the next break point is encountered or the end of the
> recorded log is reached causing gdb to stop at the wrong place.
>
> This patch adds a new address to struct execution_control_state to hold the
> address of the alternate function start address, known as the GEP on
> PowerPC.  The finish_backwards function is updated.  If the stopping point
> is between the two entry points (the LEP and GEP on PowerPC), the stepping
> range is set to execute back to the alternate entry point (GEP on PowerPC).
> Otherwise, a breakpoint is inserted at the normal entry point (LEP on
> PowerPC).
>
> Function process_event_stop_test checks uses a stepping range to stop
> execution in the caller at the first instruction of the source code line.
> Note, on systems that only support one entry point, the address of the two
> entry points are the same.
>
> Test finish-reverse-next.exp is updated to include tests for the
> reverse-finish command when the function is entered via the normal entry
> point (i.e. the LEP) and the alternate entry point (i.e. the GEP).
>
> The patch has been tested on X86 and PowerPC with no regressions.

I will reiterate that I don't know much about PPC, so the best I can do 
is check for style and tests, but apart from a few minor nits inlined, 
it looks ok

Tested-By: Bruno Larsen <blarsen@redhat.com>

> ---
>   gdb/infcmd.c                                  | 40 +++++---
>   gdb/infrun.c                                  | 16 +++-
>   .../gdb.reverse/finish-reverse-next.c         | 41 +++++++-
>   .../gdb.reverse/finish-reverse-next.exp       | 96 ++++++++++++++++---
>   4 files changed, 161 insertions(+), 32 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9c42efeae8d..6aaed34b1b6 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1722,22 +1722,25 @@ finish_backward (struct finish_command_fsm *sm)
>     sal = find_pc_line (func_addr, 0);
>   
>     frame_info_ptr frame = get_selected_frame (nullptr);
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  CORE_ADDR alt_entry_point = sal.pc;
> +  CORE_ADDR entry_point = alt_entry_point;
>   
> -  if (sal.pc != pc)
> +  if (gdbarch_skip_entrypoint_p (gdbarch))
>       {
> -      struct gdbarch *gdbarch = get_frame_arch (frame);
> -
> -      /* Set a step-resume at the function's entry point.  Once that's
> -	 hit, we'll do one more step backwards.  */
> -      symtab_and_line sr_sal;
> -      sr_sal.pc = sal.pc;
> -      sr_sal.pspace = get_frame_program_space (frame);
> -      insert_step_resume_breakpoint_at_sal (gdbarch,
> -					    sr_sal, null_frame_id);
> +      /* Some architectures, like PowerPC use local and global entry points.
> +	 There is only one Entry Point (GEP = LEP) for other architectures.
> +	 The GEP is an alternate entry point.  The LEP is the normal entry
> +	 point.  The value of entry_point was initialized to the alternate
> +	 entry point (GEP).  It will be adjusted if the normal entry point
> +	 (LEP) was used.  */
> +       entry_point = gdbarch_skip_entrypoint (gdbarch, entry_point);
>       }
> -  else
> +
> +  if (alt_entry_point <= pc && pc <= entry_point)
>       {
> -      /* We are exactly at the function entry point.  Note that this
> +      /* We are exactly at the function entry point, or between the entry
> +	 point on platforms that have two (like PowerPC).  Note that this
>   	 can only happen at frame #0.
>   
>   	 When setting a step range, need to call set_step_info
> @@ -1746,8 +1749,17 @@ finish_backward (struct finish_command_fsm *sm)
>   
>         /* Return using a step range so we will keep stepping back
>   	 to the first instruction in the source code line.  */
> -      tp->control.step_range_start = sal.pc;
> -      tp->control.step_range_end = sal.pc;
> +      tp->control.step_range_start = alt_entry_point;
> +      tp->control.step_range_end = entry_point;
> +    }
> +  else
> +    {
> +      symtab_and_line sr_sal;
> +      /* Set a step-resume at the function's entry point.  */
> +      sr_sal.pc = entry_point;
> +      sr_sal.pspace = get_frame_program_space (frame);
> +      insert_step_resume_breakpoint_at_sal (gdbarch,
> +					    sr_sal, null_frame_id);
>       }
>     proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>   }
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 86e5ef1ed12..b69f84824a3 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1868,6 +1868,7 @@ struct execution_control_state
>   
>     struct target_waitstatus ws;
>     int stop_func_filled_in = 0;
> +  CORE_ADDR stop_func_alt_start = 0;
>     CORE_ADDR stop_func_start = 0;
>     CORE_ADDR stop_func_end = 0;
>     const char *stop_func_name = nullptr;
> @@ -4663,6 +4664,12 @@ fill_in_stop_func (struct gdbarch *gdbarch,
>   				    &block);
>         ecs->stop_func_name = gsi == nullptr ? nullptr : gsi->print_name ();
>   
> +      /* PowerPC functions have a Local Entry Point and a Global Entry
> +	 Point.  There is only one Entry Point (GEP = LEP) for other
> +	 architectures.  Save the alternate entry point address (GEP) for
> +	 use later.  */
> +      ecs->stop_func_alt_start = ecs->stop_func_start;
> +
>         /* The call to find_pc_partial_function, above, will set
>   	 stop_func_start and stop_func_end to the start and end
>   	 of the range containing the stop pc.  If this range
> @@ -4679,6 +4686,9 @@ fill_in_stop_func (struct gdbarch *gdbarch,
>   	    += gdbarch_deprecated_function_start_offset (gdbarch);
>   
>   	  if (gdbarch_skip_entrypoint_p (gdbarch))
> +	    /* The PowerPC architecture uses two entry points.  Stop at the
> +	       regular entry point (LEP on PowerPC) initially.  Will setup a
> +	       breakpoint for the alternate entry point (GEP) later.  */
>   	    ecs->stop_func_start
>   	      = gdbarch_skip_entrypoint (gdbarch, ecs->stop_func_start);
>   	}
> @@ -6754,7 +6764,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>   
>   	  /* Return using a step range so we will keep stepping back to the
>   	     first instruction in the source code line.  */
> -	  tp->control.step_range_start = ecs->stop_func_start;
> +	  tp->control.step_range_start = ecs->stop_func_alt_start;
>   	  tp->control.step_range_end = ecs->stop_func_start;
>   	  keep_going (ecs);
>   	  return;
> @@ -6891,8 +6901,10 @@ process_event_stop_test (struct execution_control_state *ecs)
>   	 (unless it's the function entry point, in which case
>   	 keep going back to the call point).  */
>         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
> +	  && (stop_pc < ecs->stop_func_alt_start
> +	      || stop_pc > ecs->stop_func_start)
>   	  && execution_direction == EXEC_REVERSE)
>   	end_stepping_range (ecs);
>         else
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> index f90ecbb93cb..6bac7c6117a 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> @@ -1,4 +1,4 @@
> -/* This testcase is part of GDB, the GNU debugger.
> +j/* This testcase is part of GDB, the GNU debugger.
Accidental change that breaks the test here.
>   
>      Copyright 2012-2023 Free Software Foundation, Inc.
>   
> @@ -24,11 +24,37 @@
>      This test verifies the fix for gdb bugzilla:
>   
>      https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> -*/
> +
> +   PowerPC supports two entry points to a function.  The normal entry point
> +   is called the local entry point (LEP).  The alternat entry point is called
> +   the global entry point (GEP).  The GEP is only used if the table of
> +   contents (TOC) value stored in register r2 needs to be setup prior to
> +   execution starting at the LEP.  A function call via a function pointer
> +   will entry via the GEP.  A normal function call will enter via the LEP.
> +
> +   This test has been expanded to include tests to verify the reverse-finish
> +   command works properly if the function is called via the GEP.  The original
> +   test only verified the reverse-finish command for a normal call that used
> +   the LEP.  */
>   
>   int
>   function1 (int a, int b)   // FUNCTION1
>   {
> +  /* The assembly code for this function when compiled for PowerPC is as
> +     follows:
> +
> +     0000000010000758 <function1>:
> +     10000758:	02 10 40 3c 	lis     r2,4098        <- GEP
> +     1000075c:	00 7f 42 38 	addi    r2,r2,32512
> +     10000760:	a6 02 08 7c 	mflr    r0             <- LEP
> +     10000764:	10 00 01 f8 	std     r0,16(r1)
> +     ....
> +
> +     When the function is called on PowerPC with function1 (a, b) the call
> +     enters at the Local Entry Point (LEP).  When the function is called via
> +     a function pointer, the Global Entry Point (GEP) for function1 is used.
> +     The GEP sets up register 2 before reaching the LEP.
> +  */
>     int ret = 0;
>   
>     ret = a + b;
> @@ -39,10 +65,19 @@ int
>   main(int argc, char* argv[])
>   {
>     int a, b;
> +  int (*funp) (int, int) = &function1;
> +
> +  /* Call function via Local Entry Point (LEP).  */
>   
>     a = 1;
>     b = 5;
>   
> -  function1 (a, b);   // CALL FUNCTION
> +  function1 (a, b);   // CALL VIA LEP
> +
> +  /* Call function via Global Entry Point (GEP).  */
> +  a = 10;
> +  b = 50;
> +
> +  funp (a, b);        // CALL VIA GEP
>     return 0;
>   }
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> index 63305c109e1..240b7214ed2 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -31,6 +31,16 @@
>   # This test verifies the fix for gdb bugzilla:
>   #   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
>   
> +# PowerPC supports two entry points to a function.  The normal entry point
> +# is called the local entry point (LEP).  The alternat entry point is called
> +# the global entry point (GEP).  A function call via a function pointer
> +# will entry via the GEP.  A normal function call will enter via the LEP.
> +#
> +# This test has been expanded to include tests to verify the reverse-finish
> +# command works properly if the function is called via the GEP.  The original
> +# test only verified the reverse-finish command for a normal call that used
> +# the LEP.
> +
>   if ![supports_reverse] {
>       return
>   }
> @@ -50,30 +60,30 @@ if [supports_process_record] {
>   }
>   
>   
> -### TEST 1: reverse finish from the entry point instruction in
> -### function1.
> +### TEST 1: reverse finish from the entry point instruction (LEP) in
> +### function1 when called using the normal entry point (LEP).
>   
>   # Set breakpoint at call to function1 in main.
> -set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
> -gdb_breakpoint $srcfile:$bp_FUNCTION temporary
> +set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
> +gdb_breakpoint $srcfile:$bp_LEP_test temporary
>   
>   # Continue to break point at function1 call in main.
>   gdb_continue_to_breakpoint \
>       "stopped at function1 entry point instruction to stepi into function" \
> -    ".*$srcfile:$bp_FUNCTION\r\n.*"
> +    ".*$srcfile:$bp_LEP_test\r\n.*"
>   
>   # stepi until we see "{" indicating we entered function1
> -repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
> +repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call"
>   
> -gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
> -    "reverse-finish function1 "
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
> +    "reverse-finish function1 LEP call from LEP "
>   
>   # Check to make sure we stopped at the first instruction in the source code
>   # line.  It should only take one reverse next command to get to the previous
>   # source line.   If GDB stops at the last instruction in the source code line
>   # it will take two reverse next instructions to get to the previous source
>   # line.
> -gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from LEP"
>   
>   # Clear the recorded log.
>   gdb_test "record stop"  "Process record is stopped.*" \
> @@ -84,21 +94,81 @@ gdb_test_no_output "record" "turn on process record for test2"
>   ### TEST 2: reverse finish from the body of function1.
>   
>   # Set breakpoint at call to function1 in main.
> -gdb_breakpoint $srcfile:$bp_FUNCTION temporary
> +gdb_breakpoint $srcfile:$bp_LEP_test temporary
>   
>   # Continue to break point at function1 call in main.
>   gdb_continue_to_breakpoint \
>       "at function1 entry point instruction to step to body of function" \
> -    ".*$srcfile:$bp_FUNCTION\r\n.*"
> +    ".*$srcfile:$bp_LEP_test\r\n.*"
duplicate test name here.
>   
>   # do a step instruction to get to the body of the function
>   gdb_test "step" ".*int ret = 0;.*" "step test 1"
>   
> -gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
> -    "reverse-finish function1 call from function body"
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
> +    "reverse-finish function1 LEP call from function body"
>   
>   # Check to make sure we stopped at the first instruction in the source code
>   # line.  It should only take one reverse next command to get to the previous
>   # source line.
>   gdb_test "reverse-next" ".*b = 5;.*" \
>       "reverse next at b = 5, from function body"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test2"
> +gdb_test_no_output "record" "turn on process record for test3"
> +
> +
> +### TEST 3: reverse finish from the alternate entry point instruction (GEP) in
> +### function1 when called using the alternate entry point (GEP).
> +
> +# Set breakpoint at call to funp in main.
> +set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
> +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> +
> +# Continue to break point at funp call in main.
> +gdb_continue_to_breakpoint \
> +    "stopped at function1 entry point instruction to stepi into function" \
> +    ".*$srcfile:$bp_GEP_test\r\n.*"
Duplicated test name here too.
> +
> +# stepi until we see "{" indicating we entered function.
> +cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"

s/cmd_until/repeat_cmd_until

you probably missed because of the test compilation failed.

> +
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> +    "function1 GEP call call from GEP"
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line.  It should only take one reverse next command to get to the previous
> +# source line.  If GDB stops at the last instruction in the source code line
> +# it will take two reverse next instructions to get to the previous source
> +# line.
> +gdb_test "reverse-next" ".*b = 50;.*" "reverse next at b = 50, call from GEP"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test3"
> +gdb_test_no_output "record" "turn on process record for test4"
> +
> +
> +### TEST 4: reverse finish from the body of function 1 when calling using the
> +### alternate entrypoint (GEP).
> +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> +
> +# Continue to break point at funp call.
> +gdb_continue_to_breakpoint \
> +    "at function1 entry point instruction to step to body of function" \
> +    ".*$srcfile:$bp_GEP_test\r\n.*"
> +
> +# Step into body of funp, called via GEP.
> +gdb_test "step" ".*int ret = 0;.*" "step test 2"
> +
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> +    "reverse-finish function1 GEP call, from function body  "
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line.  It should only take one reverse next command to get to the previous
> +# source line.  If GDB stops at the last instruction in the source code line
> +# it will take two reverse next instructions to get to the previous source
> +# line.
> +gdb_test "reverse-next" ".*b = 50;.*" \
> +    "reverse next at b = 50 from function body"
  

Bruno:

On Tue, 2023-01-17 at 15:29 +0100, Bruno Larsen wrote:
> On 16/01/2023 17:37, Carl Love wrote:
> > GDB maintainers:
> > 
> > Version 2: Addressed various comments from Bruno Larsen.
> > 
> > This patch fixes the issues with the reverse-finish command on
> > PowerPC.  The reverse-finish command now correctly stops at the
> > first
> > instruction in the source code line of the caller.
> > 
> > The patch adds tests for calling a function via the GEP to the new
> > test
> > gdb.reverse/finish-reverse-next.exp.
> > 
> > Version 2 patch changes have been re-verified on PowerPC and X86
> > with
> > no regressions.
> > 
> > Please let me know if you have any comments on the patch.  Thanks.
> > 
> >                      Carl
> > 
> > ---------------------------------------------------------------
> > PowerPC: fix for gdb.reverse/finish-precsave.exp and
> > gdb.reverse/finish-reverse.exp
> > 
> > PR record/29927 - reverse-finish requires two reverse next
> > instructions to
> > reach previous source line
> > 
> > PowerPC uses two entry points called the local entry point (LEP)
> > and the
> > global entry point (GEP).  Normally the LEP is used when calling a
> > function.  However, if the table of contents (TOC) value in
> > register 2 is
> > not valid the GEP is called to setup the TOC before execution
> > continues at
> > the LEP.  When executing in reverse, the function finish_backward
> > sets the
> > break point at the alternate entry point (GEP).  However if the
> > forward
> > execution enters via the normal entry point (LEP), the reverse
> > execution
> > never sees the break point at the GEP of the function.  Reverse
> > execution
> > continues until the next break point is encountered or the end of
> > the
> > recorded log is reached causing gdb to stop at the wrong place.
> > 
> > This patch adds a new address to struct execution_control_state to
> > hold the
> > address of the alternate function start address, known as the GEP
> > on
> > PowerPC.  The finish_backwards function is updated.  If the
> > stopping point
> > is between the two entry points (the LEP and GEP on PowerPC), the
> > stepping
> > range is set to execute back to the alternate entry point (GEP on
> > PowerPC).
> > Otherwise, a breakpoint is inserted at the normal entry point (LEP
> > on
> > PowerPC).
> > 
> > Function process_event_stop_test checks uses a stepping range to
> > stop
> > execution in the caller at the first instruction of the source code
> > line.
> > Note, on systems that only support one entry point, the address of
> > the two
> > entry points are the same.
> > 
> > Test finish-reverse-next.exp is updated to include tests for the
> > reverse-finish command when the function is entered via the normal
> > entry
> > point (i.e. the LEP) and the alternate entry point (i.e. the GEP).
> > 
> > The patch has been tested on X86 and PowerPC with no regressions.
> 
> I will reiterate that I don't know much about PPC, so the best I can
> do 
> is check for style and tests, but apart from a few minor nits
> inlined, 
> it looks ok

I fixed the issues, as noted below.  I did a careful review of the log
files to make sure there are no remaining duplicates and that each of
the tests in both patches ran as expected.

Thanks for your help on these patches.

                       Carl 
> 
> Tested-By: Bruno Larsen <blarsen@redhat.com>
> 
> > ---
> >   gdb/infcmd.c                                  | 40 +++++---
> >   gdb/infrun.c                                  | 16 +++-
> >   .../gdb.reverse/finish-reverse-next.c         | 41 +++++++-
> >   .../gdb.reverse/finish-reverse-next.exp       | 96
> > ++++++++++++++++---
> >   4 files changed, 161 insertions(+), 32 deletions(-)
> > 
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 9c42efeae8d..6aaed34b1b6 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -1722,22 +1722,25 @@ finish_backward (struct finish_command_fsm
> > *sm)
> >     sal = find_pc_line (func_addr, 0);
> >   
> >     frame_info_ptr frame = get_selected_frame (nullptr);
> > +  struct gdbarch *gdbarch = get_frame_arch (frame);
> > +  CORE_ADDR alt_entry_point = sal.pc;
> > +  CORE_ADDR entry_point = alt_entry_point;
> >   
> > -  if (sal.pc != pc)
> > +  if (gdbarch_skip_entrypoint_p (gdbarch))
> >       {
> > -      struct gdbarch *gdbarch = get_frame_arch (frame);
> > -
> > -      /* Set a step-resume at the function's entry point.  Once
> > that's
> > -	 hit, we'll do one more step backwards.  */
> > -      symtab_and_line sr_sal;
> > -      sr_sal.pc = sal.pc;
> > -      sr_sal.pspace = get_frame_program_space (frame);
> > -      insert_step_resume_breakpoint_at_sal (gdbarch,
> > -					    sr_sal, null_frame_id);
> > +      /* Some architectures, like PowerPC use local and global
> > entry points.
> > +	 There is only one Entry Point (GEP = LEP) for other
> > architectures.
> > +	 The GEP is an alternate entry point.  The LEP is the normal
> > entry
> > +	 point.  The value of entry_point was initialized to the
> > alternate
> > +	 entry point (GEP).  It will be adjusted if the normal entry
> > point
> > +	 (LEP) was used.  */
> > +       entry_point = gdbarch_skip_entrypoint (gdbarch,
> > entry_point);
> >       }
> > -  else
> > +
> > +  if (alt_entry_point <= pc && pc <= entry_point)
> >       {
> > -      /* We are exactly at the function entry point.  Note that
> > this
> > +      /* We are exactly at the function entry point, or between
> > the entry
> > +	 point on platforms that have two (like PowerPC).  Note that
> > this
> >   	 can only happen at frame #0.
> >   
> >   	 When setting a step range, need to call set_step_info
> > @@ -1746,8 +1749,17 @@ finish_backward (struct finish_command_fsm
> > *sm)
> >   
> >         /* Return using a step range so we will keep stepping back
> >   	 to the first instruction in the source code line.  */
> > -      tp->control.step_range_start = sal.pc;
> > -      tp->control.step_range_end = sal.pc;
> > +      tp->control.step_range_start = alt_entry_point;
> > +      tp->control.step_range_end = entry_point;
> > +    }
> > +  else
> > +    {
> > +      symtab_and_line sr_sal;
> > +      /* Set a step-resume at the function's entry point.  */
> > +      sr_sal.pc = entry_point;
> > +      sr_sal.pspace = get_frame_program_space (frame);
> > +      insert_step_resume_breakpoint_at_sal (gdbarch,
> > +					    sr_sal, null_frame_id);
> >       }
> >     proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> >   }
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 86e5ef1ed12..b69f84824a3 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -1868,6 +1868,7 @@ struct execution_control_state
> >   
> >     struct target_waitstatus ws;
> >     int stop_func_filled_in = 0;
> > +  CORE_ADDR stop_func_alt_start = 0;
> >     CORE_ADDR stop_func_start = 0;
> >     CORE_ADDR stop_func_end = 0;
> >     const char *stop_func_name = nullptr;
> > @@ -4663,6 +4664,12 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> >   				    &block);
> >         ecs->stop_func_name = gsi == nullptr ? nullptr : gsi-
> > >print_name ();
> >   
> > +      /* PowerPC functions have a Local Entry Point and a Global
> > Entry
> > +	 Point.  There is only one Entry Point (GEP = LEP) for other
> > +	 architectures.  Save the alternate entry point address (GEP)
> > for
> > +	 use later.  */
> > +      ecs->stop_func_alt_start = ecs->stop_func_start;
> > +
> >         /* The call to find_pc_partial_function, above, will set
> >   	 stop_func_start and stop_func_end to the start and end
> >   	 of the range containing the stop pc.  If this range
> > @@ -4679,6 +4686,9 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> >   	    += gdbarch_deprecated_function_start_offset (gdbarch);
> >   
> >   	  if (gdbarch_skip_entrypoint_p (gdbarch))
> > +	    /* The PowerPC architecture uses two entry points.  Stop at
> > the
> > +	       regular entry point (LEP on PowerPC) initially.  Will
> > setup a
> > +	       breakpoint for the alternate entry point (GEP)
> > later.  */
> >   	    ecs->stop_func_start
> >   	      = gdbarch_skip_entrypoint (gdbarch, ecs-
> > >stop_func_start);
> >   	}
> > @@ -6754,7 +6764,7 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >   
> >   	  /* Return using a step range so we will keep stepping back to
> > the
> >   	     first instruction in the source code line.  */
> > -	  tp->control.step_range_start = ecs->stop_func_start;
> > +	  tp->control.step_range_start = ecs->stop_func_alt_start;
> >   	  tp->control.step_range_end = ecs->stop_func_start;
> >   	  keep_going (ecs);
> >   	  return;
> > @@ -6891,8 +6901,10 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >   	 (unless it's the function entry point, in which case
> >   	 keep going back to the call point).  */
> >         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
> > +	  && (stop_pc < ecs->stop_func_alt_start
> > +	      || stop_pc > ecs->stop_func_start)
> >   	  && execution_direction == EXEC_REVERSE)
> >   	end_stepping_range (ecs);
> >         else
> > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > index f90ecbb93cb..6bac7c6117a 100644
> > --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > @@ -1,4 +1,4 @@
> > -/* This testcase is part of GDB, the GNU debugger.
> > +j/* This testcase is part of GDB, the GNU debugger.
> Accidental change that breaks the test here.

Fixed.

> >   
> >      Copyright 2012-2023 Free Software Foundation, Inc.
> >   
> > @@ -24,11 +24,37 @@
> >      This test verifies the fix for gdb bugzilla:
> >   
> >      
> > https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> >  
> > -*/
> > +
> > +   PowerPC supports two entry points to a function.  The normal
> > entry point
> > +   is called the local entry point (LEP).  The alternat entry
> > point is called
> > +   the global entry point (GEP).  The GEP is only used if the
> > table of
> > +   contents (TOC) value stored in register r2 needs to be setup
> > prior to
> > +   execution starting at the LEP.  A function call via a function
> > pointer
> > +   will entry via the GEP.  A normal function call will enter via
> > the LEP.
> > +
> > +   This test has been expanded to include tests to verify the
> > reverse-finish
> > +   command works properly if the function is called via the
> > GEP.  The original
> > +   test only verified the reverse-finish command for a normal call
> > that used
> > +   the LEP.  */
> >   
> >   int
> >   function1 (int a, int b)   // FUNCTION1
> >   {
> > +  /* The assembly code for this function when compiled for PowerPC
> > is as
> > +     follows:
> > +
> > +     0000000010000758 <function1>:
> > +     10000758:	02 10 40 3c 	lis     r2,4098        <-
> > GEP
> > +     1000075c:	00 7f 42 38 	addi    r2,r2,32512
> > +     10000760:	a6 02 08 7c 	mflr    r0             <-
> > LEP
> > +     10000764:	10 00 01 f8 	std     r0,16(r1)
> > +     ....
> > +
> > +     When the function is called on PowerPC with function1 (a, b)
> > the call
> > +     enters at the Local Entry Point (LEP).  When the function is
> > called via
> > +     a function pointer, the Global Entry Point (GEP) for
> > function1 is used.
> > +     The GEP sets up register 2 before reaching the LEP.
> > +  */
> >     int ret = 0;
> >   
> >     ret = a + b;
> > @@ -39,10 +65,19 @@ int
> >   main(int argc, char* argv[])
> >   {
> >     int a, b;
> > +  int (*funp) (int, int) = &function1;
> > +
> > +  /* Call function via Local Entry Point (LEP).  */
> >   
> >     a = 1;
> >     b = 5;
> >   
> > -  function1 (a, b);   // CALL FUNCTION
> > +  function1 (a, b);   // CALL VIA LEP
> > +
> > +  /* Call function via Global Entry Point (GEP).  */
> > +  a = 10;
> > +  b = 50;
> > +
> > +  funp (a, b);        // CALL VIA GEP
> >     return 0;
> >   }
> > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > index 63305c109e1..240b7214ed2 100644
> > --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > @@ -31,6 +31,16 @@
> >   # This test verifies the fix for gdb bugzilla:
> >   #   
> > https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> >  
> >   
> > +# PowerPC supports two entry points to a function.  The normal
> > entry point
> > +# is called the local entry point (LEP).  The alternat entry point
> > is called
> > +# the global entry point (GEP).  A function call via a function
> > pointer
> > +# will entry via the GEP.  A normal function call will enter via
> > the LEP.
> > +#
> > +# This test has been expanded to include tests to verify the
> > reverse-finish
> > +# command works properly if the function is called via the
> > GEP.  The original
> > +# test only verified the reverse-finish command for a normal call
> > that used
> > +# the LEP.
> > +
> >   if ![supports_reverse] {
> >       return
> >   }
> > @@ -50,30 +60,30 @@ if [supports_process_record] {
> >   }
> >   
> >   
> > -### TEST 1: reverse finish from the entry point instruction in
> > -### function1.
> > +### TEST 1: reverse finish from the entry point instruction (LEP)
> > in
> > +### function1 when called using the normal entry point (LEP).
> >   
> >   # Set breakpoint at call to function1 in main.
> > -set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
> > -gdb_breakpoint $srcfile:$bp_FUNCTION temporary
> > +set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
> > +gdb_breakpoint $srcfile:$bp_LEP_test temporary
> >   
> >   # Continue to break point at function1 call in main.
> >   gdb_continue_to_breakpoint \
> >       "stopped at function1 entry point instruction to stepi into
> > function" \
> > -    ".*$srcfile:$bp_FUNCTION\r\n.*"
> > +    ".*$srcfile:$bp_LEP_test\r\n.*"
> >   
> >   # stepi until we see "{" indicating we entered function1
> > -repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1
> > call"
> > +repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1
> > call"
> >   
> > -gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL
> > FUNCTION.*" \
> > -    "reverse-finish function1 "
> > +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA
> > LEP.*" \
> > +    "reverse-finish function1 LEP call from LEP "
> >   
> >   # Check to make sure we stopped at the first instruction in the
> > source code
> >   # line.  It should only take one reverse next command to get to
> > the previous
> >   # source line.   If GDB stops at the last instruction in the
> > source code line
> >   # it will take two reverse next instructions to get to the
> > previous source
> >   # line.
> > -gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call
> > from function"
> > +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call
> > from LEP"
> >   
> >   # Clear the recorded log.
> >   gdb_test "record stop"  "Process record is stopped.*" \
> > @@ -84,21 +94,81 @@ gdb_test_no_output "record" "turn on process
> > record for test2"
> >   ### TEST 2: reverse finish from the body of function1.
> >   
> >   # Set breakpoint at call to function1 in main.
> > -gdb_breakpoint $srcfile:$bp_FUNCTION temporary
> > +gdb_breakpoint $srcfile:$bp_LEP_test temporary
> >   
> >   # Continue to break point at function1 call in main.
> >   gdb_continue_to_breakpoint \
> >       "at function1 entry point instruction to step to body of
> > function" \
> > -    ".*$srcfile:$bp_FUNCTION\r\n.*"
> > +    ".*$srcfile:$bp_LEP_test\r\n.*"
> duplicate test name here.

fixed

> >   
> >   # do a step instruction to get to the body of the function
> >   gdb_test "step" ".*int ret = 0;.*" "step test 1"
> >   
> > -gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL
> > FUNCTION.*" \
> > -    "reverse-finish function1 call from function body"
> > +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA
> > LEP.*" \
> > +    "reverse-finish function1 LEP call from function body"
> >   
> >   # Check to make sure we stopped at the first instruction in the
> > source code
> >   # line.  It should only take one reverse next command to get to
> > the previous
> >   # source line.
> >   gdb_test "reverse-next" ".*b = 5;.*" \
> >       "reverse next at b = 5, from function body"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test2"
> > +gdb_test_no_output "record" "turn on process record for test3"
> > +
> > +
> > +### TEST 3: reverse finish from the alternate entry point
> > instruction (GEP) in
> > +### function1 when called using the alternate entry point (GEP).
> > +
> > +# Set breakpoint at call to funp in main.
> > +set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
> > +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> > +
> > +# Continue to break point at funp call in main.
> > +gdb_continue_to_breakpoint \
> > +    "stopped at function1 entry point instruction to stepi into
> > function" \
> > +    ".*$srcfile:$bp_GEP_test\r\n.*"
> Duplicated test name here too.

Fixed

> > +
> > +# stepi until we see "{" indicating we entered function.
> > +cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
> 
> s/cmd_until/repeat_cmd_until
> 
> you probably missed because of the test compilation failed.

Fixed.  Yup, I always try to go read the log file for that reason.  I
have been burned in the past thinking it worked when it didn't actually
run.

> 
> > +
> > +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> > +    "function1 GEP call call from GEP"
> > +
> > +# Check to make sure we stopped at the first instruction in the
> > source code
> > +# line.  It should only take one reverse next command to get to
> > the previous
> > +# source line.  If GDB stops at the last instruction in the source
> > code line
> > +# it will take two reverse next instructions to get to the
> > previous source
> > +# line.
> > +gdb_test "reverse-next" ".*b = 50;.*" "reverse next at b = 50,
> > call from GEP"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test3"
> > +gdb_test_no_output "record" "turn on process record for test4"
> > +
> > +
> > +### TEST 4: reverse finish from the body of function 1 when
> > calling using the
> > +### alternate entrypoint (GEP).
> > +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> > +
> > +# Continue to break point at funp call.
> > +gdb_continue_to_breakpoint \
> > +    "at function1 entry point instruction to step to body of
> > function" \
> > +    ".*$srcfile:$bp_GEP_test\r\n.*"
> > +
> > +# Step into body of funp, called via GEP.
> > +gdb_test "step" ".*int ret = 0;.*" "step test 2"
> > +
> > +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> > +    "reverse-finish function1 GEP call, from function body  "
> > +
> > +# Check to make sure we stopped at the first instruction in the
> > source code
> > +# line.  It should only take one reverse next command to get to
> > the previous
> > +# source line.  If GDB stops at the last instruction in the source
> > code line
> > +# it will take two reverse next instructions to get to the
> > previous source
> > +# line.
> > +gdb_test "reverse-next" ".*b = 50;.*" \
> > +    "reverse next at b = 50 from function body"
> 
>
  

AFAICT, this has been committed just now.

I'm having difficulty finding an approval, did it happen maybe offlist?

Thanks,
- Tom
  

Tom:

On Tue, 2023-01-17 at 17:55 +0100, Tom de Vries wrote:
> AFAICT, this has been committed just now.
> 
> I'm having difficulty finding an approval, did it happen maybe
> offlist?
> 
> Thanks,
> - Tom

I just committed it based on Bruno's reply.

   > The patch has been tested on X86 and PowerPC with no regressions.

   I will reiterate that I don't know much about PPC, so the best I can do 
   is check for style and tests, but apart from a few minor nits inlined, 
   it looks ok

   Tested-By: Bruno Larsen <blarsen@redhat.com>

I thought that was an approval to commit.  Is it not?

                       Carl
  

On 1/17/23 18:03, Carl Love wrote:
> Tom:
> 
> On Tue, 2023-01-17 at 17:55 +0100, Tom de Vries wrote:
>> AFAICT, this has been committed just now.
>>
>> I'm having difficulty finding an approval, did it happen maybe
>> offlist?
>>
>> Thanks,
>> - Tom
> 
> I just committed it based on Bruno's reply.
> 
>     > The patch has been tested on X86 and PowerPC with no regressions.
> 
>     I will reiterate that I don't know much about PPC, so the best I can do
>     is check for style and tests, but apart from a few minor nits inlined,
>     it looks ok
> 
>     Tested-By: Bruno Larsen <blarsen@redhat.com>
> 
> I thought that was an approval to commit.  Is it not?

Hi Carl,

sorry, AFAIU it's not.  Bruno has been helpfully reviewing your patch, 
but AFAIK he cannot approve it.

This is an easy mistake to make though, given the used formulation, it's 
not the first time it happened, and it's one of the reasons we're trying 
to move towards replying with the Approved-By tag to make approval more 
formal, explicit and unambiguous.

FYI, I'm working my way toward reviewing these patches, and have 
regained access to my usual powerpc machine, so I'll try to reproduce 
the issue you're seeing.

Thanks,
- Tom
  

Tom:

On Tue, 2023-01-17 at 18:14 +0100, Tom de Vries wrote:
> On 1/17/23 18:03, Carl Love wrote:
> > Tom:
> > 
> > On Tue, 2023-01-17 at 17:55 +0100, Tom de Vries wrote:
> > > AFAICT, this has been committed just now.
> > > 
> > > I'm having difficulty finding an approval, did it happen maybe
> > > offlist?
> > > 
> > > Thanks,
> > > - Tom
> > 
> > I just committed it based on Bruno's reply.
> > 
> >     > The patch has been tested on X86 and PowerPC with no
> > regressions.
> > 
> >     I will reiterate that I don't know much about PPC, so the best
> > I can do
> >     is check for style and tests, but apart from a few minor nits
> > inlined,
> >     it looks ok
> > 
> >     Tested-By: Bruno Larsen <blarsen@redhat.com>
> > 
> > I thought that was an approval to commit.  Is it not?
> 
> Hi Carl,
> 
> sorry, AFAIU it's not.  Bruno has been helpfully reviewing your
> patch, 
> but AFAIK he cannot approve it.
> 
> This is an easy mistake to make though, given the used formulation,
> it's 
> not the first time it happened, and it's one of the reasons we're
> trying 
> to move towards replying with the Approved-By tag to make approval
> more 
> formal, explicit and unambiguous.
> 
> FYI, I'm working my way toward reviewing these patches, and have 
> regained access to my usual powerpc machine, so I'll try to
> reproduce 
> the issue you're seeing.

OK, thanks for the clarification on the process.  In the past, I have
had people explicitly state in their reply that they could not approve
a patch but were just reviewing and commenting on it.  

I will be sure to look for the "Approved-By tag" in the future.  Sorry
for the mistake.

Please let me know if you need me to revert the patch, make some fixes,
or?  

Thanks. 

                     Carl
  

On 1/17/23 20:31, Carl Love wrote:
> Tom:
> 
> On Tue, 2023-01-17 at 18:14 +0100, Tom de Vries wrote:
>> On 1/17/23 18:03, Carl Love wrote:
>>> Tom:
>>>
>>> On Tue, 2023-01-17 at 17:55 +0100, Tom de Vries wrote:
>>>> AFAICT, this has been committed just now.
>>>>
>>>> I'm having difficulty finding an approval, did it happen maybe
>>>> offlist?
>>>>
>>>> Thanks,
>>>> - Tom
>>>
>>> I just committed it based on Bruno's reply.
>>>
>>>      > The patch has been tested on X86 and PowerPC with no
>>> regressions.
>>>
>>>      I will reiterate that I don't know much about PPC, so the best
>>> I can do
>>>      is check for style and tests, but apart from a few minor nits
>>> inlined,
>>>      it looks ok
>>>
>>>      Tested-By: Bruno Larsen <blarsen@redhat.com>
>>>
>>> I thought that was an approval to commit.  Is it not?
>>
>> Hi Carl,
>>
>> sorry, AFAIU it's not.  Bruno has been helpfully reviewing your
>> patch,
>> but AFAIK he cannot approve it.
>>
>> This is an easy mistake to make though, given the used formulation,
>> it's
>> not the first time it happened, and it's one of the reasons we're
>> trying
>> to move towards replying with the Approved-By tag to make approval
>> more
>> formal, explicit and unambiguous.
>>
>> FYI, I'm working my way toward reviewing these patches, and have
>> regained access to my usual powerpc machine, so I'll try to
>> reproduce
>> the issue you're seeing.
> 
> OK, thanks for the clarification on the process.  In the past, I have
> had people explicitly state in their reply that they could not approve
> a patch but were just reviewing and commenting on it.
> 
> I will be sure to look for the "Approved-By tag" in the future.  Sorry
> for the mistake.
> 
> Please let me know if you need me to revert the patch, make some fixes,
> or?

I've run into regressions due to the first patch:
...
FAIL: gdb.btrace/rn-dl-bind.exp: test: reverse-next
FAIL: gdb.btrace/tailcall.exp: reverse-next.1
FAIL: gdb.btrace/tailcall.exp: step.1
...

So, please revert both.

Thanks,
- Tom
  

On Wed, 2023-01-18 at 11:55 +0100, Tom de Vries wrote:
> On 1/17/23 20:31, Carl Love wrote:
> > Tom:
> > 
> > On Tue, 2023-01-17 at 18:14 +0100, Tom de Vries wrote:
> > > On 1/17/23 18:03, Carl Love wrote:
> > > > Tom:
> > > > 
> > > > On Tue, 2023-01-17 at 17:55 +0100, Tom de Vries wrote:
> > > > > AFAICT, this has been committed just now.
> > > > > 
> > > > > I'm having difficulty finding an approval, did it happen
> > > > > maybe
> > > > > offlist?
> > > > > 
> > > > > Thanks,
> > > > > - Tom
> > > > 
> > > > I just committed it based on Bruno's reply.
> > > > 
> > > >      > The patch has been tested on X86 and PowerPC with no
> > > > regressions.
> > > > 
> > > >      I will reiterate that I don't know much about PPC, so the
> > > > best
> > > > I can do
> > > >      is check for style and tests, but apart from a few minor
> > > > nits
> > > > inlined,
> > > >      it looks ok
> > > > 
> > > >      Tested-By: Bruno Larsen <blarsen@redhat.com>
> > > > 
> > > > I thought that was an approval to commit.  Is it not?
> > > 
> > > Hi Carl,
> > > 
> > > sorry, AFAIU it's not.  Bruno has been helpfully reviewing your
> > > patch,
> > > but AFAIK he cannot approve it.
> > > 
> > > This is an easy mistake to make though, given the used
> > > formulation,
> > > it's
> > > not the first time it happened, and it's one of the reasons we're
> > > trying
> > > to move towards replying with the Approved-By tag to make
> > > approval
> > > more
> > > formal, explicit and unambiguous.
> > > 
> > > FYI, I'm working my way toward reviewing these patches, and have
> > > regained access to my usual powerpc machine, so I'll try to
> > > reproduce
> > > the issue you're seeing.
> > 
> > OK, thanks for the clarification on the process.  In the past, I
> > have
> > had people explicitly state in their reply that they could not
> > approve
> > a patch but were just reviewing and commenting on it.
> > 
> > I will be sure to look for the "Approved-By tag" in the
> > future.  Sorry
> > for the mistake.
> > 
> > Please let me know if you need me to revert the patch, make some
> > fixes,
> > or?
> 
> I've run into regressions due to the first patch:
> ...
> FAIL: gdb.btrace/rn-dl-bind.exp: test: reverse-next
> FAIL: gdb.btrace/tailcall.exp: reverse-next.1
> FAIL: gdb.btrace/tailcall.exp: step.1
> ...
> 
> So, please revert both.

I reverted both commits.  I will take see if I can reproduce the
failures you are seeing.


ommit b986eec55f460a9c77a0c06ec30d7280293f7a8c (HEAD -> master,
origin/master, origin/HEAD)
Author: Carl Love <cel@us.ibm.com>
Date:   Wed Jan 18 11:13:17 2023 -0500

    Revert "X86: reverse-finish fix"
    
    This reverts commit b22548ddb30bfb167708e82d3bb932461c1b703a.
    
    This patch is being reverted since the patch series is causing
regressions.

commit 15d2b36c5b60795067cec773a66d2627d2bf9266
Author: Carl Love <cel@us.ibm.com>
Date:   Wed Jan 18 11:12:13 2023 -0500

    Revert "PowerPC: fix for gdb.reverse/finish-precsave.exp and
gdb.reverse/finish-reverse.exp"
    
    This reverts commit 92e07580db6a5572573d5177ca23933064158f89.
    
    Reverting patch as the patch series is causing regressions.


                  Carl
  

Tom and Bruno:

On Wed, 2023-01-18 at 11:55 +0100, Tom de Vries wrote:
> 
> I've run into regressions due to the first patch:
> ...
> FAIL: gdb.btrace/rn-dl-bind.exp: test: reverse-next
> FAIL: gdb.btrace/tailcall.exp: reverse-next.1
> FAIL: gdb.btrace/tailcall.exp: step.1
> ...

I am looking into these failures.  I tried running the rn-dl-bind.exp
test on my PowerPC and X86 box.  On both systems, I get the error:

UNSUPPORTED: gdb.btrace/rn-dl-bind.exp: target does not support record-btrace
testcase /../binutils-gdb-finish-precsave/gdb/testsuite/gdb.btrace/rn-dl-bind.exp completed in 0 seconds

I tried compiling the source code and running gdb by hand on my PowerPC
machine.  When I try the command record btrace, I get the message:  You
can't do that when your target is `multi-thread'.  Digging into the GDB
manual I see it says the following about btrace:

   Hardware-supported instruction recording, supported on Intel
   processors. This method does not record data. Further, the data is
   collected in a ring buffer so old data will be overwritten when the
   buffer is full. It allows limited reverse execution. Variables and
   registers are not available during reverse execution. In remote
   debugging, recording continues on disconnect. Recorded data can be
   inspected after reconnecting. The recording may be stopped using
   record stop.

From this I am assuming you saw the error on and Intel box not a
PowerPC box.  

I tried to run the rn-dl-bind.exp test on my X66 box by hand and got
the messages:

   Breakpoint 1, main () at rn-dl-bind.c:35
   35        ret = test ();                        /* main.1 */
   (gdb) record btrace pt
   Intel Processor Trace support was disabled at compile time.
   (gdb) record btrace bts
   Could not enable branch tracing for Thread 0x7ffff7d85740 (LWP
   3087787): BTS support has been disabled for the target cpu.

I found on the web page:  
 
https://gdb-patches.sourceware.narkive.com/Y1ypJmWj/patch-btrace-diagnose-record-bt
race-pt-without-libipt

says record btrace pt neds:

   This requires a system with Linux kernel 4.1 or later running on a
   5th Generation Intel Core processor or later.

Since I couldn't get the test to run on an IBM machine, I built GDB on
my personal Linux laptop.  Initially, I was not able to get the test to
run.  The log file had the error message:

   (gdb) record btrace
   Could not enable branch tracing for process 3941218: You do not have
   permission to record the process.  Try setting
   /proc/sys/kernel/perf_event_paranoid to 2 or less.


I changed /proc/sys/kernel/perf_event_paranoid from 4 to 2.  Now the
test runs.

   (gdb) PASS: gdb.btrace/rn-dl-bind.exp: test: reverse-next
   testcase /home/carll/GDB/binutils-
   build/gdb/testsuite/../../../binutils-
   gdb/gdb/testsuite/gdb.btrace/rn-dl-bind.exp completed in 0 seconds

                   === gdb Summary ===

   # of expected passes            9

The test works fine without my patches, with just the X86 patch and
with both the X86 patch and the PPC patch.  Unfortunately, I wasn't
able to run the tests on the IBM machine but I can on my personal
laptop.  However, I am still not seeing any errors as a result of my
patches.  

Not sure what more I can do at this point.  If you have some time, can
you take a look at the failures on your machine and let me know what
you are seeing.  Maybe we can figure out what is going on.

Perhaps Bruno can also check to see if the two tests were ran on his
machine.  If not, hopefully the info above will help Bruno to get the
tests to run on his machine and we can see if they fail there as well.

Thanks for the help with this patch.

                                 Carl
  

On 18/01/2023 23:26, Carl Love wrote:
> Not sure what more I can do at this point.  If you have some time, can
> you take a look at the failures on your machine and let me know what
> you are seeing.  Maybe we can figure out what is going on.
>
> Perhaps Bruno can also check to see if the two tests were ran on his
> machine.  If not, hopefully the info above will help Bruno to get the
> tests to run on his machine and we can see if they fail there as well.

I just tried running the gdb.btrace/rn-dl-bind.exp and also got 9 
passes, so I cant help there, but I did miss the 2 failures in 
tailcall.exp; here's the relevant log:

Breakpoint 1, main () at tailcall.c:37
38        answer += 1;
(gdb) PASS: gdb.btrace/tailcall.exp: next.1
reverse-next
foo () at tailcall.c:29
29        return bar ();
(gdb) FAIL: gdb.btrace/tailcall.exp: reverse-next.1
step
bar () at tailcall.c:24
24      }
(gdb) FAIL: gdb.btrace/tailcall.exp: step.1
finish

Some extra context, line 37 (the line that is being "undone" by 
reverse-next) has a call to foo, which calls bar. Whats going on here is 
that we're hitting the step-resume breakpoint when exiting the bar call, 
instead of when exiting the foo call.Has a very similar smell to the 
type of bug I fixed in commit 1f3e37e057e876b37db49dbd8ed5ca22c33f6772, 
but that fix itself might not work because of tailcall optimizations.

If you have the test compiled, you can trigger the bug using regular 
record, no need to use btrace specifically.
  

Bruno:

On Thu, 2023-01-19 at 09:04 +0100, Bruno Larsen wrote:
> On 18/01/2023 23:26, Carl Love wrote:
> > Not sure what more I can do at this point.  If you have some time,
> > can
> > you take a look at the failures on your machine and let me know
> > what
> > you are seeing.  Maybe we can figure out what is going on.
> > 
> > Perhaps Bruno can also check to see if the two tests were ran on
> > his
> > machine.  If not, hopefully the info above will help Bruno to get
> > the
> > tests to run on his machine and we can see if they fail there as
> > well.
> 
> I just tried running the gdb.btrace/rn-dl-bind.exp and also got 9 
> passes, so I cant help there, but I did miss the 2 failures in 
> tailcall.exp; here's the relevant log:
> 
> Breakpoint 1, main () at tailcall.c:37
> 38        answer += 1;
> (gdb) PASS: gdb.btrace/tailcall.exp: next.1
> reverse-next
> foo () at tailcall.c:29
> 29        return bar ();
> (gdb) FAIL: gdb.btrace/tailcall.exp: reverse-next.1
> step
> bar () at tailcall.c:24
> 24      }
> (gdb) FAIL: gdb.btrace/tailcall.exp: step.1
> finish
> 
> Some extra context, line 37 (the line that is being "undone" by 
> reverse-next) has a call to foo, which calls bar. Whats going on here
> is 
> that we're hitting the step-resume breakpoint when exiting the bar
> call, 
> instead of when exiting the foo call.Has a very similar smell to the 
> type of bug I fixed in commit
> 1f3e37e057e876b37db49dbd8ed5ca22c33f6772, 
> but that fix itself might not work because of tailcall optimizations.
> 
> If you have the test compiled, you can trigger the bug using regular 
> record, no need to use btrace specifically.

Thanks, I went back and tried the tailcall again.  I was able to get it
to generate 2 fails on my laptop with just the X86 patch.  Not sure why
I didn't see that before, maybe I had a typo???   I did note that git
on my laptop seems to act a bit weird so maybe that was an issue??  I
am having to double check the code changes after running git to make
sure it applied the changes correctly.  Not sure why I didn't see the
passes in the full regression suite either.  Anyway, I am looking to
see if I can figure out what the regression issue is for this test.

The gdb.btrace/rn-dl-bind.exp test still runs fine with 9 passes.

                                Carl
  

Bruno, Tom:


On Thu, 2023-01-19 at 08:56 -0800, Carl Love wrote:
<snip>
> > 
> > If you have the test compiled, you can trigger the bug using
> > regular 
> > record, no need to use btrace specifically.
> 
> Thanks, I went back and tried the tailcall again.  I was able to get
> it
> to generate 2 fails on my laptop with just the X86 patch.  Not sure
> why
> I didn't see that before, maybe I had a typo??? 

I reviewed the full regression log on my X86 laptop and yup there are
the two errors for tailcall in the log.  I just missed them when I was
looking.  Argh.

>   I did note that git
> on my laptop seems to act a bit weird so maybe that was an issue??  I
> am having to double check the code changes after running git to make
> sure it applied the changes correctly.  Not sure why I didn't see the
> passes in the full regression suite either.  Anyway, I am looking to
> see if I can figure out what the regression issue is for this test.
> 
> The gdb.btrace/rn-dl-bind.exp test still runs fine with 9 passes.

I claim, the test case for gdb.btrace/tailcall.exp also needs fixing.

Without the patch, when you were in the main program at "answer += 1"
and did a reverse-next, gdb would go back thru the call to function bar
and thru the call to function foo stopping in main.  That doesn't seem
right to me as you have stepped back thru function foo and bar.  

With the patch the reverse-next only step back thru function bar and
stops in function foo at the call to bar, i.e. doesn't continue to step
back thru function bar.

The following is the tailcall source code with comments showing where
the reverse-step and reverse-next stop with and without the X86 patch
applied.  Hopefully the code below is a little easier to follow.
Initially, the reverse-step and reverse-next tests are executed from
the line "answer += 1;" in function main.


static __attribute__ ((noinline)) int
bar (void)
{
  return 42;
}                    <- reverse-step stops here (with no patch
                        and with X86 patch)

static __attribute__ ((noinline)) int
foo (void)
{
  return bar ();     <- reverse-next stops here (patch X86 applied)
}

int
main (void)
{
  int answer;

  answer = foo ();    <- reverse-next tops here (no patch applied, stepped
                         back thru both the bar and foo functions)
  answer += 1;        <- GDB is here, now issue the reverse-step
                         and reverse-next commands

  return answer;
}

As a result of the change in the reverse-next instruction, the expected
test results for the tailcall test need to be updated to reflect the
fact that gdb now stops in function foo not main.

--- a/gdb/testsuite/gdb.btrace/tailcall.exp
+++ b/gdb/testsuite/gdb.btrace/tailcall.exp
@@ -102,9 +102,9 @@ gdb_test "reverse-step" "\[^\r\n\]*main \\(\\) at tailcall.c
:37\r\n.*" \
     "reverse-step.2"
 gdb_test "next" "\[^\r\n\]*38.*" \
     "next.1"
-gdb_test "reverse-next" "\[^\r\n\]*main \\(\\) at tailcall.c:37\r\n.*" \
+gdb_test "reverse-next" "\[^\r\n\]*foo \\(\\) at tailcall.c:29\r\n.*" \
     "reverse-next.1"
-gdb_test "step" "\[^\r\n\]*foo \\(\\) at tailcall.c:29\r\n.*" \
+gdb_test "step" "\[^\r\n\]*bar \\(\\) at tailcall.c:24\r\n.*" \
     "step.1"
 gdb_test "finish" "\[^\r\n\]*main \\(\\) at tailcall.c:38\r\n.*" \
     "finish.2"

With this change, the tailcall.exp test now passes on my X86 laptop.
The PowerPC do not change since the test is not supported on PowerPC.

I will post an updated version of the X86 patch with the fixes to the
tailcall test.  It will be version 3.  There are no changes to the
PowerPC patch.

The gdb.btrace/rn-dl-bind.exp test passes with and without my patches. 
I still can not get this test to fail on my system.  

                    Carl
  

On Thu, 2023-01-19 at 15:57 -0800, Carl Love wrote:
> Bruno, Tom:
> 
> 
> On Thu, 2023-01-19 at 08:56 -0800, Carl Love wrote:
> 
<snip>
> 
> Without the patch, when you were in the main program at "answer += 1"
> and did a reverse-next, gdb would go back thru the call to function
> bar
> and thru the call to function foo stopping in main.  That doesn't
> seem
> right to me as you have stepped back thru function foo and bar.  
> 
> With the patch the reverse-next only step back thru function bar and
> stops in function foo at the call to bar, i.e. doesn't continue to
> step
> back thru function bar.
> 
> The following is the tailcall source code with comments showing where
> the reverse-step and reverse-next stop with and without the X86 patch
> applied.  Hopefully the code below is a little easier to follow.
> Initially, the reverse-step and reverse-next tests are executed from
> the line "answer += 1;" in function main.
> 
> 
> static __attribute__ ((noinline)) int
> bar (void)
> {
>   return 42;
> }                    <- reverse-step stops here (with no patch
>                         and with X86 patch)
> 
> static __attribute__ ((noinline)) int
> foo (void)
> {
>   return bar ();     <- reverse-next stops here (patch X86 applied)
> }
> 
> int
> main (void)
> {
>   int answer;
> 
>   answer = foo ();    <- reverse-next tops here (no patch applied,
> stepped
>                          back thru both the bar and foo functions)
>   answer += 1;        <- GDB is here, now issue the reverse-step
>                          and reverse-next commands
> 
>   return answer;
> }
> 
> As a result of the change in the reverse-next instruction, the
> expected
> test results for the tailcall test need to be updated to reflect the
> fact that gdb now stops in function foo not main.
> 
> --- a/gdb/testsuite/gdb.btrace/tailcall.exp
> +++ b/gdb/testsuite/gdb.btrace/tailcall.exp
> @@ -102,9 +102,9 @@ gdb_test "reverse-step" "\[^\r\n\]*main \\(\\) at
> tailcall.c
> :37\r\n.*" \
>      "reverse-step.2"
>  gdb_test "next" "\[^\r\n\]*38.*" \
>      "next.1"
> -gdb_test "reverse-next" "\[^\r\n\]*main \\(\\) at
> tailcall.c:37\r\n.*" \
> +gdb_test "reverse-next" "\[^\r\n\]*foo \\(\\) at
> tailcall.c:29\r\n.*" \
>      "reverse-next.1"
> -gdb_test "step" "\[^\r\n\]*foo \\(\\) at tailcall.c:29\r\n.*" \
> +gdb_test "step" "\[^\r\n\]*bar \\(\\) at tailcall.c:24\r\n.*" \
>      "step.1"
>  gdb_test "finish" "\[^\r\n\]*main \\(\\) at tailcall.c:38\r\n.*" \
>      "finish.2"
> 
> With this change, the tailcall.exp test now passes on my X86 laptop.
> The PowerPC do not change since the test is not supported on PowerPC.
> 
> I will post an updated version of the X86 patch with the fixes to the
> tailcall test.  It will be version 3.  There are no changes to the
> PowerPC patch.
> 
> The gdb.btrace/rn-dl-bind.exp test passes with and without my
> patches. 
> I still can not get this test to fail on my system.  

I have been thinking about this and have decided the above change is
not what we should do.  Basically if we forward step at answer = foo();
we stop at answer += 1;.  Which is the expected behavior for next.  So,
is we are at answer += 1; and do a reverse step it should take us to
answer = foo().  It would be the opposite of the forward next.

I think I have identified the single line in the function call
set_step_info() which caused the regression in gdb.btrace/tailcall.exp.
I have so far run a few experiments and it looks like not doing the 
statement:

 tp->control.step_stack_frame_id = get_stack_frame_id (frame);

eliminates the regression in tailcall.exp. 

So far.  I need to do some more testing but it looks like there is a
couple of regressions on my X86 laptop. It looks like the expected
behavior in both of these tests is for the reverse step over a function
stops at a function call within the function not stepping all the way
back to the call of the function.  Basically, the expected behavior of
these reverse tests is not consistent across tests.  

I am not seeing any PowerPC regression failures with the above change. 
My IBM x86 machine tests are still running.

Anyway, just wanted to give people a heads up that I am pursuing
another fix and that I now think the above fix is not the best
direction to go.

                          Carl
  

Bruno, Tom, Ulrich, GDB developers:

Fixed the regression failure in gdb.btrace/tailcall.exp.  The function
call set_step_info() sets the following values:

  tp->control.step_frame_id = get_frame_id (frame);
  tp->control.step_stack_frame_id = get_stack_frame_id (frame);

  tp->current_symtab = sal.symtab;
  tp->current_line = sal.line;

The regression failure was due to the function updating the value of
tp->control.step_stack_frame_id.  So, the function call was replaced
with the three lines of code to update step_frame_id, current_symtab, 
current_line.  This was done in both places where set_step_info() was
being called.

The gdb.btrace tests require a 5th generation Intel processor to run. 
I have run the regression tests on my X86 laptop with a 5th generation
processor as well as the IBM X86 system, with a pre 5th generation
processor and on PowerPC with no regressions.

I do not see the issue with test gdb.btrace/rn-dl-bind.exp Tom
reported.

Please let me know if this version of the patch is acceptable.  Thanks.

                         Carl 

----------------
X86: reverse-finish fix

PR record/29927  - reverse-finish requires two reverse next instructions to
reach previous source line

Currently on X86, when executing the finish command in reverse, gdb does a
single step from the first instruction in the callee to get back to the
caller.  GDB stops on the last instruction in the source code line where
the call was made.  When stopped at the last instruction of the source code
line, a reverse next or step command will stop at the first instruction
of the same source code line thus requiring two step/next commands to
reach the previous source code line.  It should only require one step/next
command to reach the previous source code line.

By contrast, a reverse next or step command from the first line in a
function stops at the first instruction in the source code line where the
call was made.

This patch fixes the reverse finish command so it will stop at the first
instruction of the source line where the function call was made.  The
behavior on X86 for the reverse-finish command now matches doing a
reverse-next from the beginning of the function.

The proceed_to_finish flag in struct thread_control_state is no longer
used.  This patch removes the declaration, initialization and setting of
the flag.

This patch requires a number of regression tests to be updated.  Test
gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
tests gdb.reverse/amd64-failcall-reverse.exp and
gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
result.  Test gdb.reverse/singlejmp-reverse.exp no longer fails on "no
more reverse-execution history".

The reverse-next command should step all the way back thru a function call.
For example:

   a = foo ();
   b = 1;          <- issuing reverse-next here should stop at the
                      previous line.

Test gdb.reverse/amd64-tailcall-reverse.exp expected the reverse-next
command to stop at a function call in foo ().  That is not the correct
behavior.  GDB now step to the previous line as expected.  The expected
output for the test is updated to the correct behavior.

This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
reverse-finish command when returning from the entry point and from the
body of the function.

The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp
was moved to lib/gdb.exp and renamed cmd_until.

The patch has been tested on X86 and PowerPC to verify no additional
regression failures occured.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29927
---
 gdb/gdbthread.h                               |   4 -
 gdb/infcall.c                                 |   3 -
 gdb/infcmd.c                                  |  37 +++----
 gdb/infrun.c                                  |  45 ++++----
 gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
 .../gdb.reverse/amd64-tailcall-reverse.exp    |   3 +-
 .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
 .../gdb.reverse/finish-reverse-next.exp       | 104 ++++++++++++++++++
 .../gdb.reverse/singlejmp-reverse.exp         |   3 -
 .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++-------
 gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
 gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
 gdb/testsuite/lib/gdb.exp                     |  33 ++++++
 13 files changed, 237 insertions(+), 105 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 11d69fceab0..e4edff2d621 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -150,10 +150,6 @@ struct thread_control_state
      the finished single step.  */
   int trap_expected = 0;
 
-  /* Nonzero if the thread is being proceeded for a "finish" command
-     or a similar situation when return value should be printed.  */
-  int proceed_to_finish = 0;
-
   /* Nonzero if the thread is being proceeded for an inferior function
      call.  */
   int in_infcall = 0;
diff --git a/gdb/infcall.c b/gdb/infcall.c
index e09904f9a35..116605c43ef 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -625,9 +625,6 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
 
   disable_watchpoints_before_interactive_call_start ();
 
-  /* We want to print return value, please...  */
-  call_thread->control.proceed_to_finish = 1;
-
   try
     {
       /* Infcalls run synchronously, in the foreground.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 0497ad05091..5d3221e8b90 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm *sm)
 
   sal = find_pc_line (func_addr, 0);
 
-  tp->control.proceed_to_finish = 1;
-  /* Special case: if we're sitting at the function entry point,
-     then all we need to do is take a reverse singlestep.  We
-     don't need to set a breakpoint, and indeed it would do us
-     no good to do so.
-
-     Note that this can only happen at frame #0, since there's
-     no way that a function up the stack can have a return address
-     that's equal to its entry point.  */
+  frame_info_ptr frame = get_selected_frame (nullptr);
 
   if (sal.pc != pc)
     {
-      frame_info_ptr frame = get_selected_frame (nullptr);
       struct gdbarch *gdbarch = get_frame_arch (frame);
 
       /* Set a step-resume at the function's entry point.  Once that's
@@ -1743,16 +1734,27 @@ finish_backward (struct finish_command_fsm *sm)
       sr_sal.pspace = get_frame_program_space (frame);
       insert_step_resume_breakpoint_at_sal (gdbarch,
 					    sr_sal, null_frame_id);
-
-      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
     }
   else
     {
-      /* We're almost there -- we just need to back up by one more
-	 single-step.  */
-      tp->control.step_range_start = tp->control.step_range_end = 1;
-      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
+      /* We are exactly at the function entry point.  Note that this
+	 can only happen at frame #0.
+
+	 When setting a step range, need to setup the current_symtab and
+	 current_line.  Do not change the step_stack_frame_id as this
+	 will cause the reverse-next command to stop in the wrong spot.  */
+      struct symtab_and_line stop_pc_sal;
+      stop_pc_sal = find_pc_line (pc, 0);
+      tp->control.step_frame_id = get_frame_id (frame);
+      tp->current_symtab = stop_pc_sal.symtab;
+      tp->current_line = stop_pc_sal.line;
+
+      /* Return using a step range so we will keep stepping back
+	 to the first instruction in the source code line.  */
+      tp->control.step_range_start = sal.pc;
+      tp->control.step_range_end = sal.pc;
     }
+  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
 
 /* finish_forward -- helper function for finish_command.  FRAME is the
@@ -1778,9 +1780,6 @@ finish_forward (struct finish_command_fsm *sm, frame_info_ptr frame)
 
   set_longjmp_breakpoint (tp, frame_id);
 
-  /* We want to print return value, please...  */
-  tp->control.proceed_to_finish = 1;
-
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 181d961d80d..183110a049a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct thread_info *tp)
 
   tp->control.stop_step = 0;
 
-  tp->control.proceed_to_finish = 0;
-
   tp->control.stepping_command = 0;
 
   /* Discard any remaining commands or status from previous stop.  */
@@ -6737,31 +6735,30 @@ process_event_stop_test (struct execution_control_state *ecs)
 
     case BPSTAT_WHAT_STEP_RESUME:
       infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
-
       delete_step_resume_breakpoint (ecs->event_thread);
-      if (ecs->event_thread->control.proceed_to_finish
-	  && execution_direction == EXEC_REVERSE)
-	{
-	  struct thread_info *tp = ecs->event_thread;
-
-	  /* We are finishing a function in reverse, and just hit the
-	     step-resume breakpoint at the start address of the
-	     function, and we're almost there -- just need to back up
-	     by one more single-step, which should take us back to the
-	     function call.  */
-	  tp->control.step_range_start = tp->control.step_range_end = 1;
-	  keep_going (ecs);
-	  return;
-	}
       fill_in_stop_func (gdbarch, ecs);
-      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
-	  && execution_direction == EXEC_REVERSE)
+
+      if (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
-	     the function.  Go back to single-stepping, which should
-	     take us back to the function call.  */
-	  ecs->event_thread->stepping_over_breakpoint = 1;
+	  struct thread_info *tp = ecs->event_thread;
+	  /* We are finishing a function in reverse or stepping over a function
+	     call in reverse, and just hit the step-resume breakpoint at the
+	     start address of the function, and we're almost there -- just need
+	     to back up to the function call.  */
+
+	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), 0);
+
+	  /* When setting a step range, need to setup the current_symtab and
+	     current_line.  Do not change the step_stack_frame_id as this
+	     will cause the reverse-next command to stop in the wrong spot.  */
+	  tp->control.step_frame_id = get_frame_id (frame);
+	  tp->current_symtab = stop_pc_sal.symtab;
+	  tp->current_line = stop_pc_sal.line;
+
+	  /* Return using a step range so we will keep stepping back to the
+	     first instruction in the source code line.  */
+	  tp->control.step_range_start = ecs->stop_func_start;
+	  tp->control.step_range_end = ecs->stop_func_start;
 	  keep_going (ecs);
 	  return;
 	}
diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
index d631beb17c8..30635ab1754 100644
--- a/gdb/testsuite/gdb.mi/mi-reverse.exp
+++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
@@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} {
 	"basics.c" $line_main_callme_1 "" \
 	"reverse finish from callme"
 
-    # Test exec-reverse-next
-    #   It takes two steps to get back to the previous line,
-    #   as the first step moves us to the start of the current line,
-    #   and the one after that moves back to the previous line.
-
-    mi_execute_to "exec-next --reverse 2" \
+    mi_execute_to "exec-next --reverse" \
  	"end-stepping-range" "main" "" \
  	"basics.c" $line_main_hello "" \
- 	"reverse next to get over the call to do_nothing"
+	"reverse next to get over the call to do_nothing"
 
     # Test exec-reverse-step
 
diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
index 52a87faabf7..7d441dbb7a9 100644
--- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
@@ -44,6 +44,5 @@ if [supports_process_record] {
 gdb_test "next" {f \(\);} "next to f"
 gdb_test "next" {v = 3;} "next to v = 3"
 
-# FAIL was:
-# 29        g ();
+# Reverse step back to f ().
 gdb_test "reverse-next" {f \(\);}
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
new file mode 100644
index 00000000000..f90ecbb93cb
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2023 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/>.  */
+
+/* The reverse finish command should return from a function and stop on
+   the first instruction of the source line where the function call is made.
+   Specifically, the behavior should match doing a reverse next from the
+   first instruction in the function.  GDB should only require one reverse
+   step or next statement to reach the previous source code line.
+
+   This test verifies the fix for gdb bugzilla:
+
+   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+*/
+
+int
+function1 (int a, int b)   // FUNCTION1
+{
+  int ret = 0;
+
+  ret = a + b;
+  return ret;
+}
+
+int
+main(int argc, char* argv[])
+{
+  int a, b;
+
+  a = 1;
+  b = 5;
+
+  function1 (a, b);   // CALL FUNCTION
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
new file mode 100644
index 00000000000..63305c109e1
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -0,0 +1,104 @@
+# Copyright 2008-2023 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/>.  */
+
+# This file is part of the GDB testsuite.  It tests reverse stepping.
+# Lots of code borrowed from "step-test.exp".
+
+# The reverse finish command should return from a function and stop on
+# the first instruction of the source line where the function call is made.
+# Specifically, the behavior should match doing a reverse next from the
+# first instruction in the function.  GDB should only take one reverse step
+# or next statement to reach the previous source code line.
+
+# This testcase verifies the reverse-finish command stops at the first
+# instruction in the source code line where the function was called.  There
+# are two scenarios that must be checked:
+#   1) gdb is at the entry point instruction for the function
+#   2) gdb is in the body of the function.
+
+# This test verifies the fix for gdb bugzilla:
+#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+
+if ![supports_reverse] {
+    return
+}
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+runto_main
+set target_remote [gdb_is_target_remote]
+
+if [supports_process_record] {
+    # Activate process record/replay.
+    gdb_test_no_output "record" "turn on process record for test1"
+}
+
+
+### TEST 1: reverse finish from the entry point instruction in
+### function1.
+
+# Set breakpoint at call to function1 in main.
+set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
+gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into function" \
+    ".*$srcfile:$bp_FUNCTION\r\n.*"
+
+# stepi until we see "{" indicating we entered function1
+repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
+
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
+    "reverse-finish function1 "
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.   If GDB stops at the last instruction in the source code line
+# it will take two reverse next instructions to get to the previous source
+# line.
+gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
+
+# Clear the recorded log.
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test1"
+gdb_test_no_output "record" "turn on process record for test2"
+
+
+### TEST 2: reverse finish from the body of function1.
+
+# Set breakpoint at call to function1 in main.
+gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "at function1 entry point instruction to step to body of function" \
+    ".*$srcfile:$bp_FUNCTION\r\n.*"
+
+# do a step instruction to get to the body of the function
+gdb_test "step" ".*int ret = 0;.*" "step test 1"
+
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
+    "reverse-finish function1 call from function body"
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.
+gdb_test "reverse-next" ".*b = 5;.*" \
+    "reverse next at b = 5, from function body"
diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
index 1ca7c2ce559..e94bcdbe52e 100644
--- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
@@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3"
 # {
 gdb_test "reverse-step" {nodebug \(\);}
 
-# FAIL was:
-# No more reverse-execution history.
-# {
 gdb_test "reverse-next" {f \(\);}
diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
index ad637899e5b..b82e5663bd5 100644
--- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
+++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
@@ -39,39 +39,6 @@ if { ![runto_main] } {
     return -1
 }
 
-# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
-#
-#  COMMAND is a stepping command
-#  CURRENT is a string matching the current location
-#  TARGET  is a string matching the target location
-#  TEST    is the test name
-#
-# The function issues repeated COMMANDs as long as the location matches
-# CURRENT up to a maximum of 100 steps.
-#
-# TEST passes if the resulting location matches TARGET and fails
-# otherwise.
-#
-proc step_until { command current target test } {
-    global gdb_prompt
-
-    set count 0
-    gdb_test_multiple "$command" "$test" {
-        -re "$current.*$gdb_prompt $" {
-            incr count
-            if { $count < 100 } {
-                send_gdb "$command\n"
-                exp_continue
-            } else {
-                fail "$test"
-            }
-        }
-        -re "$target.*$gdb_prompt $" {
-            pass "$test"
-        }
-    }
-}
-
 gdb_test_no_output "record"
 gdb_test "next" ".*" "record trace"
 
@@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \
     "reverse-step through thunks and over inc"
 
 # We can use instruction stepping to step into thunks.
-step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
-step_until "stepi" "indirect_thunk" "inc" \
+repeat_cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
+repeat_cmd_until "stepi" "indirect_thunk" "inc" \
     "stepi out of call thunk into inc"
 set alphanum_re "\[a-zA-Z0-9\]"
 set pic_thunk_re  "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re* \\(\\)"
-step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
-step_until "stepi" "return_thunk" "apply" \
+repeat_cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
+repeat_cmd_until "stepi" "return_thunk" "apply" \
     "stepi out of return thunk back into apply"
 
-step_until "reverse-stepi" "apply" "return_thunk" \
+repeat_cmd_until "reverse-stepi" "apply" "return_thunk" \
     "reverse-stepi into return thunk"
-step_until "reverse-stepi" "return_thunk" "inc" \
+repeat_cmd_until "reverse-stepi" "return_thunk" "inc" \
     "reverse-stepi out of return thunk into inc"
-step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
+repeat_cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
     "reverse-stepi into call thunk"
-step_until "reverse-stepi" "indirect_thunk" "apply" \
+repeat_cmd_until "reverse-stepi" "indirect_thunk" "apply" \
     "reverse-stepi out of call thunk into apply"
diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp b/gdb/testsuite/gdb.reverse/until-precsave.exp
index 0c2d7537cd6..777aec94ac1 100644
--- a/gdb/testsuite/gdb.reverse/until-precsave.exp
+++ b/gdb/testsuite/gdb.reverse/until-precsave.exp
@@ -142,7 +142,7 @@ gdb_test "advance marker2" \
 # Finish out to main scope (backward)
 
 gdb_test "finish" \
-    " in main .*$srcfile:$bp_location20.*" \
+    "main .*$srcfile:$bp_location20.*" \
     "reverse-finish from marker2"
 
 # Advance backward to last line of factorial (outer invocation)
diff --git a/gdb/testsuite/gdb.reverse/until-reverse.exp b/gdb/testsuite/gdb.reverse/until-reverse.exp
index 23fc881dbf2..3a05953329f 100644
--- a/gdb/testsuite/gdb.reverse/until-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/until-reverse.exp
@@ -113,7 +113,7 @@ gdb_test "advance marker2" \
 # Finish out to main scope (backward)
 
 gdb_test "finish" \
-    " in main .*$srcfile:$bp_location20.*" \
+    "main .*$srcfile:$bp_location20.*" \
     "reverse-finish from marker2"
 
 # Advance backward to last line of factorial (outer invocation)
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index c41d4698d66..234c21a04ea 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9301,6 +9301,39 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
     }
 }
 
+# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
+#
+#  COMMAND is a stepping command
+#  CURRENT is a string matching the current location
+#  TARGET  is a string matching the target location
+#  TEST    is the test name
+#
+# The function issues repeated COMMANDs as long as the location matches
+# CURRENT up to a maximum of 100 steps.
+#
+# TEST passes if the resulting location matches TARGET and fails
+# otherwise.
+
+proc repeat_cmd_until { command current target test } {
+    global gdb_prompt
+
+    set count 0
+    gdb_test_multiple "$command" "$test" {
+	-re "$current.*$gdb_prompt $" {
+	    incr count
+	    if { $count < 100 } {
+		send_gdb "$command\n"
+		exp_continue
+	    } else {
+		fail "$test"
+	    }
+	}
+	-re "$target.*$gdb_prompt $" {
+	    pass "$test"
+	}
+    }
+}
+
 # Check if the compiler emits epilogue information associated
 # with the closing brace or with the last statement line.
 #
  

Bruno, Tom, Ulrich, GDB maintainers:

This patch is functionally the same as version 2.  It was refreshed to
properly apply on the X86 patch.  The X86 patch changed the diff
locations slightly.

I have run the regression tests on my X86 laptop with a 5th generation
processor as well as the IBM X86 system, with a pre 5th generation
processor and on PowerPC with no regressions.

Please let me know if this version of the patch is acceptable.  Thanks.

                         Carl 

---------------------------------------------
PowerPC: fix for gdb.reverse/finish-precsave.exp and  gdb.reverse/finish-reverse.exp

PR record/29927 - reverse-finish requires two reverse next instructions to
reach previous source line

PowerPC uses two entry points called the local entry point (LEP) and the
global entry point (GEP).  Normally the LEP is used when calling a
function.  However, if the table of contents (TOC) value in register 2 is
not valid the GEP is called to setup the TOC before execution continues at
the LEP.  When executing in reverse, the function finish_backward sets the
break point at the alternate entry point (GEP).  However if the forward
execution enters via the normal entry point (LEP), the reverse execution
never sees the break point at the GEP of the function.  Reverse execution
continues until the next break point is encountered or the end of the
recorded log is reached causing gdb to stop at the wrong place.

This patch adds a new address to struct execution_control_state to hold the
address of the alternate function start address, known as the GEP on
PowerPC.  The finish_backwards function is updated.  If the stopping point
is between the two entry points (the LEP and GEP on PowerPC), the stepping
range is set to execute back to the alternate entry point (GEP on PowerPC).
Otherwise, a breakpoint is inserted at the normal entry point (LEP on
PowerPC).

Function process_event_stop_test checks uses a stepping range to stop
execution in the caller at the first instruction of the source code line.
Note, on systems that only support one entry point, the address of the two
entry points are the same.

Test finish-reverse-next.exp is updated to include tests for the
reverse-finish command when the function is entered via the normal entry
point (i.e. the LEP) and the alternate entry point (i.e. the GEP).

The patch has been tested on X86 and PowerPC with no regressions.
---
 gdb/infcmd.c                                  | 40 +++++---
 gdb/infrun.c                                  | 16 +++-
 .../gdb.reverse/finish-reverse-next.c         | 39 +++++++-
 .../gdb.reverse/finish-reverse-next.exp       | 96 ++++++++++++++++---
 4 files changed, 160 insertions(+), 31 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5d3221e8b90..63e245f7de9 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1722,22 +1722,25 @@ finish_backward (struct finish_command_fsm *sm)
   sal = find_pc_line (func_addr, 0);
 
   frame_info_ptr frame = get_selected_frame (nullptr);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  CORE_ADDR alt_entry_point = sal.pc;
+  CORE_ADDR entry_point = alt_entry_point;
 
-  if (sal.pc != pc)
+  if (gdbarch_skip_entrypoint_p (gdbarch))
     {
-      struct gdbarch *gdbarch = get_frame_arch (frame);
-
-      /* Set a step-resume at the function's entry point.  Once that's
-	 hit, we'll do one more step backwards.  */
-      symtab_and_line sr_sal;
-      sr_sal.pc = sal.pc;
-      sr_sal.pspace = get_frame_program_space (frame);
-      insert_step_resume_breakpoint_at_sal (gdbarch,
-					    sr_sal, null_frame_id);
+      /* Some architectures, like PowerPC use local and global entry points.
+	 There is only one Entry Point (GEP = LEP) for other architectures.
+	 The GEP is an alternate entry point.  The LEP is the normal entry
+	 point.  The value of entry_point was initialized to the alternate
+	 entry point (GEP).  It will be adjusted if the normal entry point
+	 (LEP) was used.  */
+       entry_point = gdbarch_skip_entrypoint (gdbarch, entry_point);
     }
-  else
+
+  if (alt_entry_point <= pc && pc <= entry_point)
     {
-      /* We are exactly at the function entry point.  Note that this
+      /* We are exactly at the function entry point, or between the entry
+	 point on platforms that have two (like PowerPC).  Note that this
 	 can only happen at frame #0.
 
 	 When setting a step range, need to setup the current_symtab and
@@ -1751,8 +1754,17 @@ finish_backward (struct finish_command_fsm *sm)
 
       /* Return using a step range so we will keep stepping back
 	 to the first instruction in the source code line.  */
-      tp->control.step_range_start = sal.pc;
-      tp->control.step_range_end = sal.pc;
+      tp->control.step_range_start = alt_entry_point;
+      tp->control.step_range_end = entry_point;
+    }
+  else
+    {
+      symtab_and_line sr_sal;
+      /* Set a step-resume at the function's entry point.  */
+      sr_sal.pc = entry_point;
+      sr_sal.pspace = get_frame_program_space (frame);
+      insert_step_resume_breakpoint_at_sal (gdbarch,
+					    sr_sal, null_frame_id);
     }
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 183110a049a..a911aec8568 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1868,6 +1868,7 @@ struct execution_control_state
 
   struct target_waitstatus ws;
   int stop_func_filled_in = 0;
+  CORE_ADDR stop_func_alt_start = 0;
   CORE_ADDR stop_func_start = 0;
   CORE_ADDR stop_func_end = 0;
   const char *stop_func_name = nullptr;
@@ -4663,6 +4664,12 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 				    &block);
       ecs->stop_func_name = gsi == nullptr ? nullptr : gsi->print_name ();
 
+      /* PowerPC functions have a Local Entry Point and a Global Entry
+	 Point.  There is only one Entry Point (GEP = LEP) for other
+	 architectures.  Save the alternate entry point address (GEP) for
+	 use later.  */
+      ecs->stop_func_alt_start = ecs->stop_func_start;
+
       /* The call to find_pc_partial_function, above, will set
 	 stop_func_start and stop_func_end to the start and end
 	 of the range containing the stop pc.  If this range
@@ -4679,6 +4686,9 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 	    += gdbarch_deprecated_function_start_offset (gdbarch);
 
 	  if (gdbarch_skip_entrypoint_p (gdbarch))
+	    /* The PowerPC architecture uses two entry points.  Stop at the
+	       regular entry point (LEP on PowerPC) initially.  Will setup a
+	       breakpoint for the alternate entry point (GEP) later.  */
 	    ecs->stop_func_start
 	      = gdbarch_skip_entrypoint (gdbarch, ecs->stop_func_start);
 	}
@@ -6757,7 +6767,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  /* Return using a step range so we will keep stepping back to the
 	     first instruction in the source code line.  */
-	  tp->control.step_range_start = ecs->stop_func_start;
+	  tp->control.step_range_start = ecs->stop_func_alt_start;
 	  tp->control.step_range_end = ecs->stop_func_start;
 	  keep_going (ecs);
 	  return;
@@ -6894,8 +6904,10 @@ process_event_stop_test (struct execution_control_state *ecs)
 	 (unless it's the function entry point, in which case
 	 keep going back to the call point).  */
       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
+	  && (stop_pc < ecs->stop_func_alt_start
+	      || stop_pc > ecs->stop_func_start)
 	  && execution_direction == EXEC_REVERSE)
 	end_stepping_range (ecs);
       else
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
index f90ecbb93cb..0347906961d 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
@@ -24,11 +24,37 @@
    This test verifies the fix for gdb bugzilla:
 
    https://sourceware.org/bugzilla/show_bug.cgi?id=29927
-*/
+
+   PowerPC supports two entry points to a function.  The normal entry point
+   is called the local entry point (LEP).  The alternat entry point is called
+   the global entry point (GEP).  The GEP is only used if the table of
+   contents (TOC) value stored in register r2 needs to be setup prior to
+   execution starting at the LEP.  A function call via a function pointer
+   will entry via the GEP.  A normal function call will enter via the LEP.
+
+   This test has been expanded to include tests to verify the reverse-finish
+   command works properly if the function is called via the GEP.  The original
+   test only verified the reverse-finish command for a normal call that used
+   the LEP.  */
 
 int
 function1 (int a, int b)   // FUNCTION1
 {
+  /* The assembly code for this function when compiled for PowerPC is as
+     follows:
+
+     0000000010000758 <function1>:
+     10000758:	02 10 40 3c 	lis     r2,4098        <- GEP
+     1000075c:	00 7f 42 38 	addi    r2,r2,32512
+     10000760:	a6 02 08 7c 	mflr    r0             <- LEP
+     10000764:	10 00 01 f8 	std     r0,16(r1)
+     ....
+
+     When the function is called on PowerPC with function1 (a, b) the call
+     enters at the Local Entry Point (LEP).  When the function is called via
+     a function pointer, the Global Entry Point (GEP) for function1 is used.
+     The GEP sets up register 2 before reaching the LEP.
+  */
   int ret = 0;
 
   ret = a + b;
@@ -39,10 +65,19 @@ int
 main(int argc, char* argv[])
 {
   int a, b;
+  int (*funp) (int, int) = &function1;
+
+  /* Call function via Local Entry Point (LEP).  */
 
   a = 1;
   b = 5;
 
-  function1 (a, b);   // CALL FUNCTION
+  function1 (a, b);   // CALL VIA LEP
+
+  /* Call function via Global Entry Point (GEP).  */
+  a = 10;
+  b = 50;
+
+  funp (a, b);        // CALL VIA GEP
   return 0;
 }
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
index 63305c109e1..a9c895dfcd4 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -31,6 +31,16 @@
 # This test verifies the fix for gdb bugzilla:
 #   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
 
+# PowerPC supports two entry points to a function.  The normal entry point
+# is called the local entry point (LEP).  The alternat entry point is called
+# the global entry point (GEP).  A function call via a function pointer
+# will entry via the GEP.  A normal function call will enter via the LEP.
+#
+# This test has been expanded to include tests to verify the reverse-finish
+# command works properly if the function is called via the GEP.  The original
+# test only verified the reverse-finish command for a normal call that used
+# the LEP.
+
 if ![supports_reverse] {
     return
 }
@@ -50,30 +60,30 @@ if [supports_process_record] {
 }
 
 
-### TEST 1: reverse finish from the entry point instruction in
-### function1.
+### TEST 1: reverse finish from the entry point instruction (LEP) in
+### function1 when called using the normal entry point (LEP).
 
 # Set breakpoint at call to function1 in main.
-set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
-gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
 
 # Continue to break point at function1 call in main.
 gdb_continue_to_breakpoint \
     "stopped at function1 entry point instruction to stepi into function" \
-    ".*$srcfile:$bp_FUNCTION\r\n.*"
+    ".*$srcfile:$bp_LEP_test\r\n.*"
 
 # stepi until we see "{" indicating we entered function1
-repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
+repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call"
 
-gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
-    "reverse-finish function1 "
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish function1 LEP call from LEP "
 
 # Check to make sure we stopped at the first instruction in the source code
 # line.  It should only take one reverse next command to get to the previous
 # source line.   If GDB stops at the last instruction in the source code line
 # it will take two reverse next instructions to get to the previous source
 # line.
-gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
+gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from LEP"
 
 # Clear the recorded log.
 gdb_test "record stop"  "Process record is stopped.*" \
@@ -84,21 +94,81 @@ gdb_test_no_output "record" "turn on process record for test2"
 ### TEST 2: reverse finish from the body of function1.
 
 # Set breakpoint at call to function1 in main.
-gdb_breakpoint $srcfile:$bp_FUNCTION temporary
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
 
 # Continue to break point at function1 call in main.
 gdb_continue_to_breakpoint \
     "at function1 entry point instruction to step to body of function" \
-    ".*$srcfile:$bp_FUNCTION\r\n.*"
+    ".*$srcfile:$bp_LEP_test\r\n.*"
 
 # do a step instruction to get to the body of the function
 gdb_test "step" ".*int ret = 0;.*" "step test 1"
 
-gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
-    "reverse-finish function1 call from function body"
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish function1 LEP call from function body"
 
 # Check to make sure we stopped at the first instruction in the source code
 # line.  It should only take one reverse next command to get to the previous
 # source line.
 gdb_test "reverse-next" ".*b = 5;.*" \
     "reverse next at b = 5, from function body"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test2"
+gdb_test_no_output "record" "turn on process record for test3"
+
+
+### TEST 3: reverse finish from the alternate entry point instruction (GEP) in
+### function1 when called using the alternate entry point (GEP).
+
+# Set breakpoint at call to funp in main.
+set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into funp" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# stepi until we see "{" indicating we entered function.
+repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
+
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "function1 GEP call call from GEP"
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.  If GDB stops at the last instruction in the source code line
+# it will take two reverse next instructions to get to the previous source
+# line.
+gdb_test "reverse-next" ".*b = 50;.*" "reverse next at b = 50, call from GEP"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test3"
+gdb_test_no_output "record" "turn on process record for test4"
+
+
+### TEST 4: reverse finish from the body of function 1 when calling using the
+### alternate entrypoint (GEP).
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call.
+gdb_continue_to_breakpoint \
+    "at function1 entry point instruction to step to body of funp call" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# Step into body of funp, called via GEP.
+gdb_test "step" ".*int ret = 0;.*" "step test 2"
+
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "reverse-finish function1 GEP call, from function body  "
+
+# Check to make sure we stopped at the first instruction in the source code
+# line.  It should only take one reverse next command to get to the previous
+# source line.  If GDB stops at the last instruction in the source code line
+# it will take two reverse next instructions to get to the previous source
+# line.
+gdb_test "reverse-next" ".*b = 50;.*" \
+    "reverse next at b = 50 from function body"