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

  Tom, Ulrich, Bruno, Pedro, GDB maintainers:

This patch fixes the reverse-finish command on PowerPC.  The command
now works the same as on other architectures, specifically X86.  There
are no functional changes for other architectures.  The patch includes
a new testcase to verify the reverse-finish command works correctly
with the multiple entry points supported by PowerPC.


Patch tested on PowerPC and 5th generation X86 with no regression
failures.

                  Carl 

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

PPC64 multiple entry points, a normal entry point and an alternate entry
point.  The alternate entry point is to setup the Table of Contents (TOC)
register before continuing at the normal entry point.  When the TOC is
already valid, the normal entry point is used, this is typically the case.
The alternate entry point is typically referred to as the global entry
point (GEP) in IBM.  The normal entry point is typically referred to as
the local entry point (LEP).

When GDB is executing the finish command in reverse, the function
finish_backward currently sets the break point at the alternate entry point.
This issue is if the function, when executing in the forward direction, entered
the function via the normal entry point, execution in the reverse direction
will never sees the break point at the alternate entry point.  In this case,
the reverse execution continues until the next break point is encountered thus
stopping at the wrong place.

This patch adds a new address to struct execution_control_state to hold the
address of the alternate entry point (GEP).  The finish_backwards function
is updated, if the stopping point is between the normal entry point (LEP)
and the end of the function, a breakpoint is set at the normal entry point.
If the stopping point is between the entry points, a breakpoint is set at
the alternate entry point.  This ensures that GDB will always stop at the
normal entry point.  If the function did enter via the alternate entry point,
GDB will detect that and continue to execute backwards in the function until
the alternate entry point is reached.

The patch fixes the behavior of the reverse-finish command on PowerPC to
match the behavior of the command on other platforms, specifically X86.
The patch does not change the behavior of the command on X86.

A new test is added to verify the reverse-finish command on PowerPC
correctly stops at the instruction where the function call is made.

The patch fixes 11 regression errors in test gdb.reverse/finish-precsave.exp
and 11 regression errors in test gdb.reverse/finish-reverse.exp.

The patch has been tested on Power 10 and X86 processor with no new
regression failures.
---
 gdb/infcmd.c                                  |  47 ++--
 gdb/infrun.c                                  |  24 ++
 .../gdb.reverse/finish-reverse-next.c         |  91 +++++++
 .../gdb.reverse/finish-reverse-next.exp       | 224 ++++++++++++++++++
 4 files changed, 369 insertions(+), 17 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp
  

GCC developers:

Ping.  Just wondering if someone could review this patch for
me.  Thanks.

                Carl 

On Wed, 2023-03-01 at 12:59 -0800, Carl Love wrote:
> Tom, Ulrich, Bruno, Pedro, GDB maintainers:
> 
> This patch fixes the reverse-finish command on PowerPC.  The command
> now works the same as on other architectures, specifically
> X86.  There
> are no functional changes for other architectures.  The patch
> includes
> a new testcase to verify the reverse-finish command works correctly
> with the multiple entry points supported by PowerPC.
> 
> 
> Patch tested on PowerPC and 5th generation X86 with no regression
> failures.
> 
>                   Carl 
> 
> --------------------------------------------------------
> PowerPC: fix for gdb.reverse/finish-precsave.exp and
> gdb.reverse/finish-reverse.exp
> 
> PPC64 multiple entry points, a normal entry point and an alternate
> entry
> point.  The alternate entry point is to setup the Table of Contents
> (TOC)
> register before continuing at the normal entry point.  When the TOC
> is
> already valid, the normal entry point is used, this is typically the
> case.
> The alternate entry point is typically referred to as the global
> entry
> point (GEP) in IBM.  The normal entry point is typically referred to
> as
> the local entry point (LEP).
> 
> When GDB is executing the finish command in reverse, the function
> finish_backward currently sets the break point at the alternate entry
> point.
> This issue is if the function, when executing in the forward
> direction, entered
> the function via the normal entry point, execution in the reverse
> direction
> will never sees the break point at the alternate entry point.  In
> this case,
> the reverse execution continues until the next break point is
> encountered thus
> stopping at the wrong place.
> 
> This patch adds a new address to struct execution_control_state to
> hold the
> address of the alternate entry point (GEP).  The finish_backwards
> function
> is updated, if the stopping point is between the normal entry point
> (LEP)
> and the end of the function, a breakpoint is set at the normal entry
> point.
> If the stopping point is between the entry points, a breakpoint is
> set at
> the alternate entry point.  This ensures that GDB will always stop at
> the
> normal entry point.  If the function did enter via the alternate
> entry point,
> GDB will detect that and continue to execute backwards in the
> function until
> the alternate entry point is reached.
> 
> The patch fixes the behavior of the reverse-finish command on PowerPC
> to
> match the behavior of the command on other platforms, specifically
> X86.
> The patch does not change the behavior of the command on X86.
> 
> A new test is added to verify the reverse-finish command on PowerPC
> correctly stops at the instruction where the function call is made.
> 
> The patch fixes 11 regression errors in test gdb.reverse/finish-
> precsave.exp
> and 11 regression errors in test gdb.reverse/finish-reverse.exp.
> 
> The patch has been tested on Power 10 and X86 processor with no new
> regression failures.
> ---
>  gdb/infcmd.c                                  |  47 ++--
>  gdb/infrun.c                                  |  24 ++
>  .../gdb.reverse/finish-reverse-next.c         |  91 +++++++
>  .../gdb.reverse/finish-reverse-next.exp       | 224
> ++++++++++++++++++
>  4 files changed, 369 insertions(+), 17 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/infcmd.c b/gdb/infcmd.c
> index c369b795757..81c617448af 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1728,28 +1728,41 @@ finish_backward (struct finish_command_fsm
> *sm)
>       no way that a function up the stack can have a return address
>       that's equal to its entry point.  */
> 
> -  if (sal.pc != pc)
> -    {
> -      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;
> +  frame_info_ptr frame = get_selected_frame (nullptr);
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +
> +  if (gdbarch_skip_entrypoint_p (gdbarch))
> +    /* Some architectures, like PowerPC use local and global entry
> points.
> +       There is only one Entry Point (GEP = LEP) for other
> architectures.
> +       The GEP is an alternate entry point.  The LEP is the normal
> entry point.
> +       The value of entry_point was initialized to the alternate
> entry point
> +       (GEP).  It will be adjusted to the normal entry point if the
> function
> +       has two entry points.  */
> +    entry_point = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> 
> -      /* Set a step-resume at the function's entry point.  Once
> that's
> -	 hit, we'll do one more step backwards.  */
> +  if  ((pc < alt_entry_point) || (pc > entry_point))
> +    {
> +      /* We are in the body of the function.  Set a breakpoint to go
> back to
> +	 the normal entry point.  */
>        symtab_and_line sr_sal;
> -      sr_sal.pc = sal.pc;
> +      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);
> +      insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
> +					    null_frame_id);
>      }
> +
>    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 either at one of the entry points or between the entry
> points.
> +       If we are not at the alt_entry point, go back to the
> alt_entry_point
> +       If we at the normal entry point step back one instruction,
> when we
> +       stop we will determine if we entered via the entry point or
> the
> +       alternate entry point.  If we are at the alternate entry
> point,
> +       single step back to the function call.  */
> +    tp->control.step_range_start = tp->control.step_range_end = 1;
> +
> +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>  }
> 
>  /* finish_forward -- helper function for finish_command.  FRAME is
> the
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index ab77300f1ff..ca2fc02898a 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1938,6 +1938,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;
> @@ -4822,6 +4823,11 @@ fill_in_stop_func (struct gdbarch *gdbarch,
>  	  ecs->stop_func_start
>  	    += gdbarch_deprecated_function_start_offset (gdbarch);
> 
> +	  /* PowerPC functions have a Local Entry Point (LEP) and a
> Global
> +	     Entry Point (GEP).  There is only one Entry Point (GEP =
> LEP) for
> +	     other architectures.  */
> +	  ecs->stop_func_alt_start = ecs->stop_func_start;
> +
>  	  if (gdbarch_skip_entrypoint_p (gdbarch))
>  	    ecs->stop_func_start
>  	      = gdbarch_skip_entrypoint (gdbarch, ecs-
> >stop_func_start);
> @@ -7411,6 +7417,24 @@ process_event_stop_test (struct
> execution_control_state *ecs)
>  	}
>      }
> 
> +  if (execution_direction == EXEC_REVERSE
> +      && ecs->event_thread->control.proceed_to_finish
> +      && ecs->event_thread->stop_pc () >= ecs->stop_func_alt_start
> +      && ecs->event_thread->stop_pc () < ecs->stop_func_start)
> +    {
> +      /* We are executing the reverse-finish command.
> +	 If the system supports multiple entry points and we are
> finishing a
> +	 function in reverse.   If we are between the entry points
> singe-step
> +	 back to the alternate entry point.  If we are at the alternate
> entry
> +	 point -- just   need to back up by one more single-step, which
> +	 should take us back to the function call.  */
> +      ecs->event_thread->control.step_range_start
> +	= ecs->event_thread->control.step_range_end = 1;
> +      keep_going (ecs);
> +      return;
> +
> +    }
> +
>    if (ecs->event_thread->control.step_range_end == 1)
>      {
>        /* It is stepi or nexti.  We always want to stop stepping
> after
> 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..e95ee8e33a6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> @@ -0,0 +1,91 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2012-2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <
> http://www.gnu.org/licenses/>.  */
> +
> +/* The reverse finish command should return from a function and stop
> on
> +   the first instruction of the source line where the function call
> is made.
> +   Specifically, the behavior should match doing a reverse next from
> the
> +   first instruction in the function.  GDB should only require one
> reverse
> +   step or next statement to reach the previous source code line.
> +
> +   This test verifies the fix for gdb bugzilla:
> +
> +   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> +
> +   PowerPC supports two entry points to a function.  The normal
> entry point
> +   is called the local entry point (LEP).  The alternate 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
> +function2 (int a, int b)
> +{
> +  int ret = 0;
> +  ret = ret + a + b;
> +  return ret;
> +}
> +
> +int
> +function1 (int a, int b)   // FUNCTION1
> +{
> +  int ret = 0;
> +  int (*funp) (int, int) = &function2;
> +  /* 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.
> +  */
> +  ret = funp (a + 1, b + 2);
> +  return ret;
> +}
> +
> +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 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
> new file mode 100644
> index 00000000000..1f53b649a7d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -0,0 +1,224 @@
> +# Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <
> http://www.gnu.org/licenses/>.  */
> +
> +# This file is part of the GDB testsuite.  It tests reverse
> stepping.
> +# Lots of code borrowed from "step-test.exp".
> +
> +# The reverse finish command should return from a function and stop
> on
> +# the first instruction of the source line where the function call
> is made.
> +# Specifically, the behavior should match doing a reverse next from
> the
> +# first instruction in the function.  GDB should only take one
> reverse step
> +# or next statement to reach the previous source code line.
> +
> +# This testcase verifies the reverse-finish command stops at the
> first
> +# instruction in the source code line where the function was
> called.  There
> +# are two scenarios that must be checked:
> +#   1) gdb is at the entry point instruction for the function
> +#   2) gdb is in the body of the function.
> +
> +# This test verifies the fix for gdb bugzilla:
> +#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> +
> +# PowerPC supports two entry points to a function.  The normal entry
> point
> +# is called the local entry point (LEP).  The alternate entry point
> is called
> +# the global entry point (GEP).  A function call via a function
> pointer
> +# will entry via the GEP.  A normal function call will enter via the
> LEP.
> +#
> +# This test has been expanded to include tests to verify the
> reverse-finish
> +# command works properly if the function is called via the GEP.  The
> original
> +# test only verified the reverse-finish command for a normal call
> that used
> +# the LEP.
> +
> +if ![supports_reverse] {
> +    return
> +}
> +
> +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 (LEP) in
> +### function1 when called using the normal entry point (LEP).
> +
> +# Set breakpoint at call to function1 in main.
> +set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
> +gdb_breakpoint $srcfile:$bp_LEP_test temporary
> +
> +# Continue to break point at function1 call in main.
> +gdb_continue_to_breakpoint \
> +    "stopped at function1 entry point instruction to stepi into
> function" \
> +    ".*$srcfile:$bp_LEP_test\r\n.*"
> +
> +# stepi until we see "{" indicating we entered function1
> +repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1
> call" "100"
> +
> +# The reverse-finish command should stop on the function call
> instruction
> +# which is the last instruction in the source code line.  A reverse-
> next
> +# instruction should then stop at the first instruction in the same
> source
> +# code line.  Another revers-next instruction stops at the previous
> source
> +# code line.
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA
> LEP.*" \
> +    "reverse-finish function1 LEP call from LEP "
> +gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP"
> \
> +    "reverse next 1 LEP entry point function call from LEP"
> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call
> from LEP"
> +
> +
> +gdb_test "reverse-continue" ".*" "setup for test 2"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test1"
> +gdb_test_no_output "record" "turn on process record for test2"
> +
> +
> +### TEST 2: reverse finish from the body of function1.
> +
> +# Set breakpoint at call to function1 in main.
> +gdb_breakpoint $srcfile:$bp_LEP_test temporary
> +
> +# Continue to break point at function1 call in main.
> +gdb_continue_to_breakpoint \
> +    "at function1 entry point instruction to step to body of
> function" \
> +    ".*$srcfile:$bp_LEP_test\r\n.*"
> +
> +# do a step instruction to get to the body of the function
> +gdb_test "step" ".*int ret = 0;.*" "step test 1"
> +
> +# The reverse-finish command should stop on the function call
> instruction
> +# which is the last instruction in the source code line.  A reverse-
> next
> +# instruction should then stop at the first instruction in the same
> source
> +# code line.  Another revers-next instruction stops at the previous
> source
> +# code line.
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA
> LEP.*" \
> +    "reverse-finish function1 LEP call from function body"
> +gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA
> LEP.*" \
> +    "reverse next 1 LEP from function body"
> +gdb_test "reverse-next" ".*b = 5;.*" \
> +    "reverse next 2 at b = 5, from function body"
> +
> +gdb_test "reverse-continue" ".*" "setup for test 3"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test2"
> +gdb_test_no_output "record" "turn on process record for test3"
> +
> +
> +### TEST 3: reverse finish from the alternate entry point
> instruction (GEP) in
> +### function1 when called using the alternate entry point (GEP).
> +
> +# Set breakpoint at call to funp in main.
> +set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
> +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> +
> +# Continue to break point at funp call in main.
> +gdb_continue_to_breakpoint \
> +    "stopped at function1 entry point instruction to stepi into
> funp" \
> +    ".*$srcfile:$bp_GEP_test\r\n.*"
> +
> +# stepi until we see "{" indicating we entered function.
> +repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
> +
> +# The reverse-finish command should stop on the function call
> instruction
> +# which is the last instruction in the source code line.  A reverse-
> next
> +# instruction should then stop at the first instruction in the same
> source
> +# code line.  Another revers-next instruction stops at the previous
> source
> +# code line.
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> +    "function1 GEP call call from GEP"
> +gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> +    "reverse next 1 GEP entry point function call from GEP"
> +gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50,
> call from GEP"
> +
> +gdb_test "reverse-continue" ".*" "setup for test 4"
> +
> +# 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 between the GEP and LEP in
> +### function1 when called using the alternate entry point (GEP).
> +
> +# Set breakpoint at call to funp in main.
> +set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
> +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> +
> +# Continue to break point at funp call in main.
> +gdb_continue_to_breakpoint \
> +    "stopped at function1 entry point instruction to stepi into funp
> again" \
> +    ".*$srcfile:$bp_GEP_test\r\n.*"
> +
> +# stepi until we see "{" indicating we entered function.
> +repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call
> again"
> +
> +# do one more stepi so we are between the GEP and LEP.
> +gdb_test "stepi" "{" "stepi to between GEP and LEP"
> +
> +# The reverse-finish command should stop on the function call
> instruction
> +# which is the last instruction in the source code line.  A reverse-
> next
> +# instruction should then stop at the first instruction in the same
> source
> +# code line.  Another revers-next instruction stops at the previous
> source
> +# code line.
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> +    "function1 GEP call call from GEP again"
> +gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> +    "reverse next 1 GEP entry point function call from GEP again"
> +gdb_test "reverse-next" ".*b = 50;.*" \
> +    "reverse next 2 at b = 50, call from GEP again"
> +
> +gdb_test "reverse-continue" ".*" "setup for test 5"
> +
> +# Turn off record to clear logs and turn on again
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test4"
> +gdb_test_no_output "record" "turn on process record for test5"
> +
> +
> +### TEST 5: reverse finish from the body of function 1 when calling
> using the
> +### alternate entrypoint (GEP).
> +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> +
> +# Continue to break point at funp call.
> +gdb_continue_to_breakpoint \
> +    "at function1 entry point instruction to step to body of funp
> call" \
> +    ".*$srcfile:$bp_GEP_test\r\n.*"
> +
> +# Step into body of funp, called via GEP.
> +gdb_test "step" ".*int ret = 0;.*" "step test 2"
> +
> +# The reverse-finish command should stop on the function call
> instruction
> +# which is the last instruction in the source code line.  A reverse-
> next
> +# instruction should then stop at the first instruction in the same
> source
> +# code line.  Another revers-next instruction stops at the previous
> source
> +# code line.
> +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> +    "reverse-finish function1 GEP call, from function body  "
> +gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> +    "reverse next 1 GEP call from function body"
> +gdb_test "reverse-next" ".*b = 50;.*" \
> +    "reverse next 2 at b = 50 from function body"
  

