[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
  

On 28/03/2023 17:38, Simon Marchi wrote:
> 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
>
Sorry for possibly necromancing this thread, but I decided to look into 
gdb.reverse failures when testing with clang and the same issue occurs 
as with gdbserver. I decided to take a look and re-read old messages and 
what Pedro actually said in that thread is a bit confusing, because 
there were multiple intertwined issues being discussed. Looking at this 
email 
(https://sourceware.org/pipermail/gdb-patches/2023-January/196130.html) 
he does mention that GDB should not stop in the same line when a 
reverse-step or reverse-next is used. Because of that, I believe that 
the behavior that the test expects is actually incorrect, and we should 
try and fix this.

Looking at the state of the program when it is compiled with GCC we get:

➜  gdb ./gdb -q 
testsuite/outputs/gdb.reverse/finish-reverse-next/finish-reverse-next 
-ex start -ex record
Reading symbols from 
testsuite/outputs/gdb.reverse/finish-reverse-next/finish-reverse-next...
Temporary breakpoint 1 at 0x401176: file 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.revers3/finish-reverse-next.c, 
line 76.
Starting program: 
/home/blarsen/Documents/fsf_build/gdb/testsuite/outputs/gdb.reverse/finish-reverse-next/finish-reverse-next
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffde78)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:76
76        int (*funp) (int, int) = &function1;
(gdb) until 83
main (argc=1, argv=0x7fffffffde78)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
83        function1 (a, b);   // CALL VIA LEP
(gdb) n
86        a = 10;
(gdb) rs
function1 (a=1, b=5)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:70
70      }
(gdb) reverse-finish
Run back to call of #0  function1 (a=1, b=5)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:70
0x0000000000401196 in main (argc=1, argv=0x7fffffffde78)
     at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
83        function1 (a, b);   // CALL VIA LEP
(gdb) maint info line-table
             (snip)
15     81     0x0000000000401185 0x0000000000401185 Y
16     83     0x000000000040118c 0x000000000040118c Y
17     86     0x000000000040119b 0x000000000040119b Y
(gdb) disas /s
Dump of assembler code for function main:
       (snip)
83        function1 (a, b);   // CALL VIA LEP
    0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
    0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
    0x0000000000401192 <+43>:    mov    %edx,%esi
    0x0000000000401194 <+45>:    mov    %eax,%edi
=> 0x0000000000401196 <+47>:    call   0x40112c <function1>

We can see that we have stopped at the right instruction, but it isn't 
mapped to any line number directly. If we reverse-next from there:

82
83        function1 (a, b);   // CALL VIA LEP
=> 0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
    0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
    0x0000000000401192 <+43>:    mov    %edx,%esi
    0x0000000000401194 <+45>:    mov    %eax,%edi
    0x0000000000401196 <+47>:    call   0x40112c <function1>

We at at an address that _is_ mapped in the line table. So my guess is 
that the code setting up the reverse-next or reverse-step is failing to 
figure out our current line (83) so cant properly setup a stepping 
range, GDB would reverse step a single instruction, but since that 
leaves us in a place that is not marked IS_STMT, GDB will continue to 
step until we hit an IS_STMT location, and we end up stopping at the 
same line twice.

For completeness sake, here's what the Clang session looks like:
(gdb) reverse-finish
Run back to call of #0  0x000000000040117b in function1 (a=1, b=5) at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:69
0x00000000004011c8 in main (argc=1, argv=0x7fffffffde78) at 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/finish-reverse-next.c:83
83        function1 (a, b);   // CALL VIA LEP
(gdb) maint info line-table
          (snip)
22     80     0x00000000004011b4 0x00000000004011b4 Y
23     81     0x00000000004011bb 0x00000000004011bb Y
24     83     0x00000000004011c2 0x00000000004011c2 Y
25     83     0x00000000004011c5 0x00000000004011c5
26     83     0x00000000004011c8 0x00000000004011c8
27     86     0x00000000004011cd 0x00000000004011cd Y
(gdb) disas /s
          (snip)
81        b = 5;
    0x00000000004011bb <+43>:    movl   $0x5,-0x18(%rbp)

