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

[Carl Love <cel@us.ibm.com>]

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.
> >   #
> 
>