Oops, that should have read GDB developers.  Sorry about that.

On Wed, 2023-03-08 at 08:19 -0800, Carl Love wrote:
> GCC developers:
> 
> Ping.  Just wondering if someone could review this patch for
> me.  Thanks.
> 
>                 Carl 
> 
> On Wed, 2023-03-01 at 12:59 -0800, Carl Love wrote:
> > Tom, Ulrich, Bruno, Pedro, GDB maintainers:
> > 
> > This patch fixes the reverse-finish command on PowerPC.  The
> > command
> > now works the same as on other architectures, specifically
> > X86.  There
> > are no functional changes for other architectures.  The patch
> > includes
> > a new testcase to verify the reverse-finish command works correctly
> > with the multiple entry points supported by PowerPC.
> > 
> > 
> > Patch tested on PowerPC and 5th generation X86 with no regression
> > failures.
> > 
> >                   Carl 
> > 
> > --------------------------------------------------------
> > PowerPC: fix for gdb.reverse/finish-precsave.exp and
> > gdb.reverse/finish-reverse.exp
> > 
> > PPC64 multiple entry points, a normal entry point and an alternate
> > entry
> > point.  The alternate entry point is to setup the Table of Contents
> > (TOC)
> > register before continuing at the normal entry point.  When the TOC
> > is
> > already valid, the normal entry point is used, this is typically
> > the
> > case.
> > The alternate entry point is typically referred to as the global
> > entry
> > point (GEP) in IBM.  The normal entry point is typically referred
> > to
> > as
> > the local entry point (LEP).
> > 
> > When GDB is executing the finish command in reverse, the function
> > finish_backward currently sets the break point at the alternate
> > entry
> > point.
> > This issue is if the function, when executing in the forward
> > direction, entered
> > the function via the normal entry point, execution in the reverse
> > direction
> > will never sees the break point at the alternate entry point.  In
> > this case,
> > the reverse execution continues until the next break point is
> > encountered thus
> > stopping at the wrong place.
> > 
> > This patch adds a new address to struct execution_control_state to
> > hold the
> > address of the alternate entry point (GEP).  The finish_backwards
> > function
> > is updated, if the stopping point is between the normal entry point
> > (LEP)
> > and the end of the function, a breakpoint is set at the normal
> > entry
> > point.
> > If the stopping point is between the entry points, a breakpoint is
> > set at
> > the alternate entry point.  This ensures that GDB will always stop
> > at
> > the
> > normal entry point.  If the function did enter via the alternate
> > entry point,
> > GDB will detect that and continue to execute backwards in the
> > function until
> > the alternate entry point is reached.
> > 
> > The patch fixes the behavior of the reverse-finish command on
> > PowerPC
> > to
> > match the behavior of the command on other platforms, specifically
> > X86.
> > The patch does not change the behavior of the command on X86.
> > 
> > A new test is added to verify the reverse-finish command on PowerPC
> > correctly stops at the instruction where the function call is made.
> > 
> > The patch fixes 11 regression errors in test gdb.reverse/finish-
> > precsave.exp
> > and 11 regression errors in test gdb.reverse/finish-reverse.exp.
> > 
> > The patch has been tested on Power 10 and X86 processor with no new
> > regression failures.
> > ---
> >  gdb/infcmd.c                                  |  47 ++--
> >  gdb/infrun.c                                  |  24 ++
> >  .../gdb.reverse/finish-reverse-next.c         |  91 +++++++
> >  .../gdb.reverse/finish-reverse-next.exp       | 224
> > ++++++++++++++++++
> >  4 files changed, 369 insertions(+), 17 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/infcmd.c b/gdb/infcmd.c
> > index c369b795757..81c617448af 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -1728,28 +1728,41 @@ finish_backward (struct finish_command_fsm
> > *sm)
> >       no way that a function up the stack can have a return address
> >       that's equal to its entry point.  */
> > 
> > -  if (sal.pc != pc)
> > -    {
> > -      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;
> > +  frame_info_ptr frame = get_selected_frame (nullptr);
> > +  struct gdbarch *gdbarch = get_frame_arch (frame);
> > +
> > +  if (gdbarch_skip_entrypoint_p (gdbarch))
> > +    /* Some architectures, like PowerPC use local and global entry
> > points.
> > +       There is only one Entry Point (GEP = LEP) for other
> > architectures.
> > +       The GEP is an alternate entry point.  The LEP is the normal
> > entry point.
> > +       The value of entry_point was initialized to the alternate
> > entry point
> > +       (GEP).  It will be adjusted to the normal entry point if
> > the
> > function
> > +       has two entry points.  */
> > +    entry_point = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> > 
> > -      /* Set a step-resume at the function's entry point.  Once
> > that's
> > -	 hit, we'll do one more step backwards.  */
> > +  if  ((pc < alt_entry_point) || (pc > entry_point))
> > +    {
> > +      /* We are in the body of the function.  Set a breakpoint to
> > go
> > back to
> > +	 the normal entry point.  */
> >        symtab_and_line sr_sal;
> > -      sr_sal.pc = sal.pc;
> > +      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);
> > +      insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
> > +					    null_frame_id);
> >      }
> > +
> >    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 either at one of the entry points or between the
> > entry
> > points.
> > +       If we are not at the alt_entry point, go back to the
> > alt_entry_point
> > +       If we at the normal entry point step back one instruction,
> > when we
> > +       stop we will determine if we entered via the entry point or
> > the
> > +       alternate entry point.  If we are at the alternate entry
> > point,
> > +       single step back to the function call.  */
> > +    tp->control.step_range_start = tp->control.step_range_end = 1;
> > +
> > +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> >  }
> > 
> >  /* finish_forward -- helper function for finish_command.  FRAME is
> > the
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index ab77300f1ff..ca2fc02898a 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -1938,6 +1938,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;
> > @@ -4822,6 +4823,11 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> >  	  ecs->stop_func_start
> >  	    += gdbarch_deprecated_function_start_offset (gdbarch);
> > 
> > +	  /* PowerPC functions have a Local Entry Point (LEP) and a
> > Global
> > +	     Entry Point (GEP).  There is only one Entry Point (GEP =
> > LEP) for
> > +	     other architectures.  */
> > +	  ecs->stop_func_alt_start = ecs->stop_func_start;
> > +
> >  	  if (gdbarch_skip_entrypoint_p (gdbarch))
> >  	    ecs->stop_func_start
> >  	      = gdbarch_skip_entrypoint (gdbarch, ecs-
> > > stop_func_start);
> > @@ -7411,6 +7417,24 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >  	}
> >      }
> > 
> > +  if (execution_direction == EXEC_REVERSE
> > +      && ecs->event_thread->control.proceed_to_finish
> > +      && ecs->event_thread->stop_pc () >= ecs->stop_func_alt_start
> > +      && ecs->event_thread->stop_pc () < ecs->stop_func_start)
> > +    {
> > +      /* We are executing the reverse-finish command.
> > +	 If the system supports multiple entry points and we are
> > finishing a
> > +	 function in reverse.   If we are between the entry points
> > singe-step
> > +	 back to the alternate entry point.  If we are at the alternate
> > entry
> > +	 point -- just   need to back up by one more single-step, which
> > +	 should take us back to the function call.  */
> > +      ecs->event_thread->control.step_range_start
> > +	= ecs->event_thread->control.step_range_end = 1;
> > +      keep_going (ecs);
> > +      return;
> > +
> > +    }
> > +
> >    if (ecs->event_thread->control.step_range_end == 1)
> >      {
> >        /* It is stepi or nexti.  We always want to stop stepping
> > after
> > 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..e95ee8e33a6
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> > @@ -0,0 +1,91 @@
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > +   Copyright 2012-2023 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published
> > by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <
> > http://www.gnu.org/licenses/>.  */
> > +
> > +/* The reverse finish command should return from a function and
> > stop
> > on
> > +   the first instruction of the source line where the function
> > call
> > is made.
> > +   Specifically, the behavior should match doing a reverse next
> > from
> > the
> > +   first instruction in the function.  GDB should only require one
> > reverse
> > +   step or next statement to reach the previous source code line.
> > +
> > +   This test verifies the fix for gdb bugzilla:
> > +
> > +   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> > +
> > +   PowerPC supports two entry points to a function.  The normal
> > entry point
> > +   is called the local entry point (LEP).  The alternate 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
> > +function2 (int a, int b)
> > +{
> > +  int ret = 0;
> > +  ret = ret + a + b;
> > +  return ret;
> > +}
> > +
> > +int
> > +function1 (int a, int b)   // FUNCTION1
> > +{
> > +  int ret = 0;
> > +  int (*funp) (int, int) = &function2;
> > +  /* 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.
> > +  */
> > +  ret = funp (a + 1, b + 2);
> > +  return ret;
> > +}
> > +
> > +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 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
> > new file mode 100644
> > index 00000000000..1f53b649a7d
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> > @@ -0,0 +1,224 @@
> > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published
> > by
> > +# the Free Software Foundation; either version 3 of the License,
> > or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/>.  */
> > +
> > +# This file is part of the GDB testsuite.  It tests reverse
> > stepping.
> > +# Lots of code borrowed from "step-test.exp".
> > +
> > +# The reverse finish command should return from a function and
> > stop
> > on
> > +# the first instruction of the source line where the function call
> > is made.
> > +# Specifically, the behavior should match doing a reverse next
> > from
> > the
> > +# first instruction in the function.  GDB should only take one
> > reverse step
> > +# or next statement to reach the previous source code line.
> > +
> > +# This testcase verifies the reverse-finish command stops at the
> > first
> > +# instruction in the source code line where the function was
> > called.  There
> > +# are two scenarios that must be checked:
> > +#   1) gdb is at the entry point instruction for the function
> > +#   2) gdb is in the body of the function.
> > +
> > +# This test verifies the fix for gdb bugzilla:
> > +#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> > +
> > +# PowerPC supports two entry points to a function.  The normal
> > entry
> > point
> > +# is called the local entry point (LEP).  The alternate entry
> > point
> > is called
> > +# the global entry point (GEP).  A function call via a function
> > pointer
> > +# will entry via the GEP.  A normal function call will enter via
> > the
> > LEP.
> > +#
> > +# This test has been expanded to include tests to verify the
> > reverse-finish
> > +# command works properly if the function is called via the
> > GEP.  The
> > original
> > +# test only verified the reverse-finish command for a normal call
> > that used
> > +# the LEP.
> > +
> > +if ![supports_reverse] {
> > +    return
> > +}
> > +
> > +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 (LEP)
> > in
> > +### function1 when called using the normal entry point (LEP).
> > +
> > +# Set breakpoint at call to function1 in main.
> > +set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
> > +gdb_breakpoint $srcfile:$bp_LEP_test temporary
> > +
> > +# Continue to break point at function1 call in main.
> > +gdb_continue_to_breakpoint \
> > +    "stopped at function1 entry point instruction to stepi into
> > function" \
> > +    ".*$srcfile:$bp_LEP_test\r\n.*"
> > +
> > +# stepi until we see "{" indicating we entered function1
> > +repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1
> > call" "100"
> > +
> > +# The reverse-finish command should stop on the function call
> > instruction
> > +# which is the last instruction in the source code line.  A
> > reverse-
> > next
> > +# instruction should then stop at the first instruction in the
> > same
> > source
> > +# code line.  Another revers-next instruction stops at the
> > previous
> > source
> > +# code line.
> > +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA
> > LEP.*" \
> > +    "reverse-finish function1 LEP call from LEP "
> > +gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA
> > LEP"
> > \
> > +    "reverse next 1 LEP entry point function call from LEP"
> > +gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5,
> > call
> > from LEP"
> > +
> > +
> > +gdb_test "reverse-continue" ".*" "setup for test 2"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test1"
> > +gdb_test_no_output "record" "turn on process record for test2"
> > +
> > +
> > +### TEST 2: reverse finish from the body of function1.
> > +
> > +# Set breakpoint at call to function1 in main.
> > +gdb_breakpoint $srcfile:$bp_LEP_test temporary
> > +
> > +# Continue to break point at function1 call in main.
> > +gdb_continue_to_breakpoint \
> > +    "at function1 entry point instruction to step to body of
> > function" \
> > +    ".*$srcfile:$bp_LEP_test\r\n.*"
> > +
> > +# do a step instruction to get to the body of the function
> > +gdb_test "step" ".*int ret = 0;.*" "step test 1"
> > +
> > +# The reverse-finish command should stop on the function call
> > instruction
> > +# which is the last instruction in the source code line.  A
> > reverse-
> > next
> > +# instruction should then stop at the first instruction in the
> > same
> > source
> > +# code line.  Another revers-next instruction stops at the
> > previous
> > source
> > +# code line.
> > +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA
> > LEP.*" \
> > +    "reverse-finish function1 LEP call from function body"
> > +gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA
> > LEP.*" \
> > +    "reverse next 1 LEP from function body"
> > +gdb_test "reverse-next" ".*b = 5;.*" \
> > +    "reverse next 2 at b = 5, from function body"
> > +
> > +gdb_test "reverse-continue" ".*" "setup for test 3"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test2"
> > +gdb_test_no_output "record" "turn on process record for test3"
> > +
> > +
> > +### TEST 3: reverse finish from the alternate entry point
> > instruction (GEP) in
> > +### function1 when called using the alternate entry point (GEP).
> > +
> > +# Set breakpoint at call to funp in main.
> > +set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
> > +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> > +
> > +# Continue to break point at funp call in main.
> > +gdb_continue_to_breakpoint \
> > +    "stopped at function1 entry point instruction to stepi into
> > funp" \
> > +    ".*$srcfile:$bp_GEP_test\r\n.*"
> > +
> > +# stepi until we see "{" indicating we entered function.
> > +repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
> > +
> > +# The reverse-finish command should stop on the function call
> > instruction
> > +# which is the last instruction in the source code line.  A
> > reverse-
> > next
> > +# instruction should then stop at the first instruction in the
> > same
> > source
> > +# code line.  Another revers-next instruction stops at the
> > previous
> > source
> > +# code line.
> > +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> > +    "function1 GEP call call from GEP"
> > +gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> > +    "reverse next 1 GEP entry point function call from GEP"
> > +gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50,
> > call from GEP"
> > +
> > +gdb_test "reverse-continue" ".*" "setup for test 4"
> > +
> > +# 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 between the GEP and LEP in
> > +### function1 when called using the alternate entry point (GEP).
> > +
> > +# Set breakpoint at call to funp in main.
> > +set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
> > +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> > +
> > +# Continue to break point at funp call in main.
> > +gdb_continue_to_breakpoint \
> > +    "stopped at function1 entry point instruction to stepi into
> > funp
> > again" \
> > +    ".*$srcfile:$bp_GEP_test\r\n.*"
> > +
> > +# stepi until we see "{" indicating we entered function.
> > +repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call
> > again"
> > +
> > +# do one more stepi so we are between the GEP and LEP.
> > +gdb_test "stepi" "{" "stepi to between GEP and LEP"
> > +
> > +# The reverse-finish command should stop on the function call
> > instruction
> > +# which is the last instruction in the source code line.  A
> > reverse-
> > next
> > +# instruction should then stop at the first instruction in the
> > same
> > source
> > +# code line.  Another revers-next instruction stops at the
> > previous
> > source
> > +# code line.
> > +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> > +    "function1 GEP call call from GEP again"
> > +gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> > +    "reverse next 1 GEP entry point function call from GEP again"
> > +gdb_test "reverse-next" ".*b = 50;.*" \
> > +    "reverse next 2 at b = 50, call from GEP again"
> > +
> > +gdb_test "reverse-continue" ".*" "setup for test 5"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test4"
> > +gdb_test_no_output "record" "turn on process record for test5"
> > +
> > +
> > +### TEST 5: reverse finish from the body of function 1 when
> > calling
> > using the
> > +### alternate entrypoint (GEP).
> > +gdb_breakpoint $srcfile:$bp_GEP_test temporary
> > +
> > +# Continue to break point at funp call.
> > +gdb_continue_to_breakpoint \
> > +    "at function1 entry point instruction to step to body of funp
> > call" \
> > +    ".*$srcfile:$bp_GEP_test\r\n.*"
> > +
> > +# Step into body of funp, called via GEP.
> > +gdb_test "step" ".*int ret = 0;.*" "step test 2"
> > +
> > +# The reverse-finish command should stop on the function call
> > instruction
> > +# which is the last instruction in the source code line.  A
> > reverse-
> > next
> > +# instruction should then stop at the first instruction in the
> > same
> > source
> > +# code line.  Another revers-next instruction stops at the
> > previous
> > source
> > +# code line.
> > +gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> > +    "reverse-finish function1 GEP call, from function body  "
> > +gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> > +    "reverse next 1 GEP call from function body"
> > +gdb_test "reverse-next" ".*b = 50;.*" \
> > +    "reverse next 2 at b = 50 from function body"
> 
>
  