82
83        function1 (a, b);   // CALL VIA LEP
    0x00000000004011c2 <+50>:    mov    -0x14(%rbp),%edi
    0x00000000004011c5 <+53>:    mov    -0x18(%rbp),%esi
=> 0x00000000004011c8 <+56>:    call   0x401140 <function1>
(gdb) rn
81        b = 5;

Basically, the best way to fix this solution is to get reverse-next and 
reverse-step to properly figure out the stepping range and not have a 
reverse-next that ends up in the same line
  

Bruno:

On Thu, 2023-07-20 at 14:01 +0200, Bruno Larsen wrote:
> (gdb) disas /s
>           (snip)
> 81        b = 5;
>     0x00000000004011bb <+43>:    movl   $0x5,-0x18(%rbp)
> 
> 82
> 83        function1 (a, b);   // CALL VIA LEP
>     0x00000000004011c2 <+50>:    mov    -0x14(%rbp),%edi
>     0x00000000004011c5 <+53>:    mov    -0x18(%rbp),%esi
> => 0x00000000004011c8 <+56>:    call   0x401140 <function1>
> (gdb) rn
> 81        b = 5;
> 
> Basically, the best way to fix this solution is to get reverse-next
> and 
> reverse-step to properly figure out the stepping range and not have
> a 
> reverse-next that ends up in the same line

---------------------

Per my reply to Simon in "Re: [EXTERNAL] Re: [PATCH 2/2 v5] Fix reverse
stepping multiple contiguous PC ranges over the line table." on
6/23/2023. Where he was wondering why control.step_range_start was not
set to the "real" range....


