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

Bruno Larsen via Gdb-patches
2023-01-13T15:56:20Z

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
> PowerPC. The reverse-finish command now correctly stops at the first
> instruction in the source code line of the caller.
>
> The patch adds tests for calling a function via the GEP to the new test
> gdb.reverse/finish-reverse-next.exp.
>
> Please let me know if you have any comments on the patch. Thanks.
I'm not all that familiar with PowerPC ABI and I dont have a computer
handy to test that the patch fixes the problem, but the logic seems
sound. Just a few nits inlined.
>
> Carl
>
> --------------------------------------------------------------
> PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp
>
> PowerPC uses two entry points called the local entry point (LEP) and the
> global entry point (GEP). Normally the LEP is used when calling a
> function. However, if the table of contents (TOC) value in
> register 3 is not valid the GEP is called to setup the TOC before execution
> continues at the LEP. When executing in reverse, the function
> finish_backward sets the break point at the alternate entry point (GEP).
> However if the forward execution enters via the normal entry point (LEP),
> the reverse execution never sees the break point at the GEP of the
> function. Reverse execution continues until the next break point is
> encountered or the end of the recorded log is reached causing gdb to stop
> at the wrong place.
>
> This patch adds a new address to struct execution_control_state to hold the
> address of the alternate function start address, known as the GEP on
> PowerPC. The finish_backwards function is updated. If the stopping point
> is between the two entry points (the LEP and GEP on PowerPC) , the stepping
> range is set to execute back to the alternate entry point (GEP on PowerPC).
> Otherwise, a breakpoint is inserted at the normal entry point (LEP on
> PowerPC).
>
> Function process_event_stop_test checks uses a stepping range to stop
> execution in the caller at the first instruction of the source code line.
> Note, on systems that only support one entry point, the address of the two
> entry points are the same.
>
> Test finish-reverse-next.exp is updated to include tests for the
> reverse-finish command when the function is entered via the normal entry
> point (i.e. the LEP) and the alternate entry point (i.e. the GEP).
>
> The patch has been tested on X86 and PowerPC with no regressions.
> ---
> gdb/infcmd.c | 41 ++++---
> gdb/infrun.c | 21 +++-
> .../gdb.reverse/finish-reverse-next.c | 41 ++++++-
> .../gdb.reverse/finish-reverse-next.exp | 107 +++++++++++++++---
> 4 files changed, 175 insertions(+), 35 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9c42efeae8d..8c30af448ce 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1722,22 +1722,28 @@ finish_backward (struct finish_command_fsm *sm)
> sal = find_pc_line (func_addr, 0);
>
> frame_info_ptr frame = get_selected_frame (nullptr);
> + struct gdbarch *gdbarch = get_frame_arch (frame);
> + CORE_ADDR alt_entry_point = sal.pc;
> + CORE_ADDR entry_point = alt_entry_point;
>
> - if (sal.pc != pc)
> + if (gdbarch_skip_entrypoint_p (gdbarch))
> {
> - struct gdbarch *gdbarch = get_frame_arch (frame);
> + /* Some architectures, like PowerPC use local and global entry
> + points. There is only one Entry Point (GEP = LEP) for other
> + architectures. The GEP is an alternate entry point that is used
> + setup the table of contents (TOC) in register r2 before execution
> + continues at the LEP. The LEP is the normal entry point.
I don't think an explanation on why there are 2 entry points is
necessary.  just mentioning that both exist is enough IMHO.
> + The value of entry_point was initialized to the alternate entry
> + point (GEP). It will be adjusted if the normal entry point
> + (LEP) was used. */
> + entry_point = gdbarch_skip_entrypoint (gdbarch, entry_point);
>
> - /* Set a step-resume at the function's entry point. Once that's
> - hit, we'll do one more step backwards. */
> - symtab_and_line sr_sal;
> - sr_sal.pc = sal.pc;
> - sr_sal.pspace = get_frame_program_space (frame);
> - insert_step_resume_breakpoint_at_sal (gdbarch,
> - sr_sal, null_frame_id);
> }
> - else
> +
> + if (alt_entry_point <= pc && pc <= entry_point)
> {
> - /* We are exactly at the function entry point. Note that this
> + /* We are exactly at the function entry point, or between the entry
> + point on platforms that have two (like PowerPC). Note that this
> can only happen at frame #0.
>
> When setting a step range, need to call set_step_info
> @@ -1746,8 +1752,17 @@ finish_backward (struct finish_command_fsm *sm)
>
> /* Return using a step range so we will keep stepping back
> to the first instruction in the source code line. */
> - tp->control.step_range_start = sal.pc;
> - tp->control.step_range_end = sal.pc;
> + tp->control.step_range_start = alt_entry_point;
> + tp->control.step_range_end = alt_entry_point;
I'm hesitant in this part. What if there is one instruction between the
GEP and the LEP and the inferior happens to be stopped there? Could this
happen? Like I said, I'm not familiar with the PPC abi, so excuse me if
the answer is obvious, but it seems to me that the step_range_end could
be set to the entry_point and we'd avoid this possibility.
> + }
> + else
> + {
> + symtab_and_line sr_sal;
> + /* Set a step-resume at the function's entry point. */
> + sr_sal.pc = entry_point;
> + sr_sal.pspace = get_frame_program_space (frame);
> + insert_step_resume_breakpoint_at_sal (gdbarch,
> + sr_sal, null_frame_id);
> }
> proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> }
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 8ed538ea9ec..89423556ec0 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1868,6 +1868,7 @@ struct execution_control_state
>
> struct target_waitstatus ws;
> int stop_func_filled_in = 0;
> + CORE_ADDR stop_func_alt_start = 0;
> CORE_ADDR stop_func_start = 0;
> CORE_ADDR stop_func_end = 0;
> const char *stop_func_name = nullptr;
> @@ -4663,6 +4664,14 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> &block);
> ecs->stop_func_name = gsi == nullptr ? nullptr : gsi->print_name ();
>
> + /* PowerPC functions have a Local Entry Point and a Global Entry
> + Point. There is only one Entry Point (GEP = LEP) for other
> + architectures. The GEP is an alternate entry point that is used
> + setup the table of contents (TOC) in register r2 before execution
> + continues at the LEP. Save the alternate entry point address for
Ditto here about explaining LEP and GEP. Just saying that GEP is the
alternate is probably enough.
> + use later. */
> + ecs->stop_func_alt_start = ecs->stop_func_start;
> +
> /* The call to find_pc_partial_function, above, will set
> stop_func_start and stop_func_end to the start and end
> of the range containing the stop pc. If this range
> @@ -4679,6 +4688,9 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> += gdbarch_deprecated_function_start_offset (gdbarch);
>
> if (gdbarch_skip_entrypoint_p (gdbarch))
> + /* The PowerPC architecture uses two entry points. Stop at the
> + regular entry point (LEP on PowerPC) initially. Will setup a
> + breakpoint for the alternate entry point (GEP) later. */
> ecs->stop_func_start
> = gdbarch_skip_entrypoint (gdbarch, ecs->stop_func_start);
> }
> @@ -6738,8 +6750,7 @@ process_event_stop_test (struct execution_control_state *ecs)
> delete_step_resume_breakpoint (ecs->event_thread);
> fill_in_stop_func (gdbarch, ecs);
>
> - if (execution_direction == EXEC_REVERSE
> - && ecs->event_thread->stop_pc () == ecs->stop_func_start)
> + if (execution_direction == EXEC_REVERSE)
Why is this change not in the previous patch?
> {
> struct thread_info *tp = ecs->event_thread;
> stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), 0);
> @@ -6755,7 +6766,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>
> Return using a step range so we will keep stepping back to the
> first instruction in the source code line. */
> - tp->control.step_range_start = ecs->stop_func_start;
> + tp->control.step_range_start = ecs->stop_func_alt_start;
> tp->control.step_range_end = ecs->stop_func_start;
> keep_going (ecs);
> return;
> @@ -6892,8 +6903,10 @@ process_event_stop_test (struct execution_control_state *ecs)
> (unless it's the function entry point, in which case
> keep going back to the call point). */
> CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +
> if (stop_pc == ecs->event_thread->control.step_range_start
> - && stop_pc != ecs->stop_func_start
> + && (stop_pc < ecs->stop_func_alt_start
> + || stop_pc > ecs->stop_func_start)
> && execution_direction == EXEC_REVERSE)
> end_stepping_range (ecs);
> else
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> index 42e41b5a2e0..55f81d2bc01 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> @@ -1,4 +1,4 @@
> -/* This testcase is part of GDB, the GNU debugger.
> +j/* This testcase is part of GDB, the GNU debugger.
>
> Copyright 2012-2022 Free Software Foundation, Inc.
>
> @@ -24,11 +24,37 @@
> This test verifies the fix for gdb bugzilla:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> -*/
> +
> + PowerPC supports two entry points to a function. The normal entry point
> + is called the local entry point (LEP). The alternat entry point is called
> + the global entry point (GEP). The GEP is only used if the table of
> + contents (TOC) value stored in register r2 needs to be setup prior to
> + execution starting at the LEP. A function call via a function pointer
> + will entry via the GEP. A normal function call will enter via the LEP.
> +
> + This test has been expanded to include tests to verify the reverse-finish
> + command works properly if the function is called via the GEP. The original
> + test only verified the reverse-finish command for a normal call that used
> + the LEP. */
>
> int
> function1 (int a, int b) // FUNCTION1
> {
> + /* The assembly code for this function when compiled for PowerPC is as
> + follows:
> +
> + 0000000010000758 <function1>:
> + 10000758: 02 10 40 3c lis r2,4098 <- GEP
> + 1000075c: 00 7f 42 38 addi r2,r2,32512
> + 10000760: a6 02 08 7c mflr r0 <- LEP
> + 10000764: 10 00 01 f8 std r0,16(r1)
> + ....
> +
> + When the function is called on PowerPC with function1 (a, b) the call
> + enters at the Local Entry Point (LEP). When the function is called via
> + a function pointer, the Global Entry Point (GEP) for function1 is used.
> + The GEP sets up register 2 before reaching the LEP.
> + */
> int ret = 0;
>
> ret = a + b;
> @@ -39,10 +65,19 @@ int
> main(int argc, char* argv[])
> {
> int a, b;
> + int (*funp) (int, int) = &function1;
> +
> + /* Call function via Local Entry Point (LEP). */
>
> a = 1;
> b = 5;
>
> - function1 (a, b); // CALL FUNCTION
> + function1 (a, b); // CALL VIA LEP
> +
> + /* Call function via Global Entry Point (GEP). */
> + a = 10;
> + b = 50;
> +
> + funp (a, b); // CALL VIA GEP
> return 0;
> }
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> index 7880de10ffc..fbc024b48b9 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -31,6 +31,18 @@
> # This test verifies the fix for gdb bugzilla:
> # https://sourceware.org/bugzilla/show_bug.cgi?id=29927
>
> +# PowerPC supports two entry points to a function. The normal entry point
> +# is called the local entry point (LEP). The alternat entry point is called
> +# the global entry point (GEP). The GEP is only used if the table of
> +# contents (TOC) value stored in register r2 needs to be setup prior to
> +# execution starting at the LEP. A function call via a function pointer
> +# will entry via the GEP. A normal function call will enter via the LEP.
> +#
> +# This test has been expanded to include tests to verify the reverse-finish
> +# command works properly if the function is called via the GEP. The original
> +# test only verified the reverse-finish command for a normal call that used
> +# the LEP.
> +
> if ![supports_reverse] {
> return
> }
> @@ -50,32 +62,32 @@ if [supports_process_record] {
> }
>
>
> -### TEST 1: reverse finish from the entry point instruction in
> -### function1.
> +### TEST 1: reverse finish from the entry point instruction (LEP) in
> +### function1 when called using the normal entry point (LEP).
>
> # Set breakpoint at call to function1 in main.
> -set FUNCTION_test [gdb_get_line_number "CALL FUNCTION" $srcfile]
> -gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at .*" \
> - "set breakpoint on function1 call to stepi into function"
> +set LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
> +gdb_test "break $srcfile:$LEP_test" "Breakpoint $decimal at .*" \
> + "set breakpoint on function1 LEP call to stepi into 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 stepi into function"
>
> # stepi until we see "{" indicating we entered function1
> -cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
> +cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call"
>
> delete_breakpoints
>
> -gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL FUNCTION.*" \
> - "reverse-finish function1 "
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
> + "reverse-finish function1 LEP call from LEP "
>
> # Check to make sure we stopped at the first instruction in the source code
> # line. It should only take one reverse next command to get to the previous
> # source line. If GDB stops at the last instruction in the source code line
> # it will take two reverse next instructions to get to the previous source
> # line.
> -gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from LEP"
>
> # Clear the recorded log.
> gdb_test "record stop" "Process record is stopped.*" \
> @@ -83,26 +95,91 @@ gdb_test "record stop" "Process record is stopped.*" \
> gdb_test_no_output "record" "turn on process record for test2"
>
>
> -### TEST 2: reverse finish from the body of function1.
> +### TEST 2: reverse finish from the body of function1 when called using the
> +### normal entry point (LEP).
>
> # 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"
> +gdb_test "break $srcfile:$LEP_test" "Breakpoint $decimal at .*" \
> + "set breakpoint on function1 LEP 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"
> + "stopped at function1 entry point instruction to step 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"
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
> + "reverse-finish function1 LEP call from function body"
>
> # Check to make sure we stopped at the first instruction in the source code
> # line. It should only take one reverse next command to get to the previous
> # source line.
> gdb_test "reverse-next" ".*b = 5;.*" \
> "reverse next at b = 5, from function body"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop" "Process record is stopped.*" \
> + "turn off process record for test2"
> +gdb_test_no_output "record" "turn on process record for test3"
> +
> +
> +### TEST 3: reverse finish from the alternate entry point instruction (GEP) in
> +### function1 when called using the alternate entry point (GEP).
> +
> +# Set breakpoint at call to funp in main.
> +set GEP_test [gdb_get_line_number "CALL VIA GEP" $srcfile]
> +gdb_test "break $srcfile:$GEP_test" "Breakpoint $decimal at .*" \
> + "set breakpoint on function1 GEP call to stepi into function"
> +
> +# Continue to break point at funp call in main.
> +gdb_test "continue" "Breakpoint $decimal.*funp \\(a, b\\).*" \
> + "stopped at funp entry point instruction"
> +
> +# stepi until we see "{" indicating we entered function.
> +cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
> +
> +delete_breakpoints
> +
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> + "function1 GEP call call from GEP"
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line. It should only take one reverse next command to get to the previous
> +# source line. If GDB stops at the last instruction in the source code line
> +# it will take two reverse next instructions to get to the previous source
> +# line.
> +gdb_test "reverse-next" ".*b = 50;.*" "reverse next at b = 50, call from GEP"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop" "Process record is stopped.*" \
> + "turn off process record for test3"
> +gdb_test_no_output "record" "turn on process record for test4"
> +
> +
> +### TEST 4: reverse finish from the body of function 1 when calling using the
> +### alternate entrypoint (GEP).
> +gdb_test "break $srcfile:$GEP_test" "Breakpoint $decimal at .*" \
> + "set breakpoint on funp GEP call to step into body of function"
> +
> +# Continue to break point at funp call.
> +gdb_test "continue" "Breakpoint $decimal,.*funp \\(a, b\\).*" \
> + "stopped at funp call"
> +
> +# Step into body of funp, called via GEP.
> +gdb_test "step" ".*int ret = 0;.*" "step test 2"
> +
> +delete_breakpoints
> +
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> + "reverse-finish function1 GEP call, from function body "
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line. It should only take one reverse next command to get to the previous
> +# source line. If GDB stops at the last instruction in the source code line
> +# it will take two reverse next instructions to get to the previous source
> +# line.
> +gdb_test "reverse-next" ".*b = 50;.*" \
> + "reverse next at b = 50 from function body"

--
Cheers,
Bruno