Carl Love <cel@us.ibm.com> wrote:

>This patch fixes the reverse-finish command on PowerPC.  The command
>now works the same as on other architectures, specifically X86.  There
>are no functional changes for other architectures.  The patch includes
>a new testcase to verify the reverse-finish command works correctly
>with the multiple entry points supported by PowerPC.

This looks good to me in general, just one cosmetic issue:

>-      /* Set a step-resume at the function's entry point.  Once that's
>-	 hit, we'll do one more step backwards.  */
>+  if  ((pc < alt_entry_point) || (pc > entry_point))
>+    {
>+      /* We are in the body of the function.  Set a breakpoint to go back to
>+	 the normal entry point.  */
>       symtab_and_line sr_sal;
>-      sr_sal.pc = sal.pc;
>+      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);
>+      insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
>+					    null_frame_id);
>     }
>+
>   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 either at one of the entry points or between the entry points.
>+       If we are not at the alt_entry point, go back to the alt_entry_point
>+       If we at the normal entry point step back one instruction, when we
>+       stop we will determine if we entered via the entry point or the
>+       alternate entry point.  If we are at the alternate entry point,
>+       single step back to the function call.  */
>+    tp->control.step_range_start = tp->control.step_range_end = 1;
>+
>+  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);

There's still a bunch of unnecessary changes in there (changes
in formatting, moving the "proceed" call, etc.).  It would be
preferable to not have those in.


Bye,
Ulrich
  

Ulrich:

On Mon, 2023-03-13 at 14:16 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
> > This patch fixes the reverse-finish command on PowerPC.  The
> > command
> > now works the same as on other architectures, specifically
> > X86.  There
> > are no functional changes for other architectures.  The patch
> > includes
> > a new testcase to verify the reverse-finish command works correctly
> > with the multiple entry points supported by PowerPC.
> 
> This looks good to me in general, just one cosmetic issue:
> 
> > -      /* Set a step-resume at the function's entry point.  Once
> > that's
> > -	 hit, we'll do one more step backwards.  */
> > +  if  ((pc < alt_entry_point) || (pc > entry_point))
> > +    {
> > +      /* We are in the body of the function.  Set a breakpoint to
> > go back to
> > +	 the normal entry point.  */
> >       symtab_and_line sr_sal;
> > -      sr_sal.pc = sal.pc;
> > +      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);
> > +      insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
> > +					    null_frame_id);
> >     }
> > +
> >   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 either at one of the entry points or between the
> > entry points.
> > +       If we are not at the alt_entry point, go back to the
> > alt_entry_point
> > +       If we at the normal entry point step back one instruction,
> > when we
> > +       stop we will determine if we entered via the entry point or
> > the
> > +       alternate entry point.  If we are at the alternate entry
> > point,
> > +       single step back to the function call.  */
> > +    tp->control.step_range_start = tp->control.step_range_end = 1;
> > +
> > +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> 
> There's still a bunch of unnecessary changes in there (changes
> in formatting, moving the "proceed" call, etc.).  It would be
> preferable to not have those in.

OK, I went back and looked again to see what else I could do to remove
"unecessary changes", etc in gdg/infcmd.c.  I took out the move of the
proceed call from the if/else branches to a single call after the
if/else statement.  I moved the declarations to the top with the other
declarations.  I moved the new if statement up a little.  These changes
make the changes in the if/else are easier to read.  I made a few other
cosmetic changes to get the diff to be as minimal as I could.

I hope this addresses your concerns.  There are no functional changes. 
I retested the patch.  I will post an updated version of the patch.

                        Carl
  

Ulrich, GDB maintainers:

I have updated gdb/infrun.c per the comments from Ulrich to remove
"unnecessary" code and format changes to make the diff as minimal as I
can.  There are no functional changes in these changes.

The patch has been retested on PowerPC to make sure there are no
regressions.

Please let me know if this version of this patch (second in the series)
is acceptable.  Thanks.

                         Carl 


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

PPC64 multiple entry points, a normal entry point and an alternate entry
point.  The alternate entry point is to setup the Table of Contents (TOC)
register before continuing at the normal entry point.  When the TOC is
already valid, the normal entry point is used, this is typically the case.
The alternate entry point is typically referred to as the global entry
point (GEP) in IBM.  The normal entry point is typically referred to as
the local entry point (LEP).