This is the code in the Patch:
> 
> > +{
> > +  /* The line table may have multiple entries for the same source
> > code line.
> > +     Given the PC, check the line table and return the PC that
> > corresponds
> > +     to the line table entry for the source line that PC is
> > in.  */
> > +  CORE_ADDR start_line_pc = ecs->event_thread-
> > >control.step_range_start;
> > +  gdb::optional<CORE_ADDR> real_range_start;
> > +
> > +  /* Call find_line_range_start to get the smallest address in the
> > +     linetable for multiple Line X entries in the line table.  */
> > +  real_range_start = find_line_range_start (pc);
> > +
> > +  if (real_range_start.has_value ())
> > +    start_line_pc = *real_range_start;
> > +
> > +  return start_line_pc;


Simon's comment about the code:

> 
> When I read this, I wonder: why was control.step_range_start not set
> to
> the "real" range start in the first place (not only in the context of
> reverse execution, every time it is set)?  It would seem more robust
> than patching it afterwards in some very specific spots.
> 
> I could see some benefits for range-stepping uses cases too (relevant
> when debugging remotely).  Using your example here:
> 
>    Line X - [0x0 - 0x8]
>    Line X - [0x8 - 0x10]
>    Line X - [0x10 - 0x18]
> 
> Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
> conditional jump to 0x5.  It seems like current GDB would send a
> "range
> step" request to GDBserver, to step in the [0x10, 0x18[ range.  When
> reaching 0x5, execution would stop, and GDB would resume it again
> with
> the [0x0,0x8[ range.  When reaching 0x8, it would stop again, GDB
> would
> resume it with [0x8,0x10[, and so on.  If GDB could send a "range
> step"
> request with the [0x0,0x18[ range, it would avoid those unnecessary
> intermediary stop.
> 

My reply to Simon's comment:

We looked at trying to set control.step_range_start to the real start
range initially.  Ulrich brought this point up in our internal review
of the patch.  

So, when I am in function finish_backward() in infcmd.c I have no way
to determine what the previous PC was.  If I assume it was the previous
value, i.e. pc - 4byes (on PowerPC).  I get a gdb internal error.  It
seems that I am not allowed to change the line range to something that
does not include the current pc value.  

   ../../binutils-gdb-reverse-multiple-contiguous/gdb/infrun.c:2740:
   internal-error: resume_1:
   Assertion `pc_in_thread_step_range (pc, tp)' failed.

In order to make that work, we concluded that it would probably entail
a much bigger change to how reverse execution works which would be
beyond the scope of what this patch is trying to fix.  So, being able
to do what I believe you want to do is in theory possible but it would
require a larger, independent change to what this patch is trying to
fix.

---------------------------------------

I think what Bruno is again asking is to have control.step_range_start
set to the real start range initially, i.e. what Simon asked about.  I
think to do that, we would need to make significant changes to how
reverse execution works to allow us to make that change.  It didn't
appear to be a straight forward fix to me.  I may be wrong.  Maybe
someone sees a good way to make that work that I am missing.  So it
looks like this patch fixes most issues but not all of the problems
with reverse-step and reverse-next.  It might be good to try and fix
this additional scenario in a separate patch, not sure???  

                 Carl
  

On 20/07/2023 16:45, Carl Love wrote:
> Bruno:
>
> On Thu, 2023-07-20 at 14:01 +0200, Bruno Larsen wrote:
>> (gdb) disas /s
>>            (snip)
>> 81        b = 5;
>>      0x00000000004011bb <+43>:    movl   $0x5,-0x18(%rbp)
>>
>> 82
>> 83        function1 (a, b);   // CALL VIA LEP
>>      0x00000000004011c2 <+50>:    mov    -0x14(%rbp),%edi
>>      0x00000000004011c5 <+53>:    mov    -0x18(%rbp),%esi
>> => 0x00000000004011c8 <+56>:    call   0x401140 <function1>
>> (gdb) rn
>> 81        b = 5;
>>
>> Basically, the best way to fix this solution is to get reverse-next
>> and
>> reverse-step to properly figure out the stepping range and not have
>> a
>> reverse-next that ends up in the same line
> ---------------------
>
> Per my reply to Simon in "Re: [EXTERNAL] Re: [PATCH 2/2 v5] Fix reverse
> stepping multiple contiguous PC ranges over the line table." on
> 6/23/2023. Where he was wondering why control.step_range_start was not
> set to the "real" range....
>
>
> This is the code in the Patch:
>>> +{
>>> +  /* The line table may have multiple entries for the same source
>>> code line.
>>> +     Given the PC, check the line table and return the PC that
>>> corresponds
>>> +     to the line table entry for the source line that PC is
>>> in.  */
>>> +  CORE_ADDR start_line_pc = ecs->event_thread-
>>>> control.step_range_start;
>>> +  gdb::optional<CORE_ADDR> real_range_start;
>>> +
>>> +  /* Call find_line_range_start to get the smallest address in the
>>> +     linetable for multiple Line X entries in the line table.  */
>>> +  real_range_start = find_line_range_start (pc);
>>> +
>>> +  if (real_range_start.has_value ())
>>> +    start_line_pc = *real_range_start;
>>> +
>>> +  return start_line_pc;
>
> Simon's comment about the code:
>
>> When I read this, I wonder: why was control.step_range_start not set
>> to
>> the "real" range start in the first place (not only in the context of
>> reverse execution, every time it is set)?  It would seem more robust
>> than patching it afterwards in some very specific spots.
>>
>> I could see some benefits for range-stepping uses cases too (relevant
>> when debugging remotely).  Using your example here:
>>
>>     Line X - [0x0 - 0x8]
>>     Line X - [0x8 - 0x10]
>>     Line X - [0x10 - 0x18]
>>
>> Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
>> conditional jump to 0x5.  It seems like current GDB would send a
>> "range
>> step" request to GDBserver, to step in the [0x10, 0x18[ range.  When
>> reaching 0x5, execution would stop, and GDB would resume it again
>> with
>> the [0x0,0x8[ range.  When reaching 0x8, it would stop again, GDB
>> would
>> resume it with [0x8,0x10[, and so on.  If GDB could send a "range
>> step"
>> request with the [0x0,0x18[ range, it would avoid those unnecessary
>> intermediary stop.
>>
> My reply to Simon's comment:
>
> We looked at trying to set control.step_range_start to the real start
> range initially.  Ulrich brought this point up in our internal review
> of the patch.
>
> So, when I am in function finish_backward() in infcmd.c I have no way
> to determine what the previous PC was.  If I assume it was the previous
> value, i.e. pc - 4byes (on PowerPC).  I get a gdb internal error.  It
> seems that I am not allowed to change the line range to something that
> does not include the current pc value.
>
>     ../../binutils-gdb-reverse-multiple-contiguous/gdb/infrun.c:2740:
>     internal-error: resume_1:
>     Assertion `pc_in_thread_step_range (pc, tp)' failed.
>
> In order to make that work, we concluded that it would probably entail
> a much bigger change to how reverse execution works which would be
> beyond the scope of what this patch is trying to fix.  So, being able
> to do what I believe you want to do is in theory possible but it would
> require a larger, independent change to what this patch is trying to
> fix.
>
> ---------------------------------------
>
> I think what Bruno is again asking is to have control.step_range_start
> set to the real start range initially, i.e. what Simon asked about.  I
> think to do that, we would need to make significant changes to how
> reverse execution works to allow us to make that change.  It didn't
> appear to be a straight forward fix to me.  I may be wrong.  Maybe
> someone sees a good way to make that work that I am missing.  So it
> looks like this patch fixes most issues but not all of the problems
> with reverse-step and reverse-next.  It might be good to try and fix
> this additional scenario in a separate patch, not sure???

I just tested with the newest version of the patch relating to reverse 
stepping over contiguous lines, but that didn't make a difference. GCC 
continued broken. I think the problem is that GCC doesn't have 
contiguous ranges, it has a single range that doesn't contain the PC

My (probably naive) thinking is to set step_range_end to the starting PC 
of the following line, and step_range_start to be the real start of the 
current line at the moment the command is parsed. This sounds like it 
would solve the GCC problem, but I assume I'm missing something. 
Unfortunately, I don't have the time to test this theory myself, but 
I'll happily test any patches you submit :). If you want to repeat my 
test, I want the behavior of gdb.reverse/finish-reverse-next from gcc to 
match clang's (on x86 machines), that is, if you reverse finish from 
function1, a single "rn" command would land you on the b=5 line.
  

Bruno:

On Fri, 2023-07-21 at 09:24 +0200, Bruno Larsen wrote:
> > I think what Bruno is again asking is to have
> > control.step_range_start
> > set to the real start range initially, i.e. what Simon asked
> > about.  I
> > think to do that, we would need to make significant changes to how
> > reverse execution works to allow us to make that change.  It didn't
> > appear to be a straight forward fix to me.  I may be wrong.  Maybe
> > someone sees a good way to make that work that I am missing.  So it
> > looks like this patch fixes most issues but not all of the problems
> > with reverse-step and reverse-next.  It might be good to try and
> > fix
> > this additional scenario in a separate patch, not sure???
> 
> I just tested with the newest version of the patch relating to
> reverse 
> stepping over contiguous lines, but that didn't make a difference.
> GCC 
> continued broken. I think the problem is that GCC doesn't have 
> contiguous ranges, it has a single range that doesn't contain the PC
> 
> My (probably naive) thinking is to set step_range_end to the starting
> PC 
> of the following line, and step_range_start to be the real start of
> the 
> current line at the moment the command is parsed. This sounds like
> it 
> would solve the GCC problem, but I assume I'm missing something. 
> Unfortunately, I don't have the time to test this theory myself, but 
> I'll happily test any patches you submit :). If you want to repeat
> my 
> test, I want the behavior of gdb.reverse/finish-reverse-next from gcc
> to 
> match clang's (on x86 machines), that is, if you reverse finish from 
> function1, a single "rn" command would land you on the b=5 line.

So, I took the example of the error from your previous patch where you
demonstrated the scenario where you have stopped at the call to the
function, then do the reverse-next which stops at the beginning of the
same line and put it into a new test case for discussion and for
debugging the issue.  Nice to have a simple example of the issue.  The
new test is bruno_7_20_2023.exp.  So, in this test, when we are stopped
at the call, as you showed on Intel

(gdb) maint info line-table
>              (snip)
> 15     81     0x0000000000401185 0x0000000000401185 Y
> 16     83     0x000000000040118c 0x000000000040118c Y
> 17     86     0x000000000040119b 0x000000000040119b Y
> (gdb) disas /s
> Dump of assembler code for function main:
>        (snip)
> 83        function1 (a, b);   // CALL VIA LEP
>     0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
>     0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
>     0x0000000000401192 <+43>:    mov    %edx,%esi
>     0x0000000000401194 <+45>:    mov    %eax,%edi
> => 0x0000000000401196 <+47>:    call   0x40112c <function1>
> 
> We can see that we have stopped at the right instruction, but it
> isn't 
> mapped to any line number directly. If we reverse-next from there:

the tp->control.step_range_start is set to 0x40118c in function
prepare_one_step() in gdb/infcmd.c.  The PC in your case is at
0x401196.  Of course both of the addresses are in the same line.  We
are executing backwards so we need step_range_start to be set to a
value in the previous line.  In general, we have no idea what the
previous line was as we may have arrived on this line via a jump or
branch in theory.  Also, note, when setting the range we need to have
the PC in the range or GDB will give us an assert error.

So really the issue is that we want the step_range_start to be in the
line where we came from.  So, if we see the line number for PC and
step_range_start are the same, we can set step_range_start to one less
then the address of the beginning of the current line when we parse the
instruction and setup the stepping range.  The goal is to have GDB stop
in the previous line and not at the beginning of the line and thus
match the behavior of gdb on clang.

I implemented the check and adjustment for step_range_start as just
described and tested it with your example, bruno_7_20_2023.exp and it
does seem to work as desired.

The existing testcases expect to have to do two reverse step/next
instructions to reach the previous line.  So, we need to fix the
existing tests gdb.reverse/finish-reverse-next.exp and gdb.mi/mi-
reverse.exp to remove the extra reverse step/next command.

Anyway, I put the gdb code fix, the bruno_7_20_2023.exp and test fixes
into the patch below for discussion purposes.  If everyone is happy
with the gdb change and test changes (we can drop the Bruno test) I can
merge these changes into the patch. Hopefully keeping these changes in
a separate patch for the moment will make it easier to see what is
being changed.

If you were to apply the following patch on top of the currently
proposed patch, does gdb then execute as you expect on your system?  I
have tested the change on IBM Power 10.  It would be good to make sure
it works on your X86 system as well.  I also ran the full gdb
regression tests on Power 10 with no additional regression failures.
Thanks for looking at the patch, proposed fix and the additional
testing.

                          Carl 
---------------------------------------------------------------
[PATCH 2/2] finish-reverse-next new test case from bruno.

GDB fix for Bruno test, updated other tests for the gdb fix.
---
 gdb/infcmd.c                                  |  9 ++
 gdb/testsuite/gdb.mi/mi-reverse.exp           |  6 +-
 gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp | 85 +++++++++++++++++++
 .../gdb.reverse/finish-reverse-next.exp       | 44 +++++++---
 4 files changed, 133 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b8134665f3f..c09b3edad3d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -981,6 +981,15 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
 	  find_pc_line_pc_range (pc,
 				 &tp->control.step_range_start,
 				 &tp->control.step_range_end);
+	  symtab_and_line sal = find_pc_line (pc, 0);
+	  symtab_and_line sal_start
+	    = find_pc_line (tp->control.step_range_start, 0);
+
+	  if (sal.line == sal_start.line)
+	    /* The step_range_start address is in the same line.  We want to
+	       stop in the previous line so move step_range_start one
+	       instruction earlier.  */
+	    tp->control.step_range_start--;
 
 	  /* There's a problem in gcc (PR gcc/98780) that causes missing line
 	     table entries, which results in a too large stepping range.
diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
index baa53a495d7..997309cfb71 100644
--- a/gdb/testsuite/gdb.mi/mi-reverse.exp
+++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
@@ -99,8 +99,12 @@ proc test_controlled_execution_reverse {} {
     #   It takes two steps to get back to the previous line,
     #   as the first step moves us to the start of the current line,
     #   and the one after that moves back to the previous line.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
 
-    mi_execute_to "exec-next --reverse 2" \
+    mi_execute_to "exec-next --reverse" \
  	"end-stepping-range" "main" "" \
  	"basics.c" $line_main_hello "" \
  	"reverse next to get over the call to do_nothing"
diff --git a/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp b/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp
new file mode 100644
index 00000000000..0f19a959043
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp
@@ -0,0 +1,85 @@
+# 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 finish-reverse-next.c
+
+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"
+}
+
+
+# until 83
+# 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.*"
+
+gdb_test "next" ".*a = 10;.*" \
+    "next from function body function1 LEP to a = 10;"
+
+gdb_test "reverse-step" ".*70.*}.*" \
+    "reverse step from a = 10, stops at last inst in function1 call"
+
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish should stop at last inst in line for function1"
+
+gdb_test "reverse-next" ".*b = 5;.*" \
+    "reverse-finish should stop at b = 5, not first inst in line for function1"
+
+
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
index 1f53b649a7d..921d0051233 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -80,10 +80,15 @@ repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100"
 # 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.
+
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same 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" ".*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"
 
 
@@ -113,10 +118,14 @@ gdb_test "step" ".*int ret = 0;.*" "step test 1"
 # 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.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same 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" ".*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"
 
@@ -148,10 +157,15 @@ repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
 # 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.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same 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" ".*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"
@@ -184,10 +198,15 @@ gdb_test "stepi" "{" "stepi to between GEP and LEP"
 # 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.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same 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" ".*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"
 
@@ -216,9 +235,14 @@ gdb_test "step" ".*int ret = 0;.*" "step test 2"
 # 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.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same 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" ".*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"
  

On 01/08/2023 00:59, Carl Love wrote:
> Bruno:
>
> On Fri, 2023-07-21 at 09:24 +0200, Bruno Larsen wrote:
>>> I think what Bruno is again asking is to have
>>> control.step_range_start
>>> set to the real start range initially, i.e. what Simon asked
>>> about.  I
>>> think to do that, we would need to make significant changes to how
>>> reverse execution works to allow us to make that change.  It didn't
>>> appear to be a straight forward fix to me.  I may be wrong.  Maybe
>>> someone sees a good way to make that work that I am missing.  So it
>>> looks like this patch fixes most issues but not all of the problems
>>> with reverse-step and reverse-next.  It might be good to try and
>>> fix
>>> this additional scenario in a separate patch, not sure???
>> I just tested with the newest version of the patch relating to
>> reverse
>> stepping over contiguous lines, but that didn't make a difference.
>> GCC
>> continued broken. I think the problem is that GCC doesn't have
>> contiguous ranges, it has a single range that doesn't contain the PC
>>
>> My (probably naive) thinking is to set step_range_end to the starting
>> PC
>> of the following line, and step_range_start to be the real start of
>> the
>> current line at the moment the command is parsed. This sounds like
>> it
>> would solve the GCC problem, but I assume I'm missing something.
>> Unfortunately, I don't have the time to test this theory myself, but
>> I'll happily test any patches you submit :). If you want to repeat
>> my
>> test, I want the behavior of gdb.reverse/finish-reverse-next from gcc
>> to
>> match clang's (on x86 machines), that is, if you reverse finish from
>> function1, a single "rn" command would land you on the b=5 line.
> So, I took the example of the error from your previous patch where you
> demonstrated the scenario where you have stopped at the call to the
> function, then do the reverse-next which stops at the beginning of the
> same line and put it into a new test case for discussion and for
> debugging the issue.  Nice to have a simple example of the issue.  The
> new test is bruno_7_20_2023.exp.  So, in this test, when we are stopped
> at the call, as you showed on Intel
>
> (gdb) maint info line-table
>>               (snip)
>> 15     81     0x0000000000401185 0x0000000000401185 Y
>> 16     83     0x000000000040118c 0x000000000040118c Y
>> 17     86     0x000000000040119b 0x000000000040119b Y
>> (gdb) disas /s
>> Dump of assembler code for function main:
>>         (snip)
>> 83        function1 (a, b);   // CALL VIA LEP
>>      0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
>>      0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
>>      0x0000000000401192 <+43>:    mov    %edx,%esi
>>      0x0000000000401194 <+45>:    mov    %eax,%edi
>> => 0x0000000000401196 <+47>:    call   0x40112c <function1>
>>
>> We can see that we have stopped at the right instruction, but it
>> isn't
>> mapped to any line number directly. If we reverse-next from there:
> the tp->control.step_range_start is set to 0x40118c in function
> prepare_one_step() in gdb/infcmd.c.  The PC in your case is at
> 0x401196.  Of course both of the addresses are in the same line.  We
> are executing backwards so we need step_range_start to be set to a
> value in the previous line.  In general, we have no idea what the
> previous line was as we may have arrived on this line via a jump or
> branch in theory.  Also, note, when setting the range we need to have
> the PC in the range or GDB will give us an assert error.
>
> So really the issue is that we want the step_range_start to be in the
> line where we came from.  So, if we see the line number for PC and
> step_range_start are the same, we can set step_range_start to one less
> then the address of the beginning of the current line when we parse the
> instruction and setup the stepping range.  The goal is to have GDB stop
> in the previous line and not at the beginning of the line and thus
> match the behavior of gdb on clang.
>
> I implemented the check and adjustment for step_range_start as just
> described and tested it with your example, bruno_7_20_2023.exp and it
> does seem to work as desired.
>
> The existing testcases expect to have to do two reverse step/next
> instructions to reach the previous line.  So, we need to fix the
> existing tests gdb.reverse/finish-reverse-next.exp and gdb.mi/mi-
> reverse.exp to remove the extra reverse step/next command.
>
> Anyway, I put the gdb code fix, the bruno_7_20_2023.exp and test fixes
> into the patch below for discussion purposes.  If everyone is happy
> with the gdb change and test changes (we can drop the Bruno test) I can
> merge these changes into the patch. Hopefully keeping these changes in
> a separate patch for the moment will make it easier to see what is
> being changed.

Hi Carl!

Thanks for the thorough explanation and test. You did understood what I 
meant perfectly :)

This is a little besides the point, but I don't see any other patches 
that touch this area and haven't been committed yet. Am I missing 
something obvious?

Regardless, I feel like this one change can propagate enough that it 
should be its own commit anyway.

>
> If you were to apply the following patch on top of the currently
> proposed patch, does gdb then execute as you expect on your system?  I
> have tested the change on IBM Power 10.  It would be good to make sure
> it works on your X86 system as well.  I also ran the full gdb
> regression tests on Power 10 with no additional regression failures.
> Thanks for looking at the patch, proposed fix and the additional
> testing.

The patch didn't apply on current master, but I added the changes 
manually and tested it. It does fix the issue, and adds no regressions.

I feel like it could be worth sending an official version this patch as 
a new thread, instead of a reply to this one, to get people who might 
not be interested in reverse debugging but may have thoughts on this 
patch, but maybe wait a bit to see if someone in here says anything :)
  

Bruno:


On Wed, 2023-08-02 at 11:29 +0200, Bruno Larsen wrote:
> Hi Carl!
> 
> Thanks for the thorough explanation and test. You did understood what
> I 
> meant perfectly :)
> 
> This is a little besides the point, but I don't see any other
> patches 
> that touch this area and haven't been committed yet. Am I missing 
> something obvious?

The patch that I was talking about is patch 2/2 for the reverse
execution that I posted a few weeks ago.  Patch 1 was for checking the
gcc version.  But yea, the discussion thread is getting long enough
that some of this is getting lost.  
> 
> Regardless, I feel like this one change can propagate enough that it 
> should be its own commit anyway.
> 
> > If you were to apply the following patch on top of the currently
> > proposed patch, does gdb then execute as you expect on your
> > system?  I
> > have tested the change on IBM Power 10.  It would be good to make
> > sure
> > it works on your X86 system as well.  I also ran the full gdb
> > regression tests on Power 10 with no additional regression
> > failures.
> > Thanks for looking at the patch, proposed fix and the additional
> > testing.
> 
> The patch didn't apply on current master, but I added the changes 
> manually and tested it. It does fix the issue, and adds no
> regressions.

I think this little fix needs to be added to patch 2/2 for fixing the
reverse execution as this fix and the patch are trying to fix the same
things.  I was also thinking this fix needs to be inside a if (reverse-
execution) statement as it is really for the reverse execution.
> 
> I feel like it could be worth sending an official version this patch
> as 
> a new thread, instead of a reply to this one, to get people who
> might 
> not be interested in reverse debugging but may have thoughts on this 
> patch, but maybe wait a bit to see if someone in here says anything
> :)

I have been thinking it is time to start a fresh thread for the patches
as I think the length of this thread is losing people.  Lets see if
anyone else responds to this thread in a week or so.  I will add the if
(reverse execution) to the little fix as mentioned, update the tests,
refresh the patch and then send out a new version in a new thread.

Thanks for the help.

                            Carl