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

Bruno Larsen via Gdb-patches
2023-01-16T12:32:11Z

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