When GDB is executing the finish command in reverse, the function
finish_backward currently sets the break point at the alternate entry point.
This issue is if the function, when executing in the forward direction, entered
the function via the normal entry point, execution in the reverse direction
will never sees the break point at the alternate entry point.  In this case,
the reverse execution continues until the next break point is encountered thus
stopping at the wrong place.

This patch adds a new address to struct execution_control_state to hold the
address of the alternate entry point (GEP).  The finish_backwards function
is updated, if the stopping point is between the normal entry point (LEP)
and the end of the function, a breakpoint is set at the normal entry point.
If the stopping point is between the entry points, a breakpoint is set at
the alternate entry point.  This ensures that GDB will always stop at the
normal entry point.  If the function did enter via the alternate entry point,
GDB will detect that and continue to execute backwards in the function until
the alternate entry point is reached.

The patch fixes the behavior of the reverse-finish command on PowerPC to
match the behavior of the command on other platforms, specifically X86.
The patch does not change the behavior of the command on X86.

A new test is added to verify the reverse-finish command on PowerPC
correctly stops at the instruction where the function call is made.

The patch fixes 11 regression errors in test gdb.reverse/finish-precsave.exp
and 11 regression errors in test gdb.reverse/finish-reverse.exp.

The patch has been tested on Power 10 and X86 processor with no new
regression failures.
---
 gdb/infcmd.c                                  |  32 ++-
 gdb/infrun.c                                  |  24 ++
 .../gdb.reverse/finish-reverse-next.c         |  91 +++++++
 .../gdb.reverse/finish-reverse-next.exp       | 224 ++++++++++++++++++
 4 files changed, 362 insertions(+), 9 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/infcmd.c b/gdb/infcmd.c
index c369b795757..f46461512fe 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1710,6 +1710,10 @@ finish_backward (struct finish_command_fsm *sm)
   struct thread_info *tp = inferior_thread ();
   CORE_ADDR pc;
   CORE_ADDR func_addr;
+  CORE_ADDR alt_entry_point = sal.pc;
+  CORE_ADDR entry_point = alt_entry_point;
+  frame_info_ptr frame = get_selected_frame (nullptr);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
 
   pc = get_frame_pc (get_current_frame ());
 
@@ -1718,6 +1722,15 @@ finish_backward (struct finish_command_fsm *sm)
 
   sal = find_pc_line (func_addr, 0);
 
+  if (gdbarch_skip_entrypoint_p (gdbarch))
+    /* Some architectures, like PowerPC use local and global entry points.
+       There is only one Entry Point (GEP = LEP) for other architectures.
+       The GEP is an alternate entry point.  The LEP is the normal entry point.
+       The value of entry_point was initialized to the alternate entry point
+       (GEP).  It will be adjusted to the normal entry point if the function
+       has two entry points.  */
+    entry_point = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+
   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
@@ -1728,15 +1741,12 @@ finish_backward (struct finish_command_fsm *sm)
      no way that a function up the stack can have a return address
      that's equal to its entry point.  */
 
-  if (sal.pc != pc)
+  if ((pc < alt_entry_point) || (pc > entry_point))
     {
-      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
-	 hit, we'll do one more step backwards.  */
+      /* We are in the body of the function.  Set a breakpoint to go back to
+	 the normal entry point.  */
       symtab_and_line sr_sal;
-      sr_sal.pc = sal.pc;
+      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);
@@ -1745,8 +1755,12 @@ finish_backward (struct finish_command_fsm *sm)
     }
   else
     {
-      /* We're almost there -- we just need to back up by one more
-	 single-step.  */
+      /* We are either at one of the entry points or between the entry points.
+	 If we are not at the alt_entry point, go back to the alt_entry_point
+	 If we at the normal entry point step back one instruction, when we
+	 stop we will determine if we entered via the entry point or the
+	 alternate entry point.  If we are at the alternate entry point,
+	 single step back to the function call.  */
       tp->control.step_range_start = tp->control.step_range_end = 1;
       proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
     }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 33aa0c8794b..5c9babb9104 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1938,6 +1938,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;
@@ -4822,6 +4823,11 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 	  ecs->stop_func_start
 	    += gdbarch_deprecated_function_start_offset (gdbarch);
 
+	  /* PowerPC functions have a Local Entry Point (LEP) and a Global
+	     Entry Point (GEP).  There is only one Entry Point (GEP = LEP) for
+	     other architectures.  */
+	  ecs->stop_func_alt_start = ecs->stop_func_start;
+
 	  if (gdbarch_skip_entrypoint_p (gdbarch))
 	    ecs->stop_func_start
 	      = gdbarch_skip_entrypoint (gdbarch, ecs->stop_func_start);
@@ -7411,6 +7417,24 @@ process_event_stop_test (struct execution_control_state *ecs)
 	}
     }
 
