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

[Bruno Larsen <blarsen@redhat.com>]

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!

-- 
Cheers,
Bruno

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