+  if (execution_direction == EXEC_REVERSE
+      && ecs->event_thread->control.proceed_to_finish
+      && ecs->event_thread->stop_pc () >= ecs->stop_func_alt_start
+      && ecs->event_thread->stop_pc () < ecs->stop_func_start)
+    {
+      /* We are executing the reverse-finish command.
+	 If the system supports multiple entry points and we are finishing a
+	 function in reverse.   If we are between the entry points singe-step
+	 back to the alternate entry point.  If we are at the alternate entry
+	 point -- just   need to back up by one more single-step, which
+	 should take us back to the function call.  */
+      ecs->event_thread->control.step_range_start
+	= ecs->event_thread->control.step_range_end = 1;
+      keep_going (ecs);
+      return;
+
+    }
+
   if (ecs->event_thread->control.step_range_end == 1)
     {
       /* It is stepi or nexti.  We always want to stop stepping after
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..e95ee8e33a6
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
@@ -0,0 +1,91 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* The reverse finish command should return from a function and stop on
+   the first instruction of the source line where the function call is made.
+   Specifically, the behavior should match doing a reverse next from the
+   first instruction in the function.  GDB should only require one reverse
+   step or next statement to reach the previous source code line.
+
+   This test verifies the fix for gdb bugzilla:
+
+   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+
+   PowerPC supports two entry points to a function.  The normal entry point
+   is called the local entry point (LEP).  The alternate 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
+function2 (int a, int b)
+{
+  int ret = 0;
+  ret = ret + a + b;
+  return ret;
+}
+
+int
+function1 (int a, int b)   // FUNCTION1
+{
+  int ret = 0;
+  int (*funp) (int, int) = &function2;
+  /* 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.
+  */
+  ret = funp (a + 1, b + 2);
+  return ret;
+}
+
+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 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
new file mode 100644
index 00000000000..1f53b649a7d
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -0,0 +1,224 @@
+# Copyright 2008-2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# This file is part of the GDB testsuite.  It tests reverse stepping.
+# Lots of code borrowed from "step-test.exp".
+
+# The reverse finish command should return from a function and stop on
+# the first instruction of the source line where the function call is made.
+# Specifically, the behavior should match doing a reverse next from the
+# first instruction in the function.  GDB should only take one reverse step
+# or next statement to reach the previous source code line.
+
+# This testcase verifies the reverse-finish command stops at the first
+# instruction in the source code line where the function was called.  There
+# are two scenarios that must be checked:
+#   1) gdb is at the entry point instruction for the function
+#   2) gdb is in the body of the function.
+
+# This test verifies the fix for gdb bugzilla:
+#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+
+# PowerPC supports two entry points to a function.  The normal entry point
+# is called the local entry point (LEP).  The alternate entry point is called
+# the global entry point (GEP).  A function call via a function pointer
+# will entry via the GEP.  A normal function call will enter via the LEP.
+#
+# This test has been expanded to include tests to verify the reverse-finish
+# command works properly if the function is called via the GEP.  The original
+# test only verified the reverse-finish command for a normal call that used
+# the LEP.
+
+if ![supports_reverse] {
+    return
+}
+
+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 (LEP) in
+### function1 when called using the normal entry point (LEP).
+
+# Set breakpoint at call to function1 in main.
+set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into function" \
+    ".*$srcfile:$bp_LEP_test\r\n.*"
+
+# stepi until we see "{" indicating we entered function1
+repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100"
+
+# The reverse-finish command should stop on the function call instruction
+# which is the last instruction in the source code line.  A reverse-next
+# instruction should then stop at the first instruction in the same source
+# code line.  Another revers-next instruction stops at the previous source
+# code line.
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish function1 LEP call from LEP "
+gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP" \
+    "reverse next 1 LEP entry point function call from LEP"
+gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call from LEP"
+
+
+gdb_test "reverse-continue" ".*" "setup for test 2"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test1"
+gdb_test_no_output "record" "turn on process record for test2"
+
+
+### TEST 2: reverse finish from the body of function1.
+
+# Set breakpoint at call to function1 in main.
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "at function1 entry point instruction to step to body of function" \
+    ".*$srcfile:$bp_LEP_test\r\n.*"
+
+# do a step instruction to get to the body of the function
+gdb_test "step" ".*int ret = 0;.*" "step test 1"
+
+# The reverse-finish command should stop on the function call instruction
+# which is the last instruction in the source code line.  A reverse-next
+# instruction should then stop at the first instruction in the same source
+# code line.  Another revers-next instruction stops at the previous source
+# code line.
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish function1 LEP call from function body"
+gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse next 1 LEP from function body"
+gdb_test "reverse-next" ".*b = 5;.*" \
+    "reverse next 2 at b = 5, from function body"
+
+gdb_test "reverse-continue" ".*" "setup for test 3"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test2"
+gdb_test_no_output "record" "turn on process record for test3"
+
+
+### TEST 3: reverse finish from the alternate entry point instruction (GEP) in
+### function1 when called using the alternate entry point (GEP).
+
+# Set breakpoint at call to funp in main.
+set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into funp" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# stepi until we see "{" indicating we entered function.
+repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
+
+# The reverse-finish command should stop on the function call instruction
+# which is the last instruction in the source code line.  A reverse-next
+# instruction should then stop at the first instruction in the same source
+# code line.  Another revers-next instruction stops at the previous source
+# code line.
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "function1 GEP call call from GEP"
+gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
+    "reverse next 1 GEP entry point function call from GEP"
+gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50, call from GEP"
+
+gdb_test "reverse-continue" ".*" "setup for test 4"
+
+# 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 between the GEP and LEP in
+### function1 when called using the alternate entry point (GEP).
+
+# Set breakpoint at call to funp in main.
+set bp_GEP_test  [gdb_get_line_number "CALL VIA GEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into funp again" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# stepi until we see "{" indicating we entered function.
+repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call again"
+
+# do one more stepi so we are between the GEP and LEP.
+gdb_test "stepi" "{" "stepi to between GEP and LEP"
+
+# The reverse-finish command should stop on the function call instruction
+# which is the last instruction in the source code line.  A reverse-next
+# instruction should then stop at the first instruction in the same source
+# code line.  Another revers-next instruction stops at the previous source
+# code line.
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "function1 GEP call call from GEP again"
+gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
+    "reverse next 1 GEP entry point function call from GEP again"
+gdb_test "reverse-next" ".*b = 50;.*" \
+    "reverse next 2 at b = 50, call from GEP again"
+
+gdb_test "reverse-continue" ".*" "setup for test 5"
+
+# Turn off record to clear logs and turn on again
+gdb_test "record stop"  "Process record is stopped.*" \
+    "turn off process record for test4"
+gdb_test_no_output "record" "turn on process record for test5"
+
+
+### TEST 5: reverse finish from the body of function 1 when calling using the
+### alternate entrypoint (GEP).
+gdb_breakpoint $srcfile:$bp_GEP_test temporary
+
+# Continue to break point at funp call.
+gdb_continue_to_breakpoint \
+    "at function1 entry point instruction to step to body of funp call" \
+    ".*$srcfile:$bp_GEP_test\r\n.*"
+
+# Step into body of funp, called via GEP.
+gdb_test "step" ".*int ret = 0;.*" "step test 2"
+
+# The reverse-finish command should stop on the function call instruction
+# which is the last instruction in the source code line.  A reverse-next
+# instruction should then stop at the first instruction in the same source
+# code line.  Another revers-next instruction stops at the previous source
+# code line.
+gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
+    "reverse-finish function1 GEP call, from function body  "
+gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
+    "reverse next 1 GEP call from function body"
+gdb_test "reverse-next" ".*b = 50;.*" \
+    "reverse next 2 at b = 50 from function body"
  

Carl Love <cel@us.ibm.com> wrote:

>I have updated gdb/infrun.c per the comments from Ulrich to remove
>"unnecessary" code and format changes to make the diff as minimal as I
>can.  There are no functional changes in these changes.
>
>The patch has been retested on PowerPC to make sure there are no
>regressions.
>
>Please let me know if this version of this patch (second in the series)
>is acceptable.  Thanks.

This version is OK.

Thanks,
Ulrich
  

On 3/17/23 18:19, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
>> I have updated gdb/infrun.c per the comments from Ulrich to remove
>> "unnecessary" code and format changes to make the diff as minimal as I
>> can.  There are no functional changes in these changes.
>>
>> The patch has been retested on PowerPC to make sure there are no
>> regressions.
>>
>> Please let me know if this version of this patch (second in the series)
>> is acceptable.  Thanks.
> 
> This version is OK.


I'm running into these regressions on x86_64-linux:
...
Running 
/data/vries/gdb/src/gdb/testsuite/gdb.reverse/finish-precsave.exp ...
FAIL: gdb.reverse/finish-precsave.exp: reverse finish from long_long_func
FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint: long_func 
backward
FAIL: gdb.reverse/finish-precsave.exp: reverse finish from long_func
FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint: int_func 
backward
FAIL: gdb.reverse/finish-precsave.exp: reverse finish from int_func
FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint: 
short_func backward
FAIL: gdb.reverse/finish-precsave.exp: reverse finish from short_func
FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint: char_func 
backward
FAIL: gdb.reverse/finish-precsave.exp: reverse finish from char_func
FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint: void_func 
backward
FAIL: gdb.reverse/finish-precsave.exp: reverse finish from void_func
Running /data/vries/gdb/src/gdb/testsuite/gdb.btrace/tailcall.exp ...
FAIL: gdb.btrace/tailcall.exp: reverse-finish.1
FAIL: gdb.btrace/tailcall.exp: reverse-step.2
FAIL: gdb.btrace/tailcall.exp: next.1
FAIL: gdb.btrace/tailcall.exp: reverse-next.1
FAIL: gdb.btrace/tailcall.exp: step.1
FAIL: gdb.btrace/tailcall.exp: finish.2
FAIL: gdb.btrace/tailcall.exp: reverse-step.3
FAIL: gdb.btrace/tailcall.exp: finish.3
Running /data/vries/gdb/src/gdb/testsuite/gdb.mi/mi-reverse.exp ...
FAIL: gdb.mi/mi-reverse.exp: reverse finish from callme (unknown output 
after running)
FAIL: gdb.mi/mi-reverse.exp: reverse next to get over the call to 
do_nothing (unknown output after running)
FAIL: gdb.mi/mi-reverse.exp: reverse step to callee1 (unknown output 
after running)
FAIL: gdb.mi/mi-reverse.exp: reverse step to callee2 (unknown output 
after running)
FAIL: gdb.mi/mi-reverse.exp: reverse step to callee3 (unknown output 
after running)
FAIL: gdb.mi/mi-reverse.exp: reverse step to callee4 (unknown output 
after running)
FAIL: gdb.mi/mi-reverse.exp: reverse-step-instruction at callee4 
(unknown output after running)
FAIL: gdb.mi/mi-reverse.exp: reverse-next-instruction at callee4 
(unknown output after running)
FAIL: gdb.mi/mi-reverse.exp: reverse-continue at callee3 (unknown output 
after running)
Running /data/vries/gdb/src/gdb/testsuite/gdb.btrace/step.exp ...
FAIL: gdb.btrace/step.exp: replay: reverse-finish.1
FAIL: gdb.btrace/step.exp: replay: reverse-next.2
FAIL: gdb.btrace/step.exp: replay: reverse-finish.2 (GDB internal error)
Running /data/vries/gdb/src/gdb/testsuite/gdb.reverse/until-precsave.exp ...
FAIL: gdb.reverse/until-precsave.exp: reverse-finish from marker2
FAIL: gdb.reverse/until-precsave.exp: reverse-advance to final return of 
factorial
FAIL: gdb.reverse/until-precsave.exp: reverse-until to entry of factorial
Running /data/vries/gdb/src/gdb/testsuite/gdb.reverse/finish-reverse.exp ...
FAIL: gdb.reverse/finish-reverse.exp: reverse finish from long_long_func
FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint: long_func 
backward
FAIL: gdb.reverse/finish-reverse.exp: reverse finish from long_func
FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint: int_func 
backward
FAIL: gdb.reverse/finish-reverse.exp: reverse finish from int_func
FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint: short_func 
backward
FAIL: gdb.reverse/finish-reverse.exp: reverse finish from short_func
FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint: char_func 
backward
FAIL: gdb.reverse/finish-reverse.exp: reverse finish from char_func
FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint: void_func 
backward
FAIL: gdb.reverse/finish-reverse.exp: reverse finish from void_func
Running 
/data/vries/gdb/src/gdb/testsuite/gdb.reverse/finish-reverse-next.exp ...
FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP 
call from LEP
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP entry 
point function call from LEP
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2, at b = 5, 
call from LEP
FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP 
call from function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from 
function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from 
function body
FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call from GEP
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry 
point function call from GEP
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50, 
call from GEP
FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call from 
GEP again
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry 
point function call from GEP again
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50, 
call from GEP again
FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 GEP 
call, from function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call from 
function body
FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50 from 
function body
Running /data/vries/gdb/src/gdb/testsuite/gdb.btrace/tailcall-only.exp ...
FAIL: gdb.btrace/tailcall-only.exp: reverse-step
FAIL: gdb.btrace/tailcall-only.exp: up
...

Thanks,
- Tom
  

On Sat, 2023-03-18 at 00:05 +0100, Tom de Vries wrote:
> On 3/17/23 18:19, Ulrich Weigand wrote:
> > Carl Love <cel@us.ibm.com> wrote:
> > 
> > > I have updated gdb/infrun.c per the comments from Ulrich to
> > > remove
> > > "unnecessary" code and format changes to make the diff as minimal
> > > as I
> > > can.  There are no functional changes in these changes.
> > > 
> > > The patch has been retested on PowerPC to make sure there are no
> > > regressions.
> > > 
> > > Please let me know if this version of this patch (second in the
> > > series)
> > > is acceptable.  Thanks.
> > 
> > This version is OK.
> 
> I'm running into these regressions on x86_64-linux:

I will take a look to see if I can reproduce the issues.  

                   Carl 
>
  

Tom, Luis:

On Sat, 2023-03-18 at 00:05 +0100, Tom de Vries wrote:
> On 3/17/23 18:19, Ulrich Weigand wrote:
> > Carl Love <cel@us.ibm.com> wrote:
> > 
> > > I have updated gdb/infrun.c per the comments from Ulrich to
> > > remove
> > > "unnecessary" code and format changes to make the diff as minimal
> > > as I
> > > can.  There are no functional changes in these changes.
> > > 
> > > The patch has been retested on PowerPC to make sure there are no
> > > regressions.
> > > 
> > > Please let me know if this version of this patch (second in the
> > > series)
> > > is acceptable.  Thanks.
> > 
> > This version is OK.
> 
> I'm running into these regressions on x86_64-linux:
> ...
> Running 
> /data/vries/gdb/src/gdb/testsuite/gdb.reverse/finish-precsave.exp ...
> FAIL: gdb.reverse/finish-precsave.exp: reverse finish from
> long_long_func
> FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint:
> long_func 
> backward
> FAIL: gdb.reverse/finish-precsave.exp: reverse finish from long_func
> FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint:
> int_func 
> backward
> FAIL: gdb.reverse/finish-precsave.exp: reverse finish from int_func
> FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint: 
> short_func backward
> FAIL: gdb.reverse/finish-precsave.exp: reverse finish from short_func
> FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint:
> char_func 
> backward
> FAIL: gdb.reverse/finish-precsave.exp: reverse finish from char_func
> FAIL: gdb.reverse/finish-precsave.exp: continue to breakpoint:
> void_func 
> backward
> FAIL: gdb.reverse/finish-precsave.exp: reverse finish from void_func
> Running /data/vries/gdb/src/gdb/testsuite/gdb.btrace/tailcall.exp ...
> FAIL: gdb.btrace/tailcall.exp: reverse-finish.1
> FAIL: gdb.btrace/tailcall.exp: reverse-step.2
> FAIL: gdb.btrace/tailcall.exp: next.1
> FAIL: gdb.btrace/tailcall.exp: reverse-next.1
> FAIL: gdb.btrace/tailcall.exp: step.1
> FAIL: gdb.btrace/tailcall.exp: finish.2
> FAIL: gdb.btrace/tailcall.exp: reverse-step.3
> FAIL: gdb.btrace/tailcall.exp: finish.3
> Running /data/vries/gdb/src/gdb/testsuite/gdb.mi/mi-reverse.exp ...
> FAIL: gdb.mi/mi-reverse.exp: reverse finish from callme (unknown
> output 
> after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse next to get over the call to 
> do_nothing (unknown output after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse step to callee1 (unknown output 
> after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse step to callee2 (unknown output 
> after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse step to callee3 (unknown output 
> after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse step to callee4 (unknown output 
> after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse-step-instruction at callee4 
> (unknown output after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse-next-instruction at callee4 
> (unknown output after running)
> FAIL: gdb.mi/mi-reverse.exp: reverse-continue at callee3 (unknown
> output 
> after running)
> Running /data/vries/gdb/src/gdb/testsuite/gdb.btrace/step.exp ...
> FAIL: gdb.btrace/step.exp: replay: reverse-finish.1
> FAIL: gdb.btrace/step.exp: replay: reverse-next.2
> FAIL: gdb.btrace/step.exp: replay: reverse-finish.2 (GDB internal
> error)
> Running /data/vries/gdb/src/gdb/testsuite/gdb.reverse/until-
> precsave.exp ...
> FAIL: gdb.reverse/until-precsave.exp: reverse-finish from marker2
> FAIL: gdb.reverse/until-precsave.exp: reverse-advance to final return
> of 
> factorial
> FAIL: gdb.reverse/until-precsave.exp: reverse-until to entry of
> factorial
> Running /data/vries/gdb/src/gdb/testsuite/gdb.reverse/finish-
> reverse.exp ...
> FAIL: gdb.reverse/finish-reverse.exp: reverse finish from
> long_long_func
> FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint:
> long_func 
> backward
> FAIL: gdb.reverse/finish-reverse.exp: reverse finish from long_func
> FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint:
> int_func 
> backward
> FAIL: gdb.reverse/finish-reverse.exp: reverse finish from int_func
> FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint:
> short_func 
> backward
> FAIL: gdb.reverse/finish-reverse.exp: reverse finish from short_func
> FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint:
> char_func 
> backward
> FAIL: gdb.reverse/finish-reverse.exp: reverse finish from char_func
> FAIL: gdb.reverse/finish-reverse.exp: continue to breakpoint:
> void_func 
> backward
> FAIL: gdb.reverse/finish-reverse.exp: reverse finish from void_func
> Running 
> /data/vries/gdb/src/gdb/testsuite/gdb.reverse/finish-reverse-next.exp 
> ...
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
> LEP 
> call from LEP
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP entry 
> point function call from LEP
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2, at b = 5, 
> call from LEP
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
> LEP 
> call from function body
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from 
> function body
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5,
> from 
> function body
> FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call
> from GEP
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry 
> point function call from GEP
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50, 
> call from GEP
> FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call
> from 
> GEP again
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry 
> point function call from GEP again
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50, 
> call from GEP again
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
> GEP 
> call, from function body
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call
> from 
> function body
> FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50
> from 
> function body
> Running /data/vries/gdb/src/gdb/testsuite/gdb.btrace/tailcall-
> only.exp ...
> FAIL: gdb.btrace/tailcall-only.exp: reverse-step
> FAIL: gdb.btrace/tailcall-only.exp: up

I believe I have found the issue.  Somewhere along the line, probably
in the code cleanup, the initialization of the new variables in
gdb/infcmd.c got out of order.  Basically, alt_entry_point and
entry_point are initialized to sal.pc.  However, sal has been declared
but not initialized so alt_entry_point and entry_point are always equal
to zero.  That doesn't work so well.  The fix is simple, just need to
move the initialization of the two variables after sal has been
initialized.  

I am still running some tests to make sure everything is OK.  But so
far it looks like the fix works on PowerPC and on one of the X86 boxes
I am testing on.  I am double checking the results on the other X86
box.  I hope to post a fix soon.

This should fix the errors on X86 and Arm.

Sorry for the regression.

                   Carl
  

Tom, Luis, GDB maintainers:

The recent commit to fix the reverse-finish command on PowerPC
introduced some regression test failures on X86 and Arm.  The following
patch fixes the failures.

The fix has been tested on Power10 and X86.  

Please let me know if this patch is acceptable.  Thanks.

                  Carl 

----------------------------------------
PowerPC: regression fix for reverse-finish command.

The recent commit:

  commit 2a8339b71f37f2d02f5b2194929c9d702ef27223
  Author: Carl Love <cel@us.ibm.com>
  Date:   Thu Mar 9 16:10:18 2023 -0500

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

   PPC64 multiple entry points, a normal entry point and an alternate entry
   point.  The alternate entry point is to setup the Table of Contents (TOC)
   register before continuing at the normal entry point.  When the TOC is
   already valid, the normal entry point is used, this is typically the case.
   The alternate entry point is typically referred to as the global entry
   point (GEP) in IBM.  The normal entry point is typically referred to as
   the local entry point (LEP).
     .....

Is causing regression failures on on PowerPC platforms.  The regression
failures are in tests:

  gdb.reverse/finish-precsave.exp
  gdb.btrace/tailcall.exp
  gdb.mi/mi-reverse.exp
  gdb.btrace/step.exp
  gdb.reverse/until-precsave.exp
  gdb.reverse/finish-reverse.exp
  gdb.btrace/tailcall-only.exp

The issue is in gdb/infcmd.c, function finish_command.  The value of the
two new variables ALT_ENTRY_POINT and ENTRY_POINT are being initializezed
to SAL.PC.  However, SAL has just been declared.  The value of SAL.PC is
zero at this point.  The intialization of ALT_ENTRY_POINT and ENTRY_POINT
needs to be after the initialization of SAL.

This patch moves the initialization of ALT_ENTRY_POINT and ENTRY_POINT
variables to fix the regression failures.

The patch has been tested on Power10 and on X86.
---
 gdb/infcmd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f46461512fe..e2032d18564 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1710,8 +1710,8 @@ finish_backward (struct finish_command_fsm *sm)
   struct thread_info *tp = inferior_thread ();
   CORE_ADDR pc;
   CORE_ADDR func_addr;
-  CORE_ADDR alt_entry_point = sal.pc;
-  CORE_ADDR entry_point = alt_entry_point;
+  CORE_ADDR alt_entry_point;
+  CORE_ADDR entry_point;
   frame_info_ptr frame = get_selected_frame (nullptr);
   struct gdbarch *gdbarch = get_frame_arch (frame);
 
@@ -1721,6 +1721,8 @@ finish_backward (struct finish_command_fsm *sm)
     error (_("Cannot find bounds of current function"));
 
   sal = find_pc_line (func_addr, 0);
+  alt_entry_point = sal.pc;
+  entry_point = alt_entry_point;
 
   if (gdbarch_skip_entrypoint_p (gdbarch))
     /* Some architectures, like PowerPC use local and global entry points.
  

Carl Love <cel@us.ibm.com> wrote:

>PowerPC: regression fix for reverse-finish command.

This is OK.

Thanks,
Ulrich
  

On 3/1/23 15:59, Carl Love via Gdb-patches wrote:
> Tom, Ulrich, Bruno, Pedro, GDB maintainers:
> 
> This patch fixes the reverse-finish command on PowerPC.  The command
> now works the same as on other architectures, specifically X86.  There
> are no functional changes for other architectures.  The patch includes
> a new testcase to verify the reverse-finish command works correctly
> with the multiple entry points supported by PowerPC.
> 
> 
> Patch tested on PowerPC and 5th generation X86 with no regression
> failures.
> 
>                   Carl 

Hi Carl,

I don't know if that particular failure has been reported yet, but I see
these failures when running with native-gdbserver or
native-extended-gdbserver:

  $ make check TESTS="gdb.reverse/finish-reverse-next.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP call from LEP
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP entry point function call from LEP
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2, at b = 5, call from LEP
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP call from function body
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body
  FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call from GEP
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry point function call from GEP
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50, call from GEP
  FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call from GEP again
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry point function call from GEP again
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50, call from GEP again
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 GEP call, from function body
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call from function body
  FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50 from function body

Simon
  

On Fri, 2023-03-24 at 13:23 -0400, Simon Marchi wrote:
> 

<snip>

> I don't know if that particular failure has been reported yet, but I
> see
> these failures when running with native-gdbserver or
> native-extended-gdbserver:
> 
>   $ make check TESTS="gdb.reverse/finish-reverse-next.exp"
> RUNTESTFLAGS="--target_board=native-gdbserver"
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
> LEP call from LEP
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP entry
> point function call from LEP
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2, at b =
> 5, call from LEP
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
> LEP call from function body
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from
> function body
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5,
> from function body
>   FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call
> from GEP
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry
> point function call from GEP
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b =
> 50, call from GEP
>   FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call
> from GEP again
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry
> point function call from GEP again
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b =
> 50, call from GEP again
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
> GEP call, from function body
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call
> from function body
>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50
> from function body

Yes, there was a regression failure.  The following was committed to
fix the reported regression issues.

https://sourceware.org/pipermail/gdb-patches/2023-March/198139.html

Are you testing with this fix?

I don't normally run the tests with --target_board=native-gdbserver but
I did try that and it seemed to work for me.  Note I had to also
specify GDBFLAGS to get the test to run.  Specifically, I used the
command:

make check TESTS="gdb.reverse/finish-reverse-next.exp"  RUNTESTFLAGS="GDBFLAGS=' ' --target_board=native-gdbserver "

The test ran fine and reported 40 passes and no errors.
                 
       Carl
  

On 3/24/23 18:16, Carl Love wrote:
> On Fri, 2023-03-24 at 13:23 -0400, Simon Marchi wrote:
>>
> 
> <snip>
> 
>> I don't know if that particular failure has been reported yet, but I
>> see
>> these failures when running with native-gdbserver or
>> native-extended-gdbserver:
>>
>>   $ make check TESTS="gdb.reverse/finish-reverse-next.exp"
>> RUNTESTFLAGS="--target_board=native-gdbserver"
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
>> LEP call from LEP
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP entry
>> point function call from LEP
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2, at b =
>> 5, call from LEP
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
>> LEP call from function body
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from
>> function body
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5,
>> from function body
>>   FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call
>> from GEP
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry
>> point function call from GEP
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b =
>> 50, call from GEP
>>   FAIL: gdb.reverse/finish-reverse-next.exp: function1 GEP call call
>> from GEP again
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP entry
>> point function call from GEP again
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b =
>> 50, call from GEP again
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse-finish function1
>> GEP call, from function body
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call
>> from function body
>>   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50
>> from function body
> 
> Yes, there was a regression failure.  The following was committed to
> fix the reported regression issues.
> 
> https://sourceware.org/pipermail/gdb-patches/2023-March/198139.html

I also saw these regressions, but they are fixed (by that commit).  This
seems like a different thing.

> Are you testing with this fix?

Yes, and I just tried with today's master, same thing.

> I don't normally run the tests with --target_board=native-gdbserver but
> I did try that and it seemed to work for me.  Note I had to also
> specify GDBFLAGS to get the test to run.  Specifically, I used the
> command:
> 
> make check TESTS="gdb.reverse/finish-reverse-next.exp"  RUNTESTFLAGS="GDBFLAGS=' ' --target_board=native-gdbserver "

That's odd, I never had to do that thing with GDBFLAGS.  What happens
when you don't specify it?

For reference, here's an example failure:

147 reverse-next^M
148 81        b = 5;^M 
149 (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body
150 reverse-next^M
151 80        a = 1;^M
152 (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body

Simon
  

Simon:

On Sat, 2023-03-25 at 08:39 -0400, Simon Marchi wrote:
> 

<snip>

> > 
> > Yes, there was a regression failure.  The following was committed
> > to
> > fix the reported regression issues.
> > 
> > https://sourceware.org/pipermail/gdb-patches/2023-March/198139.html 
> 
> I also saw these regressions, but they are fixed (by that
> commit).  This
> seems like a different thing.
> 
> > Are you testing with this fix?
> 
> Yes, and I just tried with today's master, same thing.
> 
> > I don't normally run the tests with --target_board=native-gdbserver 
> > but
> > I did try that and it seemed to work for me.  Note I had to also
> > specify GDBFLAGS to get the test to run.  Specifically, I used the
> > command:
> > 
> > make check TESTS="gdb.reverse/finish-reverse-
> > next.exp"  RUNTESTFLAGS="GDBFLAGS=' ' --target_board=native-
> > gdbserver "
> 
> That's odd, I never had to do that thing with GDBFLAGS.  What happens
> when you don't specify it?

When I run on Power 10 without GDBFLAGS I get the message:

   make check TESTS="gdb.reverse/finish-reverse-next.exp"  RUNTESTFLAGS=" --target_board=native-gdbserver " 

   <snip>

   ERROR: tcl error sourcing board description file for target /home.../gdb/testsuite/boards/gdbserver-base.exp.
   can't read "GDBFLAGS": no such variable
   can't read "GDBFLAGS": no such variable
       while executing
   "set GDBFLAGS "${GDBFLAGS} -iex \"set auto-connect-native-target off\"""
       (file "/home.../gdb/testsuite/boards/gdbserver-base.exp" line 35)
       invoked from within
   "source /home.../gdb/testsuite/boards/gdbserver-base.exp"
       ("uplevel" body line 1)
       invoked from within
   "uplevel #0 source /home/.../gdb/testsuite/boards/gdbserver-base.exp"
       invoked from within
   "catch "uplevel #0 source $filename" error"
   make[5]: *** [Makefile:1829: check-DEJAGNU] Error 1

> 
> For reference, here's an example failure:
> 
> 147 reverse-next^M
> 148 81        b = 5;^M 
> 149 (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1
> LEP from function body
> 150 reverse-next^M
> 151 80        a = 1;^M
> 152 (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2
> at b = 5, from function body

You didn't mention what platform you are running on, but I am guessing
X86.  I created a little run script to run the test with and without
the --target-board argument on Power 10.  Here is my script.

    more check-reverse

   echo " "
   echo reverse.exp
   rm testsuite/gdb.log
   make check RUNTESTFLAGS='GDB=/home/carll/bin/gdb   gdb.reverse/finish-reverse-next.exp   ' > out-reverse-next
   grep "of expected passes" testsuite/gdb.log
   grep "of unexpected failures" testsuite/gdb.log
   mv testsuite/gdb.log testsuite/gdb.log-reverse-next


   echo " "
   echo reverse.exp
   rm testsuite/gdb.log
   make check TESTS="gdb.reverse/finish-reverse-next.exp"  RUNTESTFLAGS="GDBFLAGS=' ' --target_board=native-gdbserver "  > out-reverse-next-gdbserver
   grep "of expected passes" testsuite/gdb.log
   grep "of unexpected failures" testsuite/gdb.log
   mv testsuite/gdb.log testsuite/gdb.log-reverse-next-gdbserver

So, assuming you are on X86, I copied the script to my X86 box and tried it out there as well.

I ran the script in the top of the build directory.  For the first test
without --target_board I get the results:

   Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
      Using /.../gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
      Running /.../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/gdb.reverse/finish-reverse-next.exp ...

                   === gdb Summary ===

      # of expected passes            40
      gdb version  14.0.50.20230324-git -nw -nx -iex "set height 0" -iex "set width 0"

                   === gdb Summary ===

      # of expected passes            40
      gdb version  14.0.50.20230324-git -nw -nx -iex "set height 0" -iex "set width 0"

The second test with the --target_board I get a number of failures and
never actually get to run the finish-reverse-next test.  The output I
see is:

   Running target native-gdbserver
   Using /usr/share/dejagnu/baseboards/native-gdbserver.exp as board description file for target.
   Using /home/carll/GDB/binutils-gdb-finish-precsave-PPC-Only/gprofng/testsuite/config/default.exp as tool-and-target-specific interface file.
   Running /home/carll/GDB/binutils-gdb-finish-precsave-PPC-Only/gprofng/testsuite/gprofng.display/display.exp ...
   ERROR: comparison of results in mttest failed
   ERROR: comparison of results in mttest failed
   ERROR: comparison of results in synprog failed
   ERROR: comparison of results in synprog failed

   		=== gprofng Summary ===

   # of unresolved testcases       4
   # of unsupported tests          1
   make[4]: Leaving directory '/home/.../gprofng'
   make[3]: Leaving directory '/home/.../gprofng'
   make[2]: Leaving directory '/home/.../gprofng'
   make[1]: Leaving directory '/home/...'

So, tried changing the paths in the script from gdb/testscript to
testscript and then ran the script in the subdirectory gdb.  When I do
that, I get the same results for the first test as I saw befor.  Now,
the second test with --target_board runs and I get the following
output:

   ...
   Using /home/.../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/boards/../boards/gdbserver-base.exp as board de
   scription file for target.
   Using /home/.../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/boards/../boards/local-board.exp as board description file for target.
   Using /home/carll/GDB/build-finish-precsave-PPC-Only/gdb/testsuite/../../../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/config/gdbserver.exp as tool-and-target-specifi
   c interface file.
   Running /home/carll/GDB/build-finish-precsave-PPC-Only/gdb/testsuite/../../../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/gdb.reverse/finish-reverse-next.exp ...
   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body
   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body
   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call from function body
   FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50 from function body

   		=== gdb Summary ===

   # of expected passes		36
   # of unexpected failures	4

which appears to match the failures you saw.  Why the --target-board
argument seems to make a difference is a mystery to me at this point!!

I can see what what happens...  So, when you do a reverse-finish, gdb
stops at the branch instruction for the function call.  Then when you
do a reverse-next and gdb stops at the beginning of the same source
code line.  This is the expected behavior for gdb per the discussion we
had a few months ago.  Hence in the test file we have:

   # The reverse-finish command should stop on the function call instruction                              
   # which is the last instruction in the source code line.  A reverse-next                               
   # instruction should then stop at the first instruction in the same source                             
   # code line.  Another revers-next instruction stops at the previous source                             
   # code line.                                                                                           
   gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
       "reverse-finish function1 LEP call from function body"
   gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
       "reverse next 1 LEP from function body"
   gdb_test "reverse-next" ".*b = 5;.*" \
       "reverse next 2 at b = 5, from function body"

The test is looking to see that it is at source line function1 (a, b)
after both reverse-finish and the reverse-next commands.

Looking in the output file for the run with and without the --target-
board on X86, we see:

without the target board specified:


   step
   function1 (a=1, b=5) at /home/carll/.../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
   51        int ret = 0;
   (gdb) PASS: gdb.reverse/finish-reverse-next.exp: step test 1

   reverse-finish
   Run back to call of #0  function1 (a=1, b=5) at /home/carll/..../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
   0x00005555555551cb in main (argc=1, argv=0x7fffffffde88) at /home/carll/.../gdb/testsuite/gdb.reverse/finish-reverse-next.c:83^M
   83        function1 (a, b);   // CALL VIA LEP
   (gdb) PASS: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP call from function body

   reverse-next
   83        function1 (a, b);   // CALL VIA LEP
   (gdb) PASS: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body reverse-next
   81        b = 5;

   reverse-continue
   Continuing.

   No more reverse-execution history.

   We see the reverse-finish and the reverse-next both stop at function1
   (a, b) as expected.

   But with  --target-board, we see:

   step
   function1 (a=1, b=5) at /home/carll/..../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
   51        int ret = 0;
   (gdb) PASS: gdb.reverse/finish-reverse-next.exp: step test 1

   reverse-finish
   Run back to call of #0  function1 (a=1, b=5) at /home/carll/..../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
   main (argc=1, argv=0x7fffffffde88) at /home/carll/.../gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
   83        function1 (a, b);   // CALL VIA LEP
   (gdb) PASS: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP call from function body

   reverse-next
   81        b = 5;
   (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body

   reverse-next
   80        a = 1;
   (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body

   reverse-continue
   Continuing.

It appears the reverse-finish stopped at the beginning of the source
line, not the function call as expected, because the reverse-next stops
at the previous source line not the same source line as expected.  I
think it would only do that if GDB was at the beginning of the source
line after the reverse-finish command is done.  The issue "appears to
be" that the reverse-finish command behaves differently with the --
target_board???

I am at a loss as to why using gdbserver causes this change in the
behavior of the test.  Unfortunately, I have never looked at how
gdbserver works so I don't know what the issue might be.  

I tried rewinding my gdb tree on X86 to the commit
  
commit 334d405c2ac395fca67b952affb20893002d969f (HEAD)
Author: Carl Love <cel@us.ibm.com>
Date:   Wed Mar 1 11:45:43 2023 -0500

    Move step_until procedure
    ....

which is the commit before the fix for the reverse-finish command on
Power10.  Note, the finish-reverse-next is a new test that is added
with the Power 10 reverse-finish command fix.  I had to make a copy of 
finish-reverse-next.c and finish-reverse-next.exp before rewinding the
source tree and restoring them after the rewind.  I rebuilt gdb without
the PowerPC fix.  When I run my script I see the exact same behavior on
X86, i.e. the first test works fine, the second test fails.  So, it
looks to me like the issue is independent of the changes made in my
commit for Power10.  

It would probably be good if you can try to reproduce the failure
without the Power10 fix.  Note, you do need the patch to move the
step_until proceedure so when you restore the test it will work.  The
test is dependent on that commit.

Let me know if you can verify the test fails without the Power10 fix
and with --target_board.  

When I run the above script on Power 10, the first test runs fine and
is the only test that is run.  The second test results in running a
whole bunch of tests in addition to finish-reverse-next.  I have to
search thru the log file to find where the test ran.  No errors are
reported for the finish-reverse-next test.  But I do see errors in
other tests.  Actually, the output looks like what I get when I run
"make check".  I looked to see if I could find an error about the
arguments that would explain why the test "appears" to ignore the
request to run one test and runs what appears to be everything.  I
don't see reported errors or warnings.  I also tried moving the script
to the gdb build directory/gdb, like what I did on X86, and tweaked the
paths in the script.  Again, the first test runs just fine and appears
to be the only test that is run.  The second test seems to actually run
all of the tests again.  When I look in the log file, again I can see
where finish-reverse-next ran and doesn't appear to generate any errors
but I am seeing all these other tests that ran.

In summary, there seems to be an issue with using the --target_board
argument.  On X86, the reverse-finish command seems to work a little
different with and without the --target-board command.  This is
independent of the Power 10 reverse-finish patch.  On Power 10, the use
of the --target_board option seems to cause all tests to run not just
the one requested.  Maybe there is something wrong with the make
command in the test file that explains the behavior on Power 10??
Unfortunately, I have not worked on gdbserver so I really have no idea
what is going on.

                        Carl
  

On 3/27/23 19:59, Carl Love wrote:
> Simon:
> 
> On Sat, 2023-03-25 at 08:39 -0400, Simon Marchi wrote:
>>
> 
> <snip>
> 
>>>
>>> Yes, there was a regression failure.  The following was committed
>>> to
>>> fix the reported regression issues.
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2023-March/198139.html 
>>
>> I also saw these regressions, but they are fixed (by that
>> commit).  This
>> seems like a different thing.
>>
>>> Are you testing with this fix?
>>
>> Yes, and I just tried with today's master, same thing.
>>
>>> I don't normally run the tests with --target_board=native-gdbserver 
>>> but
>>> I did try that and it seemed to work for me.  Note I had to also
>>> specify GDBFLAGS to get the test to run.  Specifically, I used the
>>> command:
>>>
>>> make check TESTS="gdb.reverse/finish-reverse-
>>> next.exp"  RUNTESTFLAGS="GDBFLAGS=' ' --target_board=native-
>>> gdbserver "
>>
>> That's odd, I never had to do that thing with GDBFLAGS.  What happens
>> when you don't specify it?
> 
> When I run on Power 10 without GDBFLAGS I get the message:
> 
>    make check TESTS="gdb.reverse/finish-reverse-next.exp"  RUNTESTFLAGS=" --target_board=native-gdbserver " 
> 
>    <snip>
> 
>    ERROR: tcl error sourcing board description file for target /home.../gdb/testsuite/boards/gdbserver-base.exp.
>    can't read "GDBFLAGS": no such variable
>    can't read "GDBFLAGS": no such variable
>        while executing
>    "set GDBFLAGS "${GDBFLAGS} -iex \"set auto-connect-native-target off\"""
>        (file "/home.../gdb/testsuite/boards/gdbserver-base.exp" line 35)
>        invoked from within
>    "source /home.../gdb/testsuite/boards/gdbserver-base.exp"
>        ("uplevel" body line 1)
>        invoked from within
>    "uplevel #0 source /home/.../gdb/testsuite/boards/gdbserver-base.exp"
>        invoked from within
>    "catch "uplevel #0 source $filename" error"
>    make[5]: *** [Makefile:1829: check-DEJAGNU] Error 1
> 
>>
>> For reference, here's an example failure:
>>
>> 147 reverse-next^M
>> 148 81        b = 5;^M 
>> 149 (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1
>> LEP from function body
>> 150 reverse-next^M
>> 151 80        a = 1;^M
>> 152 (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2
>> at b = 5, from function body
> 
> You didn't mention what platform you are running on, but I am guessing
> X86.  I created a little run script to run the test with and without
> the --target-board argument on Power 10.  Here is my script.
> 
>     more check-reverse
> 
>    echo " "
>    echo reverse.exp
>    rm testsuite/gdb.log
>    make check RUNTESTFLAGS='GDB=/home/carll/bin/gdb   gdb.reverse/finish-reverse-next.exp   ' > out-reverse-next
>    grep "of expected passes" testsuite/gdb.log
>    grep "of unexpected failures" testsuite/gdb.log
>    mv testsuite/gdb.log testsuite/gdb.log-reverse-next
> 
> 
>    echo " "
>    echo reverse.exp
>    rm testsuite/gdb.log
>    make check TESTS="gdb.reverse/finish-reverse-next.exp"  RUNTESTFLAGS="GDBFLAGS=' ' --target_board=native-gdbserver "  > out-reverse-next-gdbserver
>    grep "of expected passes" testsuite/gdb.log
>    grep "of unexpected failures" testsuite/gdb.log
>    mv testsuite/gdb.log testsuite/gdb.log-reverse-next-gdbserver
> 
> So, assuming you are on X86, I copied the script to my X86 box and tried it out there as well.
> 
> I ran the script in the top of the build directory.  For the first test
> without --target_board I get the results:
> 
>    Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
>       Using /.../gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
>       Running /.../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/gdb.reverse/finish-reverse-next.exp ...
> 
>                    === gdb Summary ===
> 
>       # of expected passes            40
>       gdb version  14.0.50.20230324-git -nw -nx -iex "set height 0" -iex "set width 0"
> 
>                    === gdb Summary ===
> 
>       # of expected passes            40
>       gdb version  14.0.50.20230324-git -nw -nx -iex "set height 0" -iex "set width 0"
> 
> The second test with the --target_board I get a number of failures and
> never actually get to run the finish-reverse-next test.  The output I
> see is:
> 
>    Running target native-gdbserver
>    Using /usr/share/dejagnu/baseboards/native-gdbserver.exp as board description file for target.
>    Using /home/carll/GDB/binutils-gdb-finish-precsave-PPC-Only/gprofng/testsuite/config/default.exp as tool-and-target-specific interface file.
>    Running /home/carll/GDB/binutils-gdb-finish-precsave-PPC-Only/gprofng/testsuite/gprofng.display/display.exp ...
>    ERROR: comparison of results in mttest failed
>    ERROR: comparison of results in mttest failed
>    ERROR: comparison of results in synprog failed
>    ERROR: comparison of results in synprog failed
> 
>    		=== gprofng Summary ===
> 
>    # of unresolved testcases       4
>    # of unsupported tests          1
>    make[4]: Leaving directory '/home/.../gprofng'
>    make[3]: Leaving directory '/home/.../gprofng'
>    make[2]: Leaving directory '/home/.../gprofng'
>    make[1]: Leaving directory '/home/...'
> 
> So, tried changing the paths in the script from gdb/testscript to
> testscript and then ran the script in the subdirectory gdb.  When I do
> that, I get the same results for the first test as I saw befor.  Now,
> the second test with --target_board runs and I get the following
> output:
> 
>    ...
>    Using /home/.../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/boards/../boards/gdbserver-base.exp as board de
>    scription file for target.
>    Using /home/.../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/boards/../boards/local-board.exp as board description file for target.
>    Using /home/carll/GDB/build-finish-precsave-PPC-Only/gdb/testsuite/../../../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/config/gdbserver.exp as tool-and-target-specifi
>    c interface file.
>    Running /home/carll/GDB/build-finish-precsave-PPC-Only/gdb/testsuite/../../../binutils-gdb-finish-precsave-PPC-Only/gdb/testsuite/gdb.reverse/finish-reverse-next.exp ...
>    FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body
>    FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body
>    FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 GEP call from function body
>    FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 50 from function body
> 
>    		=== gdb Summary ===
> 
>    # of expected passes		36
>    # of unexpected failures	4
> 
> which appears to match the failures you saw.  Why the --target-board
> argument seems to make a difference is a mystery to me at this point!!

Ahh, that must be it.  You were running the make check at the top-level.
We pretty much always run it inside gdb/ or gdb/testsuite/.

> I can see what what happens...  So, when you do a reverse-finish, gdb
> stops at the branch instruction for the function call.  Then when you
> do a reverse-next and gdb stops at the beginning of the same source
> code line.  This is the expected behavior for gdb per the discussion we
> had a few months ago.  Hence in the test file we have:
> 
>    # The reverse-finish command should stop on the function call instruction                              
>    # which is the last instruction in the source code line.  A reverse-next                               
>    # instruction should then stop at the first instruction in the same source                             
>    # code line.  Another revers-next instruction stops at the previous source                             
>    # code line.                                                                                           
>    gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
>        "reverse-finish function1 LEP call from function body"
>    gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
>        "reverse next 1 LEP from function body"
>    gdb_test "reverse-next" ".*b = 5;.*" \
>        "reverse next 2 at b = 5, from function body"
> 
> The test is looking to see that it is at source line function1 (a, b)
> after both reverse-finish and the reverse-next commands.
> 
> Looking in the output file for the run with and without the --target-
> board on X86, we see:
> 
> without the target board specified:
> 
> 
>    step
>    function1 (a=1, b=5) at /home/carll/.../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
>    51        int ret = 0;
>    (gdb) PASS: gdb.reverse/finish-reverse-next.exp: step test 1
> 
>    reverse-finish
>    Run back to call of #0  function1 (a=1, b=5) at /home/carll/..../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
>    0x00005555555551cb in main (argc=1, argv=0x7fffffffde88) at /home/carll/.../gdb/testsuite/gdb.reverse/finish-reverse-next.c:83^M
>    83        function1 (a, b);   // CALL VIA LEP
>    (gdb) PASS: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP call from function body
> 
>    reverse-next
>    83        function1 (a, b);   // CALL VIA LEP
>    (gdb) PASS: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body reverse-next
>    81        b = 5;
> 
>    reverse-continue
>    Continuing.
> 
>    No more reverse-execution history.
> 
>    We see the reverse-finish and the reverse-next both stop at function1
>    (a, b) as expected.
> 
>    But with  --target-board, we see:
> 
>    step
>    function1 (a=1, b=5) at /home/carll/..../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
>    51        int ret = 0;
>    (gdb) PASS: gdb.reverse/finish-reverse-next.exp: step test 1
> 
>    reverse-finish
>    Run back to call of #0  function1 (a=1, b=5) at /home/carll/..../gdb/testsuite/gdb.reverse/finish-reverse-next.c:51
>    main (argc=1, argv=0x7fffffffde88) at /home/carll/.../gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
>    83        function1 (a, b);   // CALL VIA LEP
>    (gdb) PASS: gdb.reverse/finish-reverse-next.exp: reverse-finish function1 LEP call from function body
> 
>    reverse-next
>    81        b = 5;
>    (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 1 LEP from function body
> 
>    reverse-next
>    80        a = 1;
>    (gdb) FAIL: gdb.reverse/finish-reverse-next.exp: reverse next 2 at b = 5, from function body
> 
>    reverse-continue
>    Continuing.
> 
> It appears the reverse-finish stopped at the beginning of the source
> line, not the function call as expected, because the reverse-next stops
> at the previous source line not the same source line as expected.  I
> think it would only do that if GDB was at the beginning of the source
> line after the reverse-finish command is done.  The issue "appears to
> be" that the reverse-finish command behaves differently with the --
> target_board???
> 
> I am at a loss as to why using gdbserver causes this change in the
> behavior of the test.  Unfortunately, I have never looked at how
> gdbserver works so I don't know what the issue might be.  
> 
> I tried rewinding my gdb tree on X86 to the commit
>   
> commit 334d405c2ac395fca67b952affb20893002d969f (HEAD)
> Author: Carl Love <cel@us.ibm.com>
> Date:   Wed Mar 1 11:45:43 2023 -0500
> 
>     Move step_until procedure
>     ....
> 
> which is the commit before the fix for the reverse-finish command on
> Power10.  Note, the finish-reverse-next is a new test that is added
> with the Power 10 reverse-finish command fix.  I had to make a copy of 
> finish-reverse-next.c and finish-reverse-next.exp before rewinding the
> source tree and restoring them after the rewind.  I rebuilt gdb without
> the PowerPC fix.  When I run my script I see the exact same behavior on
> X86, i.e. the first test works fine, the second test fails.  So, it
> looks to me like the issue is independent of the changes made in my
> commit for Power10.  
> 
> It would probably be good if you can try to reproduce the failure
> without the Power10 fix.  Note, you do need the patch to move the
> step_until proceedure so when you restore the test it will work.  The
> test is dependent on that commit.
> 
> Let me know if you can verify the test fails without the Power10 fix
> and with --target_board.  
> 
> When I run the above script on Power 10, the first test runs fine and
> is the only test that is run.  The second test results in running a
> whole bunch of tests in addition to finish-reverse-next.  I have to
> search thru the log file to find where the test ran.  No errors are
> reported for the finish-reverse-next test.  But I do see errors in
> other tests.  Actually, the output looks like what I get when I run
> "make check".  I looked to see if I could find an error about the
> arguments that would explain why the test "appears" to ignore the
> request to run one test and runs what appears to be everything.  I
> don't see reported errors or warnings.  I also tried moving the script
> to the gdb build directory/gdb, like what I did on X86, and tweaked the
> paths in the script.  Again, the first test runs just fine and appears
> to be the only test that is run.  The second test seems to actually run
> all of the tests again.  When I look in the log file, again I can see
> where finish-reverse-next ran and doesn't appear to generate any errors
> but I am seeing all these other tests that ran.
> 
> In summary, there seems to be an issue with using the --target_board
> argument.  On X86, the reverse-finish command seems to work a little
> different with and without the --target-board command.  This is
> independent of the Power 10 reverse-finish patch.  On Power 10, the use
> of the --target_board option seems to cause all tests to run not just
> the one requested.  Maybe there is something wrong with the make
> command in the test file that explains the behavior on Power 10??
> Unfortunately, I have not worked on gdbserver so I really have no idea
> what is going on.

Indeed, it just looks like a pre-existing issue that the new test has
uncovered.  Don't worry, it happens all the time in GDB.

I was able to reproduce by hand, and used the "maintenance print
record-instruction" command to print the recorded instructions in both
cases.  In the case of native debugging, these are all the recorded
instructions between the beginning of the function call line to the end
of the recording:

    (gdb) maintenance print record-instruction -9
    Register rdx changed: 140737488346408
[1] Register rip changed: (void (*)()) 0x5555555551a5 <main+40>
    (gdb) maintenance print record-instruction -8
    Register rax changed: 93824992235839
    Register rip changed: (void (*)()) 0x5555555551a8 <main+43>
    (gdb) maintenance print record-instruction -7
    Register rsi changed: 140737488346392
    Register rip changed: (void (*)()) 0x5555555551ab <main+46>
    (gdb) maintenance print record-instruction -6
    Register rdi changed: 1
    Register rip changed: (void (*)()) 0x5555555551ad <main+48>
    (gdb) maintenance print record-instruction -5
    Register rsp changed: (void *) 0x7fffffffdbe0
    8 bytes of memory at address 0x00007fffffffdbd8 changed from: 00 00 00 00 00 00 00 00
[2] Register rip changed: (void (*)()) 0x5555555551af <main+50>
    (gdb) maintenance print record-instruction -4
    Register rsp changed: (void *) 0x7fffffffdbd8
    8 bytes of memory at address 0x00007fffffffdbd0 changed from: 00 00 00 00 00 00 00 00
    Register rip changed: (void (*)()) 0x55555555513f <function1>
    (gdb) maintenance print record-instruction -3
    Register rbp changed: (void *) 0x7fffffffdc00
    Register rip changed: (void (*)()) 0x555555555140 <function1+1>
    (gdb) maintenance print record-instruction -2
    Register rsp changed: (void *) 0x7fffffffdbd0
    Register eflags changed: [ IF ]
    Register rip changed: (void (*)()) 0x555555555143 <function1+4>
    (gdb) maintenance print record-instruction -1
    4 bytes of memory at address 0x00007fffffffdbbc changed from: 00 00 00 00
    Register rip changed: (void (*)()) 0x555555555147 <function1+8>
    (gdb) maintenance print record-instruction 0
    4 bytes of memory at address 0x00007fffffffdbb8 changed from: 00 00 00 00
[3] Register rip changed: (void (*)()) 0x55555555514a <function1+11>

With gdbserver:

    (gdb) maintenance print record-instruction -5
    Register rdx changed: 140737488346600
[1] Register rip changed: (void (*)()) 0x5555555551a5 <main+40>
    (gdb) maintenance print record-instruction -4
    Register rsp changed: (void *) 0x7fffffffdc98
    8 bytes of memory at address 0x00007fffffffdc90 changed from: 00 00 00 00 00 00 00 00
    Register rip changed: (void (*)()) 0x55555555513f <function1>
    (gdb) maintenance print record-instruction -3
    Register rbp changed: (void *) 0x7fffffffdcc0
    Register rip changed: (void (*)()) 0x555555555140 <function1+1>
    (gdb) maintenance print record-instruction -2
    Register rsp changed: (void *) 0x7fffffffdc90
    Register eflags changed: [ PF IF ]
    Register rip changed: (void (*)()) 0x555555555143 <function1+4>
    (gdb) maintenance print record-instruction -1
    4 bytes of memory at address 0x00007fffffffdc7c changed from: 00 00 00 00
    Register rip changed: (void (*)()) 0x555555555147 <function1+8>
    (gdb) maintenance print record-instruction 0
    4 bytes of memory at address 0x00007fffffffdc78 changed from: 00 00 00 00
[3] Register rip changed: (void (*)()) 0x55555555514a <function1+11>

The important points are:

[1] Beginning of function call line
[2] Call instruction
[3] Starting point

We see that there are fewer recorded instructions with gdbserver.
Seeing this, I thought of the range-stepping feature of the remote
protocol.  This allows GDB to tell GDBserver to step a thread as long as
it's within a certain range of PC.  This is useful when stepping over a
source line, to avoid the back and forth of GDB asking GDBserver to step
instructions repeatedly.

If you enable "set debug remote 1" while doing the "step" command, when
connected to GDBserver, you'll see this resumption packet:


  [remote] Sending packet: $vCont;r5555555551a5,5555555551b4:p3b326.3b326;c:p3b326.-1#67

This tells GDBserver to resume the thread 3b326 (thread id in hex),
single-stepping it as long as within those PCs.  Because of that, GDB
never sees the individual steps in that region:

    83        function1 (a, b);   // CALL VIA LEP
       0x00005555555551a5 <+40>:    mov    -0x10(%rbp),%edx
       0x00005555555551a8 <+43>:    mov    -0xc(%rbp),%eax
       0x00005555555551ab <+46>:    mov    %edx,%esi
       0x00005555555551ad <+48>:    mov    %eax,%edi
       0x00005555555551af <+50>:    call   0x55555555513f <function1>

And therefore, GDB's record-full target doesn't record the individual
steps.  The native linux target doesn't use range stepping (it reports
stops for all individual steps).

To confirm the theory, I added a:

   gdb_test "set range-stepping off"

somewhere near the beginning of your test, and it makes the test pass
with native-gdbserver.

If we want recording to work the same with process targets using
range-stepping and those that don't, maybe GDB should avoid using
range-stepping when the record-full target is in effect?

Simon
  

Simon:

On Mon, 2023-03-27 at 21:19 -0400, Simon Marchi wrote:
> To confirm the theory, I added a:
> 
>    gdb_test "set range-stepping off"
> 
> somewhere near the beginning of your test, and it makes the test pass
> with native-gdbserver.
> 
> If we want recording to work the same with process targets using
> range-stepping and those that don't, maybe GDB should avoid using
> range-stepping when the record-full target is in effect?

We could just update the test to include gdb_test "set range-stepping
off" with a brief explanation of the issue. This would have a minimal
impact on performance of this test and no performance degradation for
any other tests.  It sounds like this would fix the issue for just this
test. 

It sounds like disabling range-stepping when record-full is enabled
would be the more general fix for the issue.  Not sure if there are
other tests where this issue occurs.  Doing the more general fix would
probably have some performance impact on the other tests that need to
use record-full. I can't really say how much of an impact it would be.

Thanks for the insight on how gdbserver works.  At this point, I have
not looked into that part of gdb.  

                            Carl
  

On 3/28/23 11:17, Carl Love wrote:
> Simon:
> 
> On Mon, 2023-03-27 at 21:19 -0400, Simon Marchi wrote:
>> To confirm the theory, I added a:
>>
>>    gdb_test "set range-stepping off"
>>
>> somewhere near the beginning of your test, and it makes the test pass
>> with native-gdbserver.
>>
>> If we want recording to work the same with process targets using
>> range-stepping and those that don't, maybe GDB should avoid using
>> range-stepping when the record-full target is in effect?
> 
> We could just update the test to include gdb_test "set range-stepping
> off" with a brief explanation of the issue. This would have a minimal
> impact on performance of this test and no performance degradation for
> any other tests.  It sounds like this would fix the issue for just this
> test. 
> 
> It sounds like disabling range-stepping when record-full is enabled
> would be the more general fix for the issue.  Not sure if there are
> other tests where this issue occurs.  Doing the more general fix would
> probably have some performance impact on the other tests that need to
> use record-full. I can't really say how much of an impact it would be.

It depends on what the goal is.  In general, we try to minimize the
differences in behavior when debugging native vs debugging remote.  So,
if we say that it's a bug that the record-full target doesn't see all
the intermediary steps, then putting "set range-stepping off" in that
test would just be papering over the bug.  I think the correct thing to
do would be to fix GDB.  And yes there will be a performance impact when
using remote debugging + record-full, but if that's what's needed to get
correct behavior...

Some ideas to implement this:

 - Add a target method supports_range_stepping, record-full would
   implement it and return false.  The default would be true.
 - record-full's resume method could clean the resumed thread's
   may_range_step flag.

I'm open to other ideas.  Note that we don't want to disable range
stepping for other record implementations (only btrace, currently), I
don't think that one is affected by the problem (the hardware should
record all intermediary steps in any case).

Simon