[1/2,version,3] fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp

  Bruno, Tom:

Per the conversation about the regressions for the regressions in tests

FAIL: gdb.btrace/rn-dl-bind.exp: test: reverse-next
FAIL: gdb.btrace/tailcall.exp: reverse-next.1
FAIL: gdb.btrace/tailcall.exp: step.1

The following patch fixes the expected results for the tailcall test on
X86.  Per the previous messages, the output needs to be updated since
the behavior of the reverse-next command was fixed.

As noted, I have not been able to reproduce the gdb.btrace/rn-dl-
bind.exp test failure.

The patch has been tested on my personal X86 laptop which has the
needed processor version and on the PowerPC machine with no regressions
found.

                           Carl 

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

X86: reverse-finish fix

PR record/29927  - reverse-finish requires two reverse next instructions to
reach previous source line

Currently on X86, when executing the finish command in reverse, gdb does a
single step from the first instruction in the callee to get back to the
caller.  GDB stops on the last instruction in the source code line where
the call was made.  When stopped at the last instruction of the source code
line, a reverse next or step command will stop at the first instruction
of the same source code line thus requiring two step/next commands to
reach the previous source code line.  It should only require one step/next
command to reach the previous source code line.

By contrast, a reverse next or step command from the first line in a
function stops at the first instruction in the source code line where the
call was made.

This patch fixes the reverse finish command so it will stop at the first
instruction of the source line where the function call was made.  The
behavior on X86 for the reverse-finish command now matches doing a
reverse-next from the beginning of the function.

The proceed_to_finish flag in struct thread_control_state is no longer
used.  This patch removes the declaration, initialization and setting of
the flag.

This patch requires a number of regression tests to be updated.  Test
gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
tests gdb.reverse/amd64-failcall-reverse.exp and
gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
result.  The expected result in btrace/tailcall.exp also changes since
the reverse-next now only steps back thru one function call not two
function calls.

This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
reverse-finish command when returning from the entry point and from the
body of the function.

The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp
was moved to lib/gdb.exp and renamed cmd_until.

The patch has been tested on X86 and PowerPC to verify no additional
regression failures occured.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29927
---
 gdb/gdbthread.h                               |   4 -
 gdb/infcall.c                                 |   3 -
 gdb/infcmd.c                                  |  32 +++---
 gdb/infrun.c                                  |  40 +++----
 gdb/testsuite/gdb.btrace/tailcall.exp         |   4 +-
 gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
 .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
 .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
 .../gdb.reverse/finish-reverse-next.exp       | 104 ++++++++++++++++++
 .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
 .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++-------
 gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
 gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
 gdb/testsuite/lib/gdb.exp                     |  33 ++++++
 14 files changed, 232 insertions(+), 108 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
 create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp
  

On 2023-01-20 12:03 a.m., Carl Love via Gdb-patches wrote:

> X86: reverse-finish fix
> 
> PR record/29927  - reverse-finish requires two reverse next instructions to
> reach previous source line
> 
> Currently on X86, when executing the finish command in reverse, gdb does a
> single step from the first instruction in the callee to get back to the
> caller.  GDB stops on the last instruction in the source code line where
> the call was made.  When stopped at the last instruction of the source code
> line, a reverse next or step command will stop at the first instruction
> of the same source code line thus requiring two step/next commands to
> reach the previous source code line.  It should only require one step/next
> command to reach the previous source code line.
> 
> By contrast, a reverse next or step command from the first line in a
> function stops at the first instruction in the source code line where the
> call was made.

I'd think this was on purpose.  Note that next/step/reverse-{next/step} are line-oriented
stepping commands, they step/next until the previous/next line.  While "finish" is described
as undoing the _function call_.

The manual says:

 reverse-finish
 Just as the finish command takes you to the point where the current function returns,
 reverse-finish takes you to the point where it was called. Instead of ending up at the end of
 the current function invocation, you end up at the beginning.

Say you have a line with multiple statements involving multiple function calls.
The simplest would be:

  func1 ();  func2 ();

Say you'd stopped inside 'func2'.  If you do finish there, in current master gdb
stops at the call to 'func2', and you can then decide to reverse step into 'func1'.
While with your change, reverse-finish in func2 will make gdb stop at the beginning
of the line, before the call to func1.

Thus I'm really not convinced (yet) this change is a good one.  It removes
a user choice.  You can always do reverse-next/step do get what you want
reverse-finish do to, already.

Pedro Alves

> 
> This patch fixes the reverse finish command so it will stop at the first
> instruction of the source line where the function call was made.  The
> behavior on X86 for the reverse-finish command now matches doing a
> reverse-next from the beginning of the function.
> 
> The proceed_to_finish flag in struct thread_control_state is no longer
> used.  This patch removes the declaration, initialization and setting of
> the flag.
> 
> This patch requires a number of regression tests to be updated.  Test
> gdb.mi/mi-reverse.exp no longer needs to execute two steps to get to the
> previous line.  The gdb output for tests gdb.reverse/until-precsave.exp
> and gdb.reverse/until-reverse.exp changed slightly.  The expected result in
> tests gdb.reverse/amd64-failcall-reverse.exp and
> gdb.reverse/singlejmp-reverse.exp are updated to the correct expected
> result.  The expected result in btrace/tailcall.exp also changes since
> the reverse-next now only steps back thru one function call not two
> function calls.
> 
> This patch adds a new test gdb.reverse/finish-reverse-next.exp to test the
> reverse-finish command when returning from the entry point and from the
> body of the function.
> 
> The step_until proceedure in test gdb.reverse/step-indirect-call-thunk.exp

Typo: proceedure -> procedure

> was moved to lib/gdb.exp and renamed cmd_until.

"cmd_until" here is stale.

> 
> The patch has been tested on X86 and PowerPC to verify no additional
> regression failures occured.

occured -> occurred

> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29927
> ---
>  gdb/gdbthread.h                               |   4 -
>  gdb/infcall.c                                 |   3 -
>  gdb/infcmd.c                                  |  32 +++---
>  gdb/infrun.c                                  |  40 +++----
>  gdb/testsuite/gdb.btrace/tailcall.exp         |   4 +-
>  gdb/testsuite/gdb.mi/mi-reverse.exp           |   9 +-
>  .../gdb.reverse/amd64-tailcall-reverse.exp    |   5 +-
>  .../gdb.reverse/finish-reverse-next.c         |  48 ++++++++
>  .../gdb.reverse/finish-reverse-next.exp       | 104 ++++++++++++++++++
>  .../gdb.reverse/singlejmp-reverse.exp         |   5 +-
>  .../gdb.reverse/step-indirect-call-thunk.exp  |  49 ++-------
>  gdb/testsuite/gdb.reverse/until-precsave.exp  |   2 +-
>  gdb/testsuite/gdb.reverse/until-reverse.exp   |   2 +-
>  gdb/testsuite/lib/gdb.exp                     |  33 ++++++
>  14 files changed, 232 insertions(+), 108 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.c
>  create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> 
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 11d69fceab0..e4edff2d621 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -150,10 +150,6 @@ struct thread_control_state
>       the finished single step.  */
>    int trap_expected = 0;
>  
> -  /* Nonzero if the thread is being proceeded for a "finish" command
> -     or a similar situation when return value should be printed.  */
> -  int proceed_to_finish = 0;
> -
>    /* Nonzero if the thread is being proceeded for an inferior function
>       call.  */
>    int in_infcall = 0;
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index e09904f9a35..116605c43ef 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -625,9 +625,6 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
>  
>    disable_watchpoints_before_interactive_call_start ();
>  
> -  /* We want to print return value, please...  */
> -  call_thread->control.proceed_to_finish = 1;
> -
>    try
>      {
>        /* Infcalls run synchronously, in the foreground.  */
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 0497ad05091..9c42efeae8d 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm *sm)
>  
>    sal = find_pc_line (func_addr, 0);
>  
> -  tp->control.proceed_to_finish = 1;
> -  /* Special case: if we're sitting at the function entry point,
> -     then all we need to do is take a reverse singlestep.  We
> -     don't need to set a breakpoint, and indeed it would do us
> -     no good to do so.
> -
> -     Note that this can only happen at frame #0, since there's
> -     no way that a function up the stack can have a return address
> -     that's equal to its entry point.  */
> +  frame_info_ptr frame = get_selected_frame (nullptr);
>  
>    if (sal.pc != pc)
>      {
> -      frame_info_ptr frame = get_selected_frame (nullptr);
>        struct gdbarch *gdbarch = get_frame_arch (frame);
>  
>        /* Set a step-resume at the function's entry point.  Once that's
> @@ -1743,16 +1734,22 @@ finish_backward (struct finish_command_fsm *sm)
>        sr_sal.pspace = get_frame_program_space (frame);
>        insert_step_resume_breakpoint_at_sal (gdbarch,
>  					    sr_sal, null_frame_id);
> -
> -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>      }
>    else
>      {
> -      /* We're almost there -- we just need to back up by one more
> -	 single-step.  */
> -      tp->control.step_range_start = tp->control.step_range_end = 1;
> -      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> +      /* We are exactly at the function entry point.  Note that this
> +	 can only happen at frame #0.
> +
> +	 When setting a step range, need to call set_step_info
> +	 to setup the current_line/symtab fields as well.  */
> +      set_step_info (tp, frame, find_pc_line (pc, 0));
> +
> +      /* Return using a step range so we will keep stepping back
> +	 to the first instruction in the source code line.  */
> +      tp->control.step_range_start = sal.pc;
> +      tp->control.step_range_end = sal.pc;
>      }
> +  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>  }
>  
>  /* finish_forward -- helper function for finish_command.  FRAME is the
> @@ -1778,9 +1775,6 @@ finish_forward (struct finish_command_fsm *sm, frame_info_ptr frame)
>  
>    set_longjmp_breakpoint (tp, frame_id);
>  
> -  /* We want to print return value, please...  */
> -  tp->control.proceed_to_finish = 1;
> -
>    proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>  }
>  
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 181d961d80d..86e5ef1ed12 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct thread_info *tp)
>  
>    tp->control.stop_step = 0;
>  
> -  tp->control.proceed_to_finish = 0;
> -
>    tp->control.stepping_command = 0;
>  
>    /* Discard any remaining commands or status from previous stop.  */
> @@ -6737,31 +6735,27 @@ process_event_stop_test (struct execution_control_state *ecs)
>  
>      case BPSTAT_WHAT_STEP_RESUME:
>        infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
> -
>        delete_step_resume_breakpoint (ecs->event_thread);
> -      if (ecs->event_thread->control.proceed_to_finish
> -	  && execution_direction == EXEC_REVERSE)
> +      fill_in_stop_func (gdbarch, ecs);
> +
> +      if (execution_direction == EXEC_REVERSE)
>  	{
>  	  struct thread_info *tp = ecs->event_thread;
> +	  /* We are finishing a function in reverse or stepping over a function
> +	     call in reverse, and just hit the step-resume breakpoint at the
> +	     start address of the function, and we're almost there -- just need
> +	     to back up to the function call.  */
>  
> -	  /* We are finishing a function in reverse, and just hit the
> -	     step-resume breakpoint at the start address of the
> -	     function, and we're almost there -- just need to back up
> -	     by one more single-step, which should take us back to the
> -	     function call.  */
> -	  tp->control.step_range_start = tp->control.step_range_end = 1;
> -	  keep_going (ecs);
> -	  return;
> -	}
> -      fill_in_stop_func (gdbarch, ecs);
> -      if (ecs->event_thread->stop_pc () == ecs->stop_func_start
> -	  && execution_direction == EXEC_REVERSE)
> -	{
> -	  /* We are stepping over a function call in reverse, and just
> -	     hit the step-resume breakpoint at the start address of
> -	     the function.  Go back to single-stepping, which should
> -	     take us back to the function call.  */
> -	  ecs->event_thread->stepping_over_breakpoint = 1;
> +	  stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), 0);
> +
> +	  /* When setting a step range, need to call set_step_info
> +	     to setup the current_line/symtab fields as well.  */
> +	  set_step_info (tp, frame, stop_pc_sal);
> +
> +	  /* Return using a step range so we will keep stepping back to the
> +	     first instruction in the source code line.  */
> +	  tp->control.step_range_start = ecs->stop_func_start;
> +	  tp->control.step_range_end = ecs->stop_func_start;
>  	  keep_going (ecs);
>  	  return;
>  	}
> diff --git a/gdb/testsuite/gdb.btrace/tailcall.exp b/gdb/testsuite/gdb.btrace/tailcall.exp
> index 028e03fc6f6..fa254664a09 100644
> --- a/gdb/testsuite/gdb.btrace/tailcall.exp
> +++ b/gdb/testsuite/gdb.btrace/tailcall.exp
> @@ -102,9 +102,9 @@ gdb_test "reverse-step" "\[^\r\n\]*main \\(\\) at tailcall.c:37\r\n.*" \
>      "reverse-step.2"
>  gdb_test "next" "\[^\r\n\]*38.*" \
>      "next.1"
> -gdb_test "reverse-next" "\[^\r\n\]*main \\(\\) at tailcall.c:37\r\n.*" \
> +gdb_test "reverse-next" "\[^\r\n\]*foo \\(\\) at tailcall.c:29\r\n.*" \
>      "reverse-next.1"
> -gdb_test "step" "\[^\r\n\]*foo \\(\\) at tailcall.c:29\r\n.*" \
> +gdb_test "step" "\[^\r\n\]*bar \\(\\) at tailcall.c:24\r\n.*" \
>      "step.1"
>  gdb_test "finish" "\[^\r\n\]*main \\(\\) at tailcall.c:38\r\n.*" \
>      "finish.2"
> diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
> index d631beb17c8..30635ab1754 100644
> --- a/gdb/testsuite/gdb.mi/mi-reverse.exp
> +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
> @@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} {
>  	"basics.c" $line_main_callme_1 "" \
>  	"reverse finish from callme"
>  
> -    # Test exec-reverse-next
> -    #   It takes two steps to get back to the previous line,
> -    #   as the first step moves us to the start of the current line,
> -    #   and the one after that moves back to the previous line.
> -
> -    mi_execute_to "exec-next --reverse 2" \
> +    mi_execute_to "exec-next --reverse" \
>   	"end-stepping-range" "main" "" \
>   	"basics.c" $line_main_hello "" \
> - 	"reverse next to get over the call to do_nothing"
> +	"reverse next to get over the call to do_nothing"
>  
>      # Test exec-reverse-step
>  
> diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> index 52a87faabf7..9964b4f8e4b 100644
> --- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp
> @@ -44,6 +44,5 @@ if [supports_process_record] {
>  gdb_test "next" {f \(\);} "next to f"
>  gdb_test "next" {v = 3;} "next to v = 3"
>  
> -# FAIL was:
> -# 29        g ();
> -gdb_test "reverse-next" {f \(\);}
> +# Reverse step back into f ().  Puts us at call to g () in function f ().
> +gdb_test "reverse-next" {g \(\);}
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> new file mode 100644
> index 00000000000..f90ecbb93cb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c
> @@ -0,0 +1,48 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2012-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
> +*/
> +
> +int
> +function1 (int a, int b)   // FUNCTION1
> +{
> +  int ret = 0;
> +
> +  ret = a + b;
> +  return ret;
> +}
> +
> +int
> +main(int argc, char* argv[])
> +{
> +  int a, b;
> +
> +  a = 1;
> +  b = 5;
> +
> +  function1 (a, b);   // CALL FUNCTION
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> new file mode 100644
> index 00000000000..63305c109e1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -0,0 +1,104 @@
> +# 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
> +
> +if ![supports_reverse] {
> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +runto_main
> +set target_remote [gdb_is_target_remote]
> +
> +if [supports_process_record] {
> +    # Activate process record/replay.
> +    gdb_test_no_output "record" "turn on process record for test1"
> +}
> +
> +
> +### TEST 1: reverse finish from the entry point instruction in
> +### function1.
> +
> +# Set breakpoint at call to function1 in main.
> +set bp_FUNCTION [gdb_get_line_number "CALL FUNCTION" $srcfile]
> +gdb_breakpoint $srcfile:$bp_FUNCTION 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_FUNCTION\r\n.*"
> +
> +# stepi until we see "{" indicating we entered function1
> +repeat_cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call"
> +
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
> +    "reverse-finish function1 "
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line.  It should only take one reverse next command to get to the previous
> +# source line.   If GDB stops at the last instruction in the source code line
> +# it will take two reverse next instructions to get to the previous source
> +# line.
> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call from function"
> +
> +# Clear the recorded log.
> +gdb_test "record stop"  "Process record is stopped.*" \
> +    "turn off process record for test1"
> +gdb_test_no_output "record" "turn on process record for test2"
> +
> +
> +### TEST 2: reverse finish from the body of function1.
> +
> +# Set breakpoint at call to function1 in main.
> +gdb_breakpoint $srcfile:$bp_FUNCTION 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_FUNCTION\r\n.*"
> +
> +# do a step instruction to get to the body of the function
> +gdb_test "step" ".*int ret = 0;.*" "step test 1"
> +
> +gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL FUNCTION.*" \
> +    "reverse-finish function1 call from function body"
> +
> +# Check to make sure we stopped at the first instruction in the source code
> +# line.  It should only take one reverse next command to get to the previous
> +# source line.
> +gdb_test "reverse-next" ".*b = 5;.*" \
> +    "reverse next at b = 5, from function body"
> diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> index 1ca7c2ce559..eb03051625a 100644
> --- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp
> @@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3"
>  # {
>  gdb_test "reverse-step" {nodebug \(\);}
>  
> -# FAIL was:
> -# No more reverse-execution history.
> -# {
> -gdb_test "reverse-next" {f \(\);}
> +gdb_test "reverse-next" {g \(\);}
> diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> index ad637899e5b..b82e5663bd5 100644
> --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> @@ -39,39 +39,6 @@ if { ![runto_main] } {
>      return -1
>  }
>  
> -# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
> -#
> -#  COMMAND is a stepping command
> -#  CURRENT is a string matching the current location
> -#  TARGET  is a string matching the target location
> -#  TEST    is the test name
> -#
> -# The function issues repeated COMMANDs as long as the location matches
> -# CURRENT up to a maximum of 100 steps.
> -#
> -# TEST passes if the resulting location matches TARGET and fails
> -# otherwise.
> -#
> -proc step_until { command current target test } {
> -    global gdb_prompt
> -
> -    set count 0
> -    gdb_test_multiple "$command" "$test" {
> -        -re "$current.*$gdb_prompt $" {
> -            incr count
> -            if { $count < 100 } {
> -                send_gdb "$command\n"
> -                exp_continue
> -            } else {
> -                fail "$test"
> -            }
> -        }
> -        -re "$target.*$gdb_prompt $" {
> -            pass "$test"
> -        }
> -    }
> -}
> -
>  gdb_test_no_output "record"
>  gdb_test "next" ".*" "record trace"
>  
> @@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \
>      "reverse-step through thunks and over inc"
>  
>  # We can use instruction stepping to step into thunks.
> -step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
> -step_until "stepi" "indirect_thunk" "inc" \
> +repeat_cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call thunk"
> +repeat_cmd_until "stepi" "indirect_thunk" "inc" \
>      "stepi out of call thunk into inc"
>  set alphanum_re "\[a-zA-Z0-9\]"
>  set pic_thunk_re  "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re* \\(\\)"
> -step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
> -step_until "stepi" "return_thunk" "apply" \
> +repeat_cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into return thunk"
> +repeat_cmd_until "stepi" "return_thunk" "apply" \
>      "stepi out of return thunk back into apply"
>  
> -step_until "reverse-stepi" "apply" "return_thunk" \
> +repeat_cmd_until "reverse-stepi" "apply" "return_thunk" \
>      "reverse-stepi into return thunk"
> -step_until "reverse-stepi" "return_thunk" "inc" \
> +repeat_cmd_until "reverse-stepi" "return_thunk" "inc" \
>      "reverse-stepi out of return thunk into inc"
> -step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
> +repeat_cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \
>      "reverse-stepi into call thunk"
> -step_until "reverse-stepi" "indirect_thunk" "apply" \
> +repeat_cmd_until "reverse-stepi" "indirect_thunk" "apply" \
>      "reverse-stepi out of call thunk into apply"
> diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp b/gdb/testsuite/gdb.reverse/until-precsave.exp
> index 0c2d7537cd6..777aec94ac1 100644
> --- a/gdb/testsuite/gdb.reverse/until-precsave.exp
> +++ b/gdb/testsuite/gdb.reverse/until-precsave.exp
> @@ -142,7 +142,7 @@ gdb_test "advance marker2" \
>  # Finish out to main scope (backward)
>  
>  gdb_test "finish" \
> -    " in main .*$srcfile:$bp_location20.*" \
> +    "main .*$srcfile:$bp_location20.*" \
>      "reverse-finish from marker2"
>  
>  # Advance backward to last line of factorial (outer invocation)
> diff --git a/gdb/testsuite/gdb.reverse/until-reverse.exp b/gdb/testsuite/gdb.reverse/until-reverse.exp
> index 23fc881dbf2..3a05953329f 100644
> --- a/gdb/testsuite/gdb.reverse/until-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/until-reverse.exp
> @@ -113,7 +113,7 @@ gdb_test "advance marker2" \
>  # Finish out to main scope (backward)
>  
>  gdb_test "finish" \
> -    " in main .*$srcfile:$bp_location20.*" \
> +    "main .*$srcfile:$bp_location20.*" \
>      "reverse-finish from marker2"
>  
>  # Advance backward to last line of factorial (outer invocation)
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index c41d4698d66..234c21a04ea 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9301,6 +9301,39 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
>      }
>  }
>  
> +# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
> +#
> +#  COMMAND is a stepping command
> +#  CURRENT is a string matching the current location
> +#  TARGET  is a string matching the target location
> +#  TEST    is the test name
> +#
> +# The function issues repeated COMMANDs as long as the location matches
> +# CURRENT up to a maximum of 100 steps.
> +#
> +# TEST passes if the resulting location matches TARGET and fails
> +# otherwise.
> +
> +proc repeat_cmd_until { command current target test } {
> +    global gdb_prompt
> +
> +    set count 0
> +    gdb_test_multiple "$command" "$test" {
> +	-re "$current.*$gdb_prompt $" {
> +	    incr count
> +	    if { $count < 100 } {
> +		send_gdb "$command\n"
> +		exp_continue
> +	    } else {
> +		fail "$test"
> +	    }
> +	}
> +	-re "$target.*$gdb_prompt $" {
> +	    pass "$test"
> +	}
> +    }
> +}
> +
>  # Check if the compiler emits epilogue information associated
>  # with the closing brace or with the last statement line.
>  #
>
  

Pedro:

On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
> > Currently on X86, when executing the finish command in reverse, gdb
> > does a
> > single step from the first instruction in the callee to get back to
> > the
> > caller.  GDB stops on the last instruction in the source code line
> > where
> > the call was made.  When stopped at the last instruction of the
> > source code
> > line, a reverse next or step command will stop at the first
> > instruction
> > of the same source code line thus requiring two step/next commands
> > to
> > reach the previous source code line.  It should only require one
> > step/next
> > command to reach the previous source code line.
> > 
> > By contrast, a reverse next or step command from the first line in
> > a
> > function stops at the first instruction in the source code line
> > where the
> > call was made.
> 
> I'd think this was on purpose.  Note that next/step/reverse-
> {next/step} are line-oriented
> stepping commands, they step/next until the previous/next line. 
> While "finish" is described
> as undoing the _function call_.
> 
> The manual says:
> 
>  reverse-finish
>  Just as the finish command takes you to the point where the current
> function returns,
>  reverse-finish takes you to the point where it was called. Instead
> of ending up at the end of
>  the current function invocation, you end up at the beginning.
> 
> Say you have a line with multiple statements involving multiple
> function calls.
> The simplest would be:
> 
>   func1 ();  func2 ();
> 
> Say you'd stopped inside 'func2'.  If you do finish there, in current
> master gdb
> stops at the call to 'func2', and you can then decide to reverse step
> into 'func1'.

I don't think you followed the issue.  

So, if you are in func2 and do a reverse-finish, without the patch gdb
stops on the last instruction for the line that calls func2.  Now if
you issue a reverse-step, you stop at the first instruction for the
call to func2, i.e. you are still on the same source code line.  You
have not stepped back into func1 like you wanted to.  Now you have to
issue a second reverse-step to get into func1.   With the patch, you
issue the reverse-finish from func2 and stop at the first instruction
in the line that calls func2.  Now when you issue the reverse-step you
step back into func1 as expected.

> While with your change, reverse-finish in func2 will make gdb stop at
> the beginning
> of the line, before the call to func1.
> 
> Thus I'm really not convinced (yet) this change is a good one.  It
> removes
> a user choice.  You can always do reverse-next/step do get what you
> want
> reverse-finish do to, already.

                                    Carl
  

On 2023-01-23 9:13 p.m., Carl Love wrote:
> Pedro:
> 
> On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
>>> Currently on X86, when executing the finish command in reverse, gdb
>>> does a
>>> single step from the first instruction in the callee to get back to
>>> the
>>> caller.  GDB stops on the last instruction in the source code line
>>> where
>>> the call was made.  When stopped at the last instruction of the
>>> source code
>>> line, a reverse next or step command will stop at the first
>>> instruction
>>> of the same source code line thus requiring two step/next commands
>>> to
>>> reach the previous source code line.  It should only require one
>>> step/next
>>> command to reach the previous source code line.
>>>
>>> By contrast, a reverse next or step command from the first line in
>>> a
>>> function stops at the first instruction in the source code line
>>> where the
>>> call was made.
>>
>> I'd think this was on purpose.  Note that next/step/reverse-
>> {next/step} are line-oriented
>> stepping commands, they step/next until the previous/next line. 
>> While "finish" is described
>> as undoing the _function call_.
>>
>> The manual says:
>>
>>  reverse-finish
>>  Just as the finish command takes you to the point where the current
>> function returns,
>>  reverse-finish takes you to the point where it was called. Instead
>> of ending up at the end of
>>  the current function invocation, you end up at the beginning.
>>
>> Say you have a line with multiple statements involving multiple
>> function calls.
>> The simplest would be:
>>
>>   func1 ();  func2 ();
>>
>> Say you'd stopped inside 'func2'.  If you do finish there, in current
>> master gdb
>> stops at the call to 'func2', and you can then decide to reverse step
>> into 'func1'.
> 
> I don't think you followed the issue.  

Totally possible!

> 
> So, if you are in func2 and do a reverse-finish, without the patch gdb
> stops on the last instruction for the line that calls func2.  

Right.

> Now if
> you issue a reverse-step, you stop at the first instruction for the
> call to func2, i.e. you are still on the same source code line.  

Wait.  That right there sounds bogus.  The source line looks like:

   func1 ();  func2 ();

so stepping backwards over that line should always stop at the first
instruction of the line, not in the middle.  Let's simplify this.

Here's the full source code of my example:

(gdb) list 1
1       void func1 ()
2       {
3       }
4
5       void func2 ()
6       {
7       }
8
9       int main ()
10      {
11        func1 (); func2 ();
12      }

Compiled with:

 $ gcc reverse.c -o reverse -g3 -O0
 $ gcc -v
 ...
 gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)

Now let's debug it with target record, using current gdb git master (f3d8ae90b236),
without your patch:

 $ gdb ~/reverse
 GNU gdb (GDB) 14.0.50.20230124-git
 ...
 Reading symbols from /home/pedro/reverse...
 (gdb) start
 Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
 Starting program: /home/pedro/reverse 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
 
 Temporary breakpoint 1, main () at reverse.c:11
 11        func1 (); func2 ();
 (gdb) record 

 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x000055555555513f <+0>:     endbr64
    0x0000555555555143 <+4>:     push   %rbp
    0x0000555555555144 <+5>:     mov    %rsp,%rbp
 
 11        func1 (); func2 ();
 => 0x0000555555555147 <+8>:     mov    $0x0,%eax
    0x000055555555514c <+13>:    call   0x555555555129 <func1>
    0x0000555555555151 <+18>:    mov    $0x0,%eax
    0x0000555555555156 <+23>:    call   0x555555555134 <func2>
    0x000055555555515b <+28>:    mov    $0x0,%eax
 
 12      }
    0x0000555555555160 <+33>:    pop    %rbp
    0x0000555555555161 <+34>:    ret
 End of assembler dump.

 (gdb) n
 12      }

So far so good, a "next" stepped over the whole of line 11 and stopped at line 12.

Let's confirm where we are now:

 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x000055555555513f <+0>:     endbr64
    0x0000555555555143 <+4>:     push   %rbp
    0x0000555555555144 <+5>:     mov    %rsp,%rbp
 
 11        func1 (); func2 ();
    0x0000555555555147 <+8>:     mov    $0x0,%eax
    0x000055555555514c <+13>:    call   0x555555555129 <func1>
    0x0000555555555151 <+18>:    mov    $0x0,%eax
    0x0000555555555156 <+23>:    call   0x555555555134 <func2>
    0x000055555555515b <+28>:    mov    $0x0,%eax
 
 12      }
 => 0x0000555555555160 <+33>:    pop    %rbp
    0x0000555555555161 <+34>:    ret
 End of assembler dump.

Good, we're at the first instruction of line 12.

Now let's undo the "next", with "reverse-next":

 (gdb) reverse-next
 11        func1 (); func2 ();

Seemingly stopped at line 11.  Let's see exactly where:

 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x000055555555513f <+0>:     endbr64
    0x0000555555555143 <+4>:     push   %rbp
    0x0000555555555144 <+5>:     mov    %rsp,%rbp
 
 11        func1 (); func2 ();
    0x0000555555555147 <+8>:     mov    $0x0,%eax
    0x000055555555514c <+13>:    call   0x555555555129 <func1>
 => 0x0000555555555151 <+18>:    mov    $0x0,%eax
    0x0000555555555156 <+23>:    call   0x555555555134 <func2>
    0x000055555555515b <+28>:    mov    $0x0,%eax
 
 12      }
    0x0000555555555160 <+33>:    pop    %rbp
    0x0000555555555161 <+34>:    ret
 End of assembler dump.
 (gdb) 

And lo, we stopped in the middle of line 11!  That is a bug, we should have stepped
back all the way to the beginning of the line.  The "reverse-next" should have fully
undone the prior "next" command.  Here's the same thing without the distracting
disassemble commands:

 (gdb) b 11
 Breakpoint 1 at 0x1147: file reverse.c, line 11.
 (gdb) r
 Starting program: /home/pedro/reverse 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
 
 Breakpoint 1, main () at reverse.c:11
 11        func1 (); func2 ();
 (gdb) p $pc
 $1 = (void (*)()) 0x555555555147 <main+8>
 (gdb) record 
 (gdb) next
 12      }
 (gdb) reverse-next
 11        func1 (); func2 ();
 (gdb) p $pc
 $2 = (void (*)()) 0x555555555151 <main+18>
 (gdb) 


This:

 next -> reverse-next -> next -> reverse-next

... should leave you at the same instruction.  But it doesn't in this example!

How does this happen?  Let's look at the line table as seen by GDB:

 (gdb) maint info line-table reverse.c
 objfile: /home/pedro/reverse ((struct objfile *) 0x55dd5df77c50)
 compunit_symtab: reverse.c ((struct compunit_symtab *) 0x55dd5de6b2e0)
 symtab: /home/pedro/reverse.c ((struct symtab *) 0x55dd5de6b360)
 linetable: ((struct linetable *) 0x55dd5dfd3290):
 INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
 0      2      0x0000555555555129 Y                    
 1      3      0x0000555555555131 Y                    
 2      6      0x0000555555555134 Y                    
 3      7      0x000055555555513c Y                    
 4      10     0x000055555555513f Y                    
 5      11     0x0000555555555147 Y                      <<< here 
 6      11     0x0000555555555151 Y                      <<< here
 7      12     0x0000555555555160 Y                    
 8      END    0x0000555555555162 Y                    

Ah, there are two entries for line 11, both marked with IS-STMT.  So when
stepping backward GDB only considered the region with index 6, that's why it
stopped at 0x0000555555555151.



Let's look at what we get with clang instead (Ubuntu clang version 14.0.0-1ubuntu1) :

 (gdb) maintenance info line-table reverse.c
 objfile: /home/pedro/reverse.clang ((struct objfile *) 0x5576be591ca0)
 compunit_symtab: reverse.c ((struct compunit_symtab *) 0x5576be485300)
 symtab: /home/pedro/reverse.c ((struct symtab *) 0x5576be485380)
 linetable: ((struct linetable *) 0x5576be5ec8e0):
 INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
 0      2      0x0000555555555130 Y                    
 1      3      0x0000555555555134 Y       Y            
 2      6      0x0000555555555140 Y                    
 3      7      0x0000555555555144 Y       Y            
 4      10     0x0000555555555150 Y                    
 5      11     0x0000555555555154 Y       Y            
 6      11     0x0000555555555159                      
 7      12     0x000055555555515e Y                    
 8      END    0x0000555555555162 Y                    

Note no IS-STMT for the second range.  And let's look at how GDB behaves with it:

 (gdb) b 11
 Breakpoint 1 at 0x1154: file reverse.c, line 11.
 (gdb) r
 Starting program: /home/pedro/reverse.clang 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
 
 Breakpoint 1, main () at reverse.c:11
 11        func1 (); func2 ();
 (gdb) record
 (gdb) p $pc
 $1 = (void (*)()) 0x555555555154 <main+4>
 (gdb) n
 12      }
 (gdb) reverse-next
 
 No more reverse-execution history.
 main () at reverse.c:11
 11        func1 (); func2 ();
 (gdb) p $pc
 $2 = (void (*)()) 0x555555555154 <main+4>
 (gdb) 

Bingo.  reverse-next fully stepped the whole line backwards.

> You
> have not stepped back into func1 like you wanted to.  Now you have to
> issue a second reverse-step to get into func1.   With the patch, you
> issue the reverse-finish from func2 and stop at the first instruction
> in the line that calls func2.  Now when you issue the reverse-step you
> step back into func1 as expected.
> 

So this fix is assuming that the reverse step stops in the middle of a
line, which I think is the real bug to fix.  Once that is fixed, then
you will no longer need two reverse-steps after reverse-finish.

Here's what you get with current master without your patch, but using
the test program compiled with clang.  Actually, let's use a slightly
modified program to force clang to emit some instructions between
the two calls.  Like this:

 $ cat reverse.c
 void func1 (int i)
 {
 }
 
 void func2 (int i)
 {
 }
 
 int main (int argc, char **argv)
 {
   func1 (argc); func2 (argc);
 }

Alright, here's the gdb session, with clang, no gdb patch:

 Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
 11        func1 (argc); func2 (argc);
 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x0000555555555150 <+0>:     push   %rbp
    0x0000555555555151 <+1>:     mov    %rsp,%rbp
    0x0000555555555154 <+4>:     sub    $0x10,%rsp
    0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
    0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
 
 11        func1 (argc); func2 (argc);
 => 0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
    0x0000555555555162 <+18>:    call   0x555555555130 <func1>
    0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
    0x000055555555516a <+26>:    call   0x555555555140 <func2>
 
 12      }
    0x000055555555516f <+31>:    xor    %eax,%eax
    0x0000555555555171 <+33>:    add    $0x10,%rsp
    0x0000555555555175 <+37>:    pop    %rbp
    0x0000555555555176 <+38>:    ret
 End of assembler dump.
 (gdb) record 
 (gdb) b func2
 Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
 (gdb) c
 Continuing.
 
 Breakpoint 2, func2 (i=1) at reverse.c:7
 7       }
 (gdb) reverse-finish 
 Run back to call of #0  func2 (i=1) at reverse.c:7
 0x000055555555516a in main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
 11        func1 (argc); func2 (argc);
 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x0000555555555150 <+0>:     push   %rbp
    0x0000555555555151 <+1>:     mov    %rsp,%rbp
    0x0000555555555154 <+4>:     sub    $0x10,%rsp
    0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
    0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
 
 11        func1 (argc); func2 (argc);
    0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
    0x0000555555555162 <+18>:    call   0x555555555130 <func1>
    0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
 => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
 
 12      }
    0x000055555555516f <+31>:    xor    %eax,%eax
    0x0000555555555171 <+33>:    add    $0x10,%rsp
    0x0000555555555175 <+37>:    pop    %rbp
    0x0000555555555176 <+38>:    ret
 End of assembler dump.
 (gdb) reverse-step
 func1 (i=1) at reverse.c:3
 3       }
 (gdb) 
 
 
Note how a single reverse-step after the reverse-finish immediately
stepped backwards into func1.  Exactly how I describing it originally.

With your patch, you'd break reverse-finish with clang:

 (gdb) record 
 (gdb) b func2
 Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
 (gdb) c
 Continuing.

 Breakpoint 2, func2 (i=1) at reverse.c:7
 7       }
 (gdb) reverse-finish 
 Run back to call of #0  func2 (i=1) at reverse.c:7
 func1 (i=1) at reverse.c:3
 3       }
 (gdb) 

GDB stopped at line 3, info func1 which means it stepped too far without
stopping at func2's call site.

GDB is misbehaving with GCC's debug info.  I suspect the reason we get
the two line entries for line 11 and both with IS-STMT is because GCC emits
column debug info nowadays by default.   Here's what e.g.,
"llvm-dwarfdump --debug-line reverse" shows:

 ~~~
 Address            Line   Column File   ISA Discriminator Flags
 ------------------ ------ ------ ------ --- ------------- -------------
 0x0000000000001129      2      1      1   0             0  is_stmt
 0x0000000000001131      3      1      1   0             0  is_stmt
 0x0000000000001134      6      1      1   0             0  is_stmt
 0x000000000000113c      7      1      1   0             0  is_stmt
 0x000000000000113f     10      1      1   0             0  is_stmt
 0x0000000000001147     11      3      1   0             0  is_stmt
 0x0000000000001151     11     13      1   0             0  is_stmt
 0x0000000000001160     12      1      1   0             0  is_stmt
 0x0000000000001162     12      1      1   0             0  is_stmt end_sequence
 ~~~

We can try disabling that with -gno-column-info, let's see what we get:

 (gdb) maint info line-table reverse.c
 objfile: /home/pedro/reverse.nocol ((struct objfile *) 0x5611464f6c10)
 compunit_symtab: reverse.c ((struct compunit_symtab *) 0x5611463ea2e0)
 symtab: /home/pedro/reverse.c ((struct symtab *) 0x5611463ea360)
 linetable: ((struct linetable *) 0x561146474c80):
 INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
 0      2      0x0000555555555129 Y                    
 1      3      0x0000555555555134 Y                    
 2      6      0x0000555555555137 Y                    
 3      7      0x0000555555555142 Y                    
 4      10     0x0000555555555145 Y                    
 5      11     0x0000555555555158 Y                    
 6      12     0x0000555555555171 Y                    
 7      END    0x0000555555555173 Y                    
 
 (gdb) 

... and in llvm-dwarfdump:

 Address            Line   Column File   ISA Discriminator Flags
 ------------------ ------ ------ ------ --- ------------- -------------
 0x0000000000001129      2      0      1   0             0  is_stmt
 0x0000000000001134      3      0      1   0             0  is_stmt
 0x0000000000001137      6      0      1   0             0  is_stmt
 0x0000000000001142      7      0      1   0             0  is_stmt
 0x0000000000001145     10      0      1   0             0  is_stmt
 0x0000000000001158     11      0      1   0             0  is_stmt
 0x0000000000001171     12      0      1   0             0  is_stmt
 0x0000000000001173     12      0      1   0             0  is_stmt end_sequence

Bingo.  With no column info, only one entry for line 11.

Pedro Alves

>> While with your change, reverse-finish in func2 will make gdb stop at
>> the beginning
>> of the line, before the call to func1.
>>
>> Thus I'm really not convinced (yet) this change is a good one.  It
>> removes
>> a user choice.  You can always do reverse-next/step do get what you
>> want
>> reverse-finish do to, already.
> 
>                                     Carl 
>
  

On 24/01/2023 15:08, Pedro Alves wrote:
> On 2023-01-23 9:13 p.m., Carl Love wrote:
>> Pedro:
>>
>> On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
>>>> Currently on X86, when executing the finish command in reverse, gdb
>>>> does a
>>>> single step from the first instruction in the callee to get back to
>>>> the
>>>> caller.  GDB stops on the last instruction in the source code line
>>>> where
>>>> the call was made.  When stopped at the last instruction of the
>>>> source code
>>>> line, a reverse next or step command will stop at the first
>>>> instruction
>>>> of the same source code line thus requiring two step/next commands
>>>> to
>>>> reach the previous source code line.  It should only require one
>>>> step/next
>>>> command to reach the previous source code line.
>>>>
>>>> By contrast, a reverse next or step command from the first line in
>>>> a
>>>> function stops at the first instruction in the source code line
>>>> where the
>>>> call was made.
>>> I'd think this was on purpose.  Note that next/step/reverse-
>>> {next/step} are line-oriented
>>> stepping commands, they step/next until the previous/next line.
>>> While "finish" is described
>>> as undoing the _function call_.
>>>
>>> The manual says:
>>>
>>>   reverse-finish
>>>   Just as the finish command takes you to the point where the current
>>> function returns,
>>>   reverse-finish takes you to the point where it was called. Instead
>>> of ending up at the end of
>>>   the current function invocation, you end up at the beginning.
>>>
>>> Say you have a line with multiple statements involving multiple
>>> function calls.
>>> The simplest would be:
>>>
>>>    func1 ();  func2 ();
>>>
>>> Say you'd stopped inside 'func2'.  If you do finish there, in current
>>> master gdb
>>> stops at the call to 'func2', and you can then decide to reverse step
>>> into 'func1'.
>> I don't think you followed the issue.
> Totally possible!
>
>> So, if you are in func2 and do a reverse-finish, without the patch gdb
>> stops on the last instruction for the line that calls func2.
> Right.
>
>> Now if
>> you issue a reverse-step, you stop at the first instruction for the
>> call to func2, i.e. you are still on the same source code line.
> Wait.  That right there sounds bogus.  The source line looks like:
>
>     func1 ();  func2 ();
>
> so stepping backwards over that line should always stop at the first
> instruction of the line, not in the middle.  Let's simplify this.
>
> Here's the full source code of my example:
>
> (gdb) list 1
> 1       void func1 ()
> 2       {
> 3       }
> 4
> 5       void func2 ()
> 6       {
> 7       }
> 8
> 9       int main ()
> 10      {
> 11        func1 (); func2 ();
> 12      }
>
I think you are describing a different issue to what Carl is trying to 
solve. Using your example code, I have the following debugging session:

$ ./gdb -q reverse
Reading symbols from reverse...
(gdb) start
Temporary breakpoint 1 at 0x401118: file t.c, line 8.
Starting program: /home/blarsen/Documents/build/gdb/reverse
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main () at t.c:8
8           func1(); func2();
(gdb) record
(gdb) n
9       }
(gdb) rs
func2 () at t.c:5
5       }
(gdb) reverse-finish
Run back to call of #0  func2 () at t.c:5
0x0000000000401127 in main () at t.c:8
8           func1(); func2();
(gdb) rs
8           func1(); func2();
(gdb)
func1 () at t.c:2
2       }
(gdb)

Notice how there were two "reverse-step" commands needed after the 
"reverse-finish" used to exit func2. What Carl is proposing we do is 
make it so GDB only needs one command. If I compare the $pc of where GDB 
is stopped and the linetable, we get:

(gdb) print $pc
$2 = (void (*)()) 0x401127 <main+19>
(gdb) maint info line-table
objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct 
objfile *) 0x10f7750)
compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab 
*) 0x116c3b0)
linetable: ((struct linetable *) 0x11aec80):
INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
0      1      0x0000000000401106 Y
1      2      0x000000000040110a Y
2      4      0x000000000040110d Y
3      5      0x0000000000401111 Y
4      7      0x0000000000401114 Y
5      8      0x0000000000401118 Y
6      8      0x0000000000401122 Y
7      9      0x0000000000401131 Y
8      END    0x0000000000401133 Y

We can see that GDB shouldn't even be able to stop at that $pc, it isn't 
an is_stmt. We should have stopped at 0x401122, which is where the first 
reverse-step stops:

(gdb) rs
8           f1(); f2();
(gdb) p $pc
$4 = (void (*)()) 0x401122 <main+14>

So not only are we needing an extra command to re-sync with user 
expectations, we are in an instruction where the inferior state might be 
all over the place.
  

On 2023-01-24 2:23 p.m., Bruno Larsen wrote:
> On 24/01/2023 15:08, Pedro Alves wrote:
>> On 2023-01-23 9:13 p.m., Carl Love wrote:
>>> Pedro:
>>>
>>> On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
>>>>> Currently on X86, when executing the finish command in reverse, gdb
>>>>> does a
>>>>> single step from the first instruction in the callee to get back to
>>>>> the
>>>>> caller.  GDB stops on the last instruction in the source code line
>>>>> where
>>>>> the call was made.  When stopped at the last instruction of the
>>>>> source code
>>>>> line, a reverse next or step command will stop at the first
>>>>> instruction
>>>>> of the same source code line thus requiring two step/next commands
>>>>> to
>>>>> reach the previous source code line.  It should only require one
>>>>> step/next
>>>>> command to reach the previous source code line.
>>>>>
>>>>> By contrast, a reverse next or step command from the first line in
>>>>> a
>>>>> function stops at the first instruction in the source code line
>>>>> where the
>>>>> call was made.
>>>> I'd think this was on purpose.  Note that next/step/reverse-
>>>> {next/step} are line-oriented
>>>> stepping commands, they step/next until the previous/next line.
>>>> While "finish" is described
>>>> as undoing the _function call_.
>>>>
>>>> The manual says:
>>>>
>>>>   reverse-finish
>>>>   Just as the finish command takes you to the point where the current
>>>> function returns,
>>>>   reverse-finish takes you to the point where it was called. Instead
>>>> of ending up at the end of
>>>>   the current function invocation, you end up at the beginning.
>>>>
>>>> Say you have a line with multiple statements involving multiple
>>>> function calls.
>>>> The simplest would be:
>>>>
>>>>    func1 ();  func2 ();
>>>>
>>>> Say you'd stopped inside 'func2'.  If you do finish there, in current
>>>> master gdb
>>>> stops at the call to 'func2', and you can then decide to reverse step
>>>> into 'func1'.
>>> I don't think you followed the issue.
>> Totally possible!
>>
>>> So, if you are in func2 and do a reverse-finish, without the patch gdb
>>> stops on the last instruction for the line that calls func2.
>> Right.
>>
>>> Now if
>>> you issue a reverse-step, you stop at the first instruction for the
>>> call to func2, i.e. you are still on the same source code line.
>> Wait.  That right there sounds bogus.  The source line looks like:
>>
>>     func1 ();  func2 ();
>>
>> so stepping backwards over that line should always stop at the first
>> instruction of the line, not in the middle.  Let's simplify this.
>>
>> Here's the full source code of my example:
>>
>> (gdb) list 1
>> 1       void func1 ()
>> 2       {
>> 3       }
>> 4
>> 5       void func2 ()
>> 6       {
>> 7       }
>> 8
>> 9       int main ()
>> 10      {
>> 11        func1 (); func2 ();
>> 12      }
>>
> I think you are describing a different issue to what Carl is trying to solve. Using your example code, I have the following debugging session:

No, it's the exact same.  Please re-read.

> 
> $ ./gdb -q reverse
> Reading symbols from reverse...
> (gdb) start
> Temporary breakpoint 1 at 0x401118: file t.c, line 8.
> Starting program: /home/blarsen/Documents/build/gdb/reverse
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> 
> Temporary breakpoint 1, main () at t.c:8
> 8           func1(); func2();
> (gdb) record
> (gdb) n
> 9       }
> (gdb) rs
> func2 () at t.c:5
> 5       }
> (gdb) reverse-finish
> Run back to call of #0  func2 () at t.c:5
> 0x0000000000401127 in main () at t.c:8
> 8           func1(); func2();
> (gdb) rs
> 8           func1(); func2();
> (gdb)
> func1 () at t.c:2
> 2       }
> (gdb)
> 
> Notice how there were two "reverse-step" commands needed after the "reverse-finish" used to exit func2. 

Yes.  And the reason you need two "reverse-step" is because there are two line ranges for line 8, and reverse-step stops
at the beginning of the second range instead of at the beginning of the first.  It's the exact same as with my simpler
example of just doing "next" -> "reverse-next", which I used as a simpler example to explain the problem.

> What Carl is proposing we do is make it so GDB only needs one command.

I understand.  However, I am saying that that is papering over the actual problem, _and_ it only works in the situation
where you ended up with two line entries with is-stmt for the same line.  Note how the patch breaks clang, and gcc with
-gno-column-info...

> If I compare the $pc of where GDB is stopped and the linetable, we get:
> 
> (gdb) print $pc
> $2 = (void (*)()) 0x401127 <main+19>
> (gdb) maint info line-table
> objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct objfile *) 0x10f7750)
> compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
> symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab *) 0x116c3b0)
> linetable: ((struct linetable *) 0x11aec80):
> INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
> 0      1      0x0000000000401106 Y
> 1      2      0x000000000040110a Y
> 2      4      0x000000000040110d Y
> 3      5      0x0000000000401111 Y
> 4      7      0x0000000000401114 Y
> 5      8      0x0000000000401118 Y
> 6      8      0x0000000000401122 Y
> 7      9      0x0000000000401131 Y
> 8      END    0x0000000000401133 Y
> 
> We can see that GDB shouldn't even be able to stop at that $pc, it isn't an is_stmt. 

reverse-finish is not supposed to step backwards until it reaches is_stmt.  Doing so makes it
step backwards too much, as I've shown in my previous example.

> We should have stopped at 0x401122, which is where the first reverse-step stops:

No...

> 
> (gdb) rs
> 8           f1(); f2();
> (gdb) p $pc
> $4 = (void (*)()) 0x401122 <main+14>

... no because in the case where you don't have column debug info (or with clang), there won't be
an is-stmt entry for the f2 call/column, there will only be one single line entry for the whole of
line 8, so gdb would step back too much.

> 
> So not only are we needing an extra command to re-sync with user expectations, we are in an instruction where the inferior state might be all over the place.
> 

What does that mean, the state might be all over the place?  The DWARF should describe the locations of all variables accurately at all instructions.
  

Pedro:

On Tue, 2023-01-24 at 14:08 +0000, Pedro Alves wrote:
> On 2023-01-23 9:13 p.m., Carl Love wrote:
> > Pedro:
> > 
> > On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
> > > > Currently on X86, when executing the finish command in reverse,
> > > > gdb
> > > > does a
> > > > single step from the first instruction in the callee to get
> > > > back to
> > > > the
> > > > caller.  GDB stops on the last instruction in the source code
> > > > line
> > > > where
> > > > the call was made.  When stopped at the last instruction of the
> > > > source code
> > > > line, a reverse next or step command will stop at the first
> > > > instruction
> > > > of the same source code line thus requiring two step/next
> > > > commands
> > > > to
> > > > reach the previous source code line.  It should only require
> > > > one
> > > > step/next
> > > > command to reach the previous source code line.
> > > > 
> > > > By contrast, a reverse next or step command from the first line
> > > > in
> > > > a
> > > > function stops at the first instruction in the source code line
> > > > where the
> > > > call was made.
> > > 
> > > I'd think this was on purpose.  Note that next/step/reverse-
> > > {next/step} are line-oriented
> > > stepping commands, they step/next until the previous/next line. 
> > > While "finish" is described
> > > as undoing the _function call_.
> > > 
> > > The manual says:
> > > 
> > >   reverse-finish
> > >   Just as the finish command takes you to the point where the
> > > current
> > > function returns,
> > >   reverse-finish takes you to the point where it was called.
> > > Instead
> > > of ending up at the end of
> > >   the current function invocation, you end up at the beginning.
> > > 
> > > Say you have a line with multiple statements involving multiple
> > > function calls.
> > > The simplest would be:
> > > 
> > >    func1 ();  func2 ();
> > > 
> > > Say you'd stopped inside 'func2'.  If you do finish there, in
> > > current
> > > master gdb
> > > stops at the call to 'func2', and you can then decide to reverse
> > > step
> > > into 'func1'.
> > 
> > I don't think you followed the issue.  
> 
> Totally possible!
> 
> > So, if you are in func2 and do a reverse-finish, without the patch
> > gdb
> > stops on the last instruction for the line that calls func2.  
> 
> Right.
> 
> > Now if
> > you issue a reverse-step, you stop at the first instruction for the
> > call to func2, i.e. you are still on the same source code line.  
> 
> Wait.  That right there sounds bogus.  The source line looks like:
> 
>    func1 ();  func2 ();

My bad, I didn't catch that you were implying func1 and func2 as being
on the same source line.  There is an existing bugzilla for the case of
multiple executable statements on the same line.

https://sourceware.org/bugzilla/show_bug.cgi?id=28426

I have worked with Luis Machado <luis.machado@arm.com> on a patch to
address that issue.  We have posted a few versions of the patch but it
still needs some work for finish.  I wanted to get back to that patch
once the reverse-finish issue is done.

I need to spend some more time looking at the rest of your response to
understand everything you are talking about.  That said, my first read
looked like the issue in the bugzilla I mentioned.  The patch Luis and
I have for addressing multiple statements on the same line applies on
top of the X86 and PowerPC reverse-finish patches.  

Let me spend some more time looking at your response.  Thanks.

                      Carl 


> 
> so stepping backwards over that line should always stop at the first
> instruction of the line, not in the middle.  Let's simplify this.
  

On 1/23/23 20:17, Pedro Alves wrote:
> I'd think this was on purpose.  Note that next/step/reverse-{next/step} are line-oriented
> stepping commands, they step/next until the previous/next line.  While "finish" is described
> as undoing the_function call_.
> 
> The manual says:
> 
>   reverse-finish
>   Just as the finish command takes you to the point where the current function returns,
>   reverse-finish takes you to the point where it was called. Instead of ending up at the end of
>   the current function invocation, you end up at the beginning.

As well as:
...
finish
Continue running until just after function in the selected stack frame 
returns. Print the returned value (if any). This command can be 
abbreviated as fin.
...

It's only now that you mention the non-line nature of 
finish/reverse-finish that I realize that that is the case.  So I 
suppose the docs could be a bit more explicit about this aspect.

I suppose an intuitive way to make available the two approaches (so 
without the user losing options) would be to introduce finishi next to 
finish and reverse-finishi next to finish, with the new commands 
retaining the current functionality and the old ones being adjusted to 
respect line boundaries.  But having the commands change behaviour is 
likely to cause confusion (well, assuming users notice the difference in 
the first place), so I'm not sure if that's a good idea.

Thanks,
- Tom
  

On 24/01/2023 16:06, Pedro Alves wrote:
> On 2023-01-24 2:23 p.m., Bruno Larsen wrote:
>> On 24/01/2023 15:08, Pedro Alves wrote:
>>> On 2023-01-23 9:13 p.m., Carl Love wrote:
>>>> Pedro:
>>>>
>>>> On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
>>>>>> Currently on X86, when executing the finish command in reverse, gdb
>>>>>> does a
>>>>>> single step from the first instruction in the callee to get back to
>>>>>> the
>>>>>> caller.  GDB stops on the last instruction in the source code line
>>>>>> where
>>>>>> the call was made.  When stopped at the last instruction of the
>>>>>> source code
>>>>>> line, a reverse next or step command will stop at the first
>>>>>> instruction
>>>>>> of the same source code line thus requiring two step/next commands
>>>>>> to
>>>>>> reach the previous source code line.  It should only require one
>>>>>> step/next
>>>>>> command to reach the previous source code line.
>>>>>>
>>>>>> By contrast, a reverse next or step command from the first line in
>>>>>> a
>>>>>> function stops at the first instruction in the source code line
>>>>>> where the
>>>>>> call was made.
>>>>> I'd think this was on purpose.  Note that next/step/reverse-
>>>>> {next/step} are line-oriented
>>>>> stepping commands, they step/next until the previous/next line.
>>>>> While "finish" is described
>>>>> as undoing the _function call_.
>>>>>
>>>>> The manual says:
>>>>>
>>>>>    reverse-finish
>>>>>    Just as the finish command takes you to the point where the current
>>>>> function returns,
>>>>>    reverse-finish takes you to the point where it was called. Instead
>>>>> of ending up at the end of
>>>>>    the current function invocation, you end up at the beginning.
>>>>>
>>>>> Say you have a line with multiple statements involving multiple
>>>>> function calls.
>>>>> The simplest would be:
>>>>>
>>>>>     func1 ();  func2 ();
>>>>>
>>>>> Say you'd stopped inside 'func2'.  If you do finish there, in current
>>>>> master gdb
>>>>> stops at the call to 'func2', and you can then decide to reverse step
>>>>> into 'func1'.
>>>> I don't think you followed the issue.
>>> Totally possible!
>>>
>>>> So, if you are in func2 and do a reverse-finish, without the patch gdb
>>>> stops on the last instruction for the line that calls func2.
>>> Right.
>>>
>>>> Now if
>>>> you issue a reverse-step, you stop at the first instruction for the
>>>> call to func2, i.e. you are still on the same source code line.
>>> Wait.  That right there sounds bogus.  The source line looks like:
>>>
>>>      func1 ();  func2 ();
>>>
>>> so stepping backwards over that line should always stop at the first
>>> instruction of the line, not in the middle.  Let's simplify this.
>>>
>>> Here's the full source code of my example:
>>>
>>> (gdb) list 1
>>> 1       void func1 ()
>>> 2       {
>>> 3       }
>>> 4
>>> 5       void func2 ()
>>> 6       {
>>> 7       }
>>> 8
>>> 9       int main ()
>>> 10      {
>>> 11        func1 (); func2 ();
>>> 12      }
>>>
>> I think you are describing a different issue to what Carl is trying to solve. Using your example code, I have the following debugging session:
> No, it's the exact same.  Please re-read.
>
>> $ ./gdb -q reverse
>> Reading symbols from reverse...
>> (gdb) start
>> Temporary breakpoint 1 at 0x401118: file t.c, line 8.
>> Starting program: /home/blarsen/Documents/build/gdb/reverse
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib64/libthread_db.so.1".
>>
>> Temporary breakpoint 1, main () at t.c:8
>> 8           func1(); func2();
>> (gdb) record
>> (gdb) n
>> 9       }
>> (gdb) rs
>> func2 () at t.c:5
>> 5       }
>> (gdb) reverse-finish
>> Run back to call of #0  func2 () at t.c:5
>> 0x0000000000401127 in main () at t.c:8
>> 8           func1(); func2();
>> (gdb) rs
>> 8           func1(); func2();
>> (gdb)
>> func1 () at t.c:2
>> 2       }
>> (gdb)
>>
>> Notice how there were two "reverse-step" commands needed after the "reverse-finish" used to exit func2.
> Yes.  And the reason you need two "reverse-step" is because there are two line ranges for line 8, and reverse-step stops
> at the beginning of the second range instead of at the beginning of the first.  It's the exact same as with my simpler
> example of just doing "next" -> "reverse-next", which I used as a simpler example to explain the problem.

The simplified example is not the exact same use case. Looking at how 
the program executes forward, if we next over this line we skip 
everything, but if we step into func1 and use finish we will still stop 
in line 11 and the user can decide to step into func2 or not. That is 
why I assumed you were thinking of a different issue when I saw you 
using next and reverse-next as examples.

Looking at where GDB lands when finishing from func1 we get (code 
compiled with gcc -gno-column-info):

(gdb) start
Temporary breakpoint 1 at 0x401118: file t.c, line 8.
Starting program: /home/blarsen/Documents/downstream_build/gdb/reverse
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main () at t.c:8
8           f1(); f2();
(gdb) s
f1 () at t.c:2
2       }
(gdb) finish
Run till exit from #0  f1 () at t.c:2
0x0000000000401122 in main () at t.c:8
8           f1(); f2();
(gdb) disas /s
Dump of assembler code for function main:
t.c:
7       int main(){
    0x0000000000401114 <+0>:     push   %rbp
    0x0000000000401115 <+1>:     mov    %rsp,%rbp

8           f1(); f2();
    0x0000000000401118 <+4>:     mov    $0x0,%eax
    0x000000000040111d <+9>:     call   0x401106 <f1>
=> 0x0000000000401122 <+14>:    mov    $0x0,%eax
    0x0000000000401127 <+19>:    call   0x40110d <f2>
    0x000000000040112c <+24>:    mov    $0x0,%eax

9       }
    0x0000000000401131 <+29>:    pop    %rbp
    0x0000000000401132 <+30>:    ret
End of assembler dump.

The behavior that we should emulate when going backwards is that 
finishing from func2 stops us on the 'mov' instruction between the 
calls. Looking at the clang session that I cut off from the previous email:

Alright, here's the gdb session, with clang, no gdb patch:

  Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
  11        func1 (argc); func2 (argc);
  (gdb) disassemble /s
  Dump of assembler code for function main:
  reverse.c:
  10      {
     0x0000555555555150 <+0>:     push   %rbp
     0x0000555555555151 <+1>:     mov    %rsp,%rbp
     0x0000555555555154 <+4>:     sub    $0x10,%rsp
     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
  
  11        func1 (argc); func2 (argc);
  => 0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
     0x000055555555516a <+26>:    call   0x555555555140 <func2>
  
  12      }
     0x000055555555516f <+31>:    xor    %eax,%eax
     0x0000555555555171 <+33>:    add    $0x10,%rsp
     0x0000555555555175 <+37>:    pop    %rbp
     0x0000555555555176 <+38>:    ret
  End of assembler dump.
  (gdb) record
  (gdb) b func2
  Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
  (gdb) c
  Continuing.
  
  Breakpoint 2, func2 (i=1) at reverse.c:7
  7       }
  (gdb) reverse-finish
  Run back to call of #0  func2 (i=1) at reverse.c:7
  0x000055555555516a in main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
  11        func1 (argc); func2 (argc);
  (gdb) disassemble /s
  Dump of assembler code for function main:
  reverse.c:
  10      {
     0x0000555555555150 <+0>:     push   %rbp
     0x0000555555555151 <+1>:     mov    %rsp,%rbp
     0x0000555555555154 <+4>:     sub    $0x10,%rsp
     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
  
  11        func1 (argc); func2 (argc);
     0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
  => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
  
  12      }
     0x000055555555516f <+31>:    xor    %eax,%eax
     0x0000555555555171 <+33>:    add    $0x10,%rsp
     0x0000555555555175 <+37>:    pop    %rbp
     0x0000555555555176 <+38>:    ret
  End of assembler dump.
  (gdb) reverse-step
  func1 (i=1) at reverse.c:3
  3       }
  (gdb)

We can see that GDB stopped on the call instruction instead. So a user 
that finished from func1 or reverse-finished from func2 may see 
different inferior states.


>
>> What Carl is proposing we do is make it so GDB only needs one command.
> I understand.  However, I am saying that that is papering over the actual problem, _and_ it only works in the situation
> where you ended up with two line entries with is-stmt for the same line.  Note how the patch breaks clang, and gcc with
> -gno-column-info...
>
>> If I compare the $pc of where GDB is stopped and the linetable, we get:
>>
>> (gdb) print $pc
>> $2 = (void (*)()) 0x401127 <main+19>
>> (gdb) maint info line-table
>> objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct objfile *) 0x10f7750)
>> compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
>> symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab *) 0x116c3b0)
>> linetable: ((struct linetable *) 0x11aec80):
>> INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
>> 0      1      0x0000000000401106 Y
>> 1      2      0x000000000040110a Y
>> 2      4      0x000000000040110d Y
>> 3      5      0x0000000000401111 Y
>> 4      7      0x0000000000401114 Y
>> 5      8      0x0000000000401118 Y
>> 6      8      0x0000000000401122 Y
>> 7      9      0x0000000000401131 Y
>> 8      END    0x0000000000401133 Y
>>
>> We can see that GDB shouldn't even be able to stop at that $pc, it isn't an is_stmt.
> reverse-finish is not supposed to step backwards until it reaches is_stmt.  Doing so makes it
> step backwards too much, as I've shown in my previous example.
>
>> We should have stopped at 0x401122, which is where the first reverse-step stops:
> No...
>
>> (gdb) rs
>> 8           f1(); f2();
>> (gdb) p $pc
>> $4 = (void (*)()) 0x401122 <main+14>
> ... no because in the case where you don't have column debug info (or with clang), there won't be
> an is-stmt entry for the f2 call/column, there will only be one single line entry for the whole of
> line 8, so gdb would step back too much.
>
>> So not only are we needing an extra command to re-sync with user expectations, we are in an instruction where the inferior state might be all over the place.
>>
> What does that mean, the state might be all over the place?  The DWARF should describe the locations of all variables accurately at all instructions.
>
Unrelated to this specific issue, but I was looking at record/30025 
(https://sourceware.org/bugzilla/show_bug.cgi?id=30025) and it confused 
me because because GDB sometimes reported the incorrect parameter, but 
if we continue single-stepping through instructions, GDB eventually 
finds the correct value when we stop at an is_stmt instruction.

If I misunderstood something about 30025 and is_stmt, please let me 
know! but as far as I can see, this is what happened.
  

Pedro:

On Tue, 2023-01-24 at 14:08 +0000, Pedro Alves wrote:
> On 2023-01-23 9:13 p.m., Carl Love wrote:
> > Pedro:
> > 
> > On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
> > > > Currently on X86, when executing the finish command in reverse,
> > > > gdb
> > > > does a
> > > > single step from the first instruction in the callee to get
> > > > back to
> > > > the
> > > > caller.  GDB stops on the last instruction in the source code
> > > > line
> > > > where
> > > > the call was made.  When stopped at the last instruction of the
> > > > source code
> > > > line, a reverse next or step command will stop at the first
> > > > instruction
> > > > of the same source code line thus requiring two step/next
> > > > commands
> > > > to
> > > > reach the previous source code line.  It should only require
> > > > one
> > > > step/next
> > > > command to reach the previous source code line.
> > > > 
> > > > By contrast, a reverse next or step command from the first line
> > > > in
> > > > a
> > > > function stops at the first instruction in the source code line
> > > > where the
> > > > call was made.
> > > 
> > > I'd think this was on purpose.  Note that next/step/reverse-
> > > {next/step} are line-oriented
> > > stepping commands, they step/next until the previous/next line. 
> > > While "finish" is described
> > > as undoing the _function call_.
> > > 
> > > The manual says:
> > > 
> > >  reverse-finish
> > >  Just as the finish command takes you to the point where the
> > > current
> > > function returns,
> > >  reverse-finish takes you to the point where it was called.
> > > Instead
> > > of ending up at the end of
> > >  the current function invocation, you end up at the beginning.
> > > 
> > > Say you have a line with multiple statements involving multiple
> > > function calls.
> > > The simplest would be:
> > > 
> > >   func1 ();  func2 ();
> > > 
> > > Say you'd stopped inside 'func2'.  If you do finish there, in
> > > current
> > > master gdb
> > > stops at the call to 'func2', and you can then decide to reverse
> > > step
> > > into 'func1'.
> > 
> > I don't think you followed the issue.  
> 
> Totally possible!
> 
> > So, if you are in func2 and do a reverse-finish, without the patch
> > gdb
> > stops on the last instruction for the line that calls func2.  
> 
> Right.
> 
> > Now if
> > you issue a reverse-step, you stop at the first instruction for the
> > call to func2, i.e. you are still on the same source code line.  
> 
> Wait.  That right there sounds bogus.  The source line looks like:
> 
>    func1 ();  func2 ();
> 
> so stepping backwards over that line should always stop at the first
> instruction of the line, not in the middle.  Let's simplify this.
> 
> Here's the full source code of my example:
> 
> (gdb) list 1
> 1       void func1 ()
> 2       {
> 3       }
> 4
> 5       void func2 ()
> 6       {
> 7       }
> 8
> 9       int main ()
> 10      {
> 11        func1 (); func2 ();
> 12      }
> 
> Compiled with:
> 
>  $ gcc reverse.c -o reverse -g3 -O0
>  $ gcc -v
>  ...
>  gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
> 
> Now let's debug it with target record, using current gdb git master
> (f3d8ae90b236),
> without your patch:
> 
>  $ gdb ~/reverse
>  GNU gdb (GDB) 14.0.50.20230124-git
>  ...
>  Reading symbols from /home/pedro/reverse...
>  (gdb) start
>  Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
>  Starting program: /home/pedro/reverse 
>  [Thread debugging using libthread_db enabled]
>  Using host libthread_db library "/lib/x86_64-linux-
> gnu/libthread_db.so.1".
> 
>  Temporary breakpoint 1, main () at reverse.c:11
>  11        func1 (); func2 ();
>  (gdb) record 
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>  => 0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>     0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>     0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
> 
>  (gdb) n
>  12      }
> 
> So far so good, a "next" stepped over the whole of line 11 and
> stopped at line 12.
> 
> Let's confirm where we are now:
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>     0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>     0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>  => 0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
> 
> Good, we're at the first instruction of line 12.
> 
> Now let's undo the "next", with "reverse-next":
> 
>  (gdb) reverse-next
>  11        func1 (); func2 ();
> 
> Seemingly stopped at line 11.  Let's see exactly where:
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>     0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>  => 0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>     0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
>  (gdb) 
> 
> And lo, we stopped in the middle of line 11!  That is a bug, we
> should have stepped
> back all the way to the beginning of the line.  The "reverse-next"
> should have fully
> undone the prior "next" command.  Here's the same thing without the
> distracting
> disassemble commands:
> 
>  (gdb) b 11
>  Breakpoint 1 at 0x1147: file reverse.c, line 11.
>  (gdb) r
>  Starting program: /home/pedro/reverse 
>  [Thread debugging using libthread_db enabled]
>  Using host libthread_db library "/lib/x86_64-linux-
> gnu/libthread_db.so.1".
> 
>  Breakpoint 1, main () at reverse.c:11
>  11        func1 (); func2 ();
>  (gdb) p $pc
>  $1 = (void (*)()) 0x555555555147 <main+8>
>  (gdb) record 
>  (gdb) next
>  12      }
>  (gdb) reverse-next
>  11        func1 (); func2 ();
>  (gdb) p $pc
>  $2 = (void (*)()) 0x555555555151 <main+18>
>  (gdb) 
> 
> 
> This:
> 
>  next -> reverse-next -> next -> reverse-next
> 
> ... should leave you at the same instruction.  But it doesn't in this
> example!
> 
> How does this happen?  Let's look at the line table as seen by GDB:
> 
>  (gdb) maint info line-table reverse.c
>  objfile: /home/pedro/reverse ((struct objfile *) 0x55dd5df77c50)
>  compunit_symtab: reverse.c ((struct compunit_symtab *)
> 0x55dd5de6b2e0)
>  symtab: /home/pedro/reverse.c ((struct symtab *) 0x55dd5de6b360)
>  linetable: ((struct linetable *) 0x55dd5dfd3290):
>  INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
>  0      2      0x0000555555555129 Y                    
>  1      3      0x0000555555555131 Y                    
>  2      6      0x0000555555555134 Y                    
>  3      7      0x000055555555513c Y                    
>  4      10     0x000055555555513f Y                    
>  5      11     0x0000555555555147 Y                      <<< here 
>  6      11     0x0000555555555151 Y                      <<< here
>  7      12     0x0000555555555160 Y                    
>  8      END    0x0000555555555162 Y                    
> 
> Ah, there are two entries for line 11, both marked with IS-STMT.  So
> when
> stepping backward GDB only considered the region with index 6, that's
> why it
> stopped at 0x0000555555555151.

So, I walked thru your example on PowerPC and agree that the case where
we have  func1 (); func2 (); on the same source line fails with the
reverse-next command.  However, I think this is actually a "new"
regression failure for my patch.  My patch was trying to fix the
behavior of the reverse-finish command and appears to have broken the
reverse-next command for this scenario.

Note, the initial version of this patch also broke the reverse-next
command (gdb.btrace/rn-dl-bind.exp and gdb.btrace/tailcall.exp) but the
current version of the patch fixed the tailcall.exp failure.  Bruno and
I were not able to reproduce the failure for the rn-dl-bind.exp test. 
Not sure if the test still fails for Tom with the latest patch or not.

Anyway, see the status summary below.
> 
> 
> 
> Let's look at what we get with clang instead (Ubuntu clang version
> 14.0.0-1ubuntu1) :
> 
>  (gdb) maintenance info line-table reverse.c
>  objfile: /home/pedro/reverse.clang ((struct objfile *)
> 0x5576be591ca0)
>  compunit_symtab: reverse.c ((struct compunit_symtab *)
> 0x5576be485300)
>  symtab: /home/pedro/reverse.c ((struct symtab *) 0x5576be485380)
>  linetable: ((struct linetable *) 0x5576be5ec8e0):
>  INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
>  0      2      0x0000555555555130 Y                    
>  1      3      0x0000555555555134 Y       Y            
>  2      6      0x0000555555555140 Y                    
>  3      7      0x0000555555555144 Y       Y            
>  4      10     0x0000555555555150 Y                    
>  5      11     0x0000555555555154 Y       Y            
>  6      11     0x0000555555555159                      
>  7      12     0x000055555555515e Y                    
>  8      END    0x0000555555555162 Y                    
> 
> Note no IS-STMT for the second range.  And let's look at how GDB
> behaves with it:
> 
>  (gdb) b 11
>  Breakpoint 1 at 0x1154: file reverse.c, line 11.
>  (gdb) r
>  Starting program: /home/pedro/reverse.clang 
>  [Thread debugging using libthread_db enabled]
>  Using host libthread_db library "/lib/x86_64-linux-
> gnu/libthread_db.so.1".
> 
>  Breakpoint 1, main () at reverse.c:11
>  11        func1 (); func2 ();
>  (gdb) record
>  (gdb) p $pc
>  $1 = (void (*)()) 0x555555555154 <main+4>
>  (gdb) n
>  12      }
>  (gdb) reverse-next
> 
>  No more reverse-execution history.
>  main () at reverse.c:11
>  11        func1 (); func2 ();
>  (gdb) p $pc
>  $2 = (void (*)()) 0x555555555154 <main+4>
>  (gdb) 
> 
> Bingo.  reverse-next fully stepped the whole line backwards.
> 
> > You
> > have not stepped back into func1 like you wanted to.  Now you have
> > to
> > issue a second reverse-step to get into func1.   With the patch,
> > you
> > issue the reverse-finish from func2 and stop at the first
> > instruction
> > in the line that calls func2.  Now when you issue the reverse-step
> > you
> > step back into func1 as expected.
> > 
> 
> So this fix is assuming that the reverse step stops in the middle of
> a
> line, which I think is the real bug to fix.  Once that is fixed, then
> you will no longer need two reverse-steps after reverse-finish.
> 
> Here's what you get with current master without your patch, but using
> the test program compiled with clang.  Actually, let's use a slightly
> modified program to force clang to emit some instructions between
> the two calls.  Like this:
> 
>  $ cat reverse.c
>  void func1 (int i)
>  {
>  }
> 
>  void func2 (int i)
>  {
>  }
> 
>  int main (int argc, char **argv)
>  {
>    func1 (argc); func2 (argc);
>  }
> 
> Alright, here's the gdb session, with clang, no gdb patch:
> 
>  Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at
> reverse.c:11
>  11        func1 (argc); func2 (argc);
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x0000555555555150 <+0>:     push   %rbp
>     0x0000555555555151 <+1>:     mov    %rsp,%rbp
>     0x0000555555555154 <+4>:     sub    $0x10,%rsp
>     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
>     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
> 
>  11        func1 (argc); func2 (argc);
>  => 0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>     0x000055555555516a <+26>:    call   0x555555555140 <func2>
> 
>  12      }
>     0x000055555555516f <+31>:    xor    %eax,%eax
>     0x0000555555555171 <+33>:    add    $0x10,%rsp
>     0x0000555555555175 <+37>:    pop    %rbp
>     0x0000555555555176 <+38>:    ret
>  End of assembler dump.
>  (gdb) record 
>  (gdb) b func2
>  Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
>  (gdb) c
>  Continuing.
> 
>  Breakpoint 2, func2 (i=1) at reverse.c:7
>  7       }
>  (gdb) reverse-finish 
>  Run back to call of #0  func2 (i=1) at reverse.c:7
>  0x000055555555516a in main (argc=1, argv=0x7fffffffdc48) at
> reverse.c:11
>  11        func1 (argc); func2 (argc);
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x0000555555555150 <+0>:     push   %rbp
>     0x0000555555555151 <+1>:     mov    %rsp,%rbp
>     0x0000555555555154 <+4>:     sub    $0x10,%rsp
>     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
>     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
> 
>  11        func1 (argc); func2 (argc);
>     0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>  => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
> 
>  12      }
>     0x000055555555516f <+31>:    xor    %eax,%eax
>     0x0000555555555171 <+33>:    add    $0x10,%rsp
>     0x0000555555555175 <+37>:    pop    %rbp
>     0x0000555555555176 <+38>:    ret
>  End of assembler dump.
>  (gdb) reverse-step
>  func1 (i=1) at reverse.c:3
>  3       }
>  (gdb) 
> 
> 
> Note how a single reverse-step after the reverse-finish immediately
> stepped backwards into func1.  Exactly how I describing it
> originally.
> 
> With your patch, you'd break reverse-finish with clang:
> 
>  (gdb) record 
>  (gdb) b func2
>  Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
>  (gdb) c
>  Continuing.
> 
>  Breakpoint 2, func2 (i=1) at reverse.c:7
>  7       }
>  (gdb) reverse-finish 
>  Run back to call of #0  func2 (i=1) at reverse.c:7
>  func1 (i=1) at reverse.c:3
>  3       }
>  (gdb) 
> 
> GDB stopped at line 3, info func1 which means it stepped too far
> without
> stopping at func2's call site.
> 
> GDB is misbehaving with GCC's debug info.  I suspect the reason we
> get
> the two line entries for line 11 and both with IS-STMT is because GCC
> emits
> column debug info nowadays by default.   Here's what e.g.,
> "llvm-dwarfdump --debug-line reverse" shows:
> 
>  ~~~
>  Address            Line   Column File   ISA Discriminator Flags
>  ------------------ ------ ------ ------ --- ------------- ----------
> ---
>  0x0000000000001129      2      1      1   0             0  is_stmt
>  0x0000000000001131      3      1      1   0             0  is_stmt
>  0x0000000000001134      6      1      1   0             0  is_stmt
>  0x000000000000113c      7      1      1   0             0  is_stmt
>  0x000000000000113f     10      1      1   0             0  is_stmt
>  0x0000000000001147     11      3      1   0             0  is_stmt
>  0x0000000000001151     11     13      1   0             0  is_stmt
>  0x0000000000001160     12      1      1   0             0  is_stmt
>  0x0000000000001162     12      1      1   0             0  is_stmt
> end_sequence
>  ~~~
> 
> We can try disabling that with -gno-column-info, let's see what we
> get:
> 
>  (gdb) maint info line-table reverse.c
>  objfile: /home/pedro/reverse.nocol ((struct objfile *)
> 0x5611464f6c10)
>  compunit_symtab: reverse.c ((struct compunit_symtab *)
> 0x5611463ea2e0)
>  symtab: /home/pedro/reverse.c ((struct symtab *) 0x5611463ea360)
>  linetable: ((struct linetable *) 0x561146474c80):
>  INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
>  0      2      0x0000555555555129 Y                    
>  1      3      0x0000555555555134 Y                    
>  2      6      0x0000555555555137 Y                    
>  3      7      0x0000555555555142 Y                    
>  4      10     0x0000555555555145 Y                    
>  5      11     0x0000555555555158 Y                    
>  6      12     0x0000555555555171 Y                    
>  7      END    0x0000555555555173 Y                    
> 
>  (gdb) 
> 
> ... and in llvm-dwarfdump:
> 
>  Address            Line   Column File   ISA Discriminator Flags
>  ------------------ ------ ------ ------ --- ------------- ----------
> ---
>  0x0000000000001129      2      0      1   0             0  is_stmt
>  0x0000000000001134      3      0      1   0             0  is_stmt
>  0x0000000000001137      6      0      1   0             0  is_stmt
>  0x0000000000001142      7      0      1   0             0  is_stmt
>  0x0000000000001145     10      0      1   0             0  is_stmt
>  0x0000000000001158     11      0      1   0             0  is_stmt
>  0x0000000000001171     12      0      1   0             0  is_stmt
>  0x0000000000001173     12      0      1   0             0  is_stmt
> end_sequence
> 
> Bingo.  With no column info, only one entry for line 11.
> 

I have not tested with clang.  Actually I have never used clang so this
is another thing to look at and test.


Let me see if I can summarize the current situation.

1) The goal of the current X86 reverse patch is to fix the case where
we have called function foo () and are trying to do a reverse-finish
from inside the function.  The mainline gdb code stops at the entry to
foo () then does a single instruction in the reverse direction to get
to the caller.  Specifically, the code that this patch removes:

-      if (ecs->event_thread->control.proceed_to_finish  
-         && execution_direction == EXEC_REVERSE)
-       {         
-         struct thread_info *tp = ecs->event_thread;
-                  
-         /* We are finishing a function in reverse, and just hit the
-            step-resume breakpoint at the start address of the
-            function, and we're almost there -- just need to back up
-            by one more single-step, which should take us back to the
-            function call.  */
-         tp->control.step_range_start = tp->control.step_range_end = 1;
-         keep_going (ecs);
-         return;

The single-step in gdb results in stopping at the last instruction in
the line where function foo is called. The result here is that you now
need to do two reverse-next instructions to get to the previous line. 
The command sequence is:  reverse-finish; reverse-next; reverst-next to
go from inside the function to the previous line in the caller.

Note, in this case you are in the function and trying to return to the
caller with the reverse-finish command.  That is a different scenario
from what Pedro is talking about in 2) below.

2) The scenario that Pedro brings up is a reverse-next over a line with
two function calls on the same source line, i.e.

> 9       int main ()
> 10      {
> 11        func1 (); func2 ();
> 12      }

In this case you are in the caller and are trying to do a reverse-next
over the two function callers.  This is a different scenario from 1).

This looks to me like an additional regression failure of the patch. 
Unfortunately, this scenario does not seem to exit in any of the
current tests.  Mainline gdb handles this case correctly.  With my
patch, the reverse-next over the line now fails.

With out the patch, you just need reverse-next to step back over func1
(); func2 ();

With the patch you need reverse-next, reverse-next to step back over
the line with the two function calls.

3)  The bug that I mentioned earlier for the case of
multiple executable statements on the same line.

https://sourceware.org/bugzilla/show_bug.cgi?id=28426

is for a case like:

int main ()
{
  int a, b, c, d;
  a = 1;
  b = 2;
  c = 3; d = 4;
  a = a + b + c + d;
}

In this case the reverse-next from the last line a = a + b + c + d; is
not stepping all the way back over the line c = 3; d = 4;.  This is a
simpler version of 2).  Specifically the command sequence to step over
line c = 3; d = 4; is reverse-next, reverse-next.  Only one reverse-
next should be required.

The patch that Luis and I have worked on fixes this issue, however it
does not fix the case of the multiple statements being function calls,
i.e. 2) above that Pedro is bringing up.


From a subsequent message from Pedro in this thread:

   On Tue, 2023-01-24 at 15:06 +0000, Pedro Alves wrote:
   > Yes.  And the reason you need two "reverse-step" is because there are
   > two line ranges for line 8, and reverse-step stops
   > at the beginning of the second range instead of at the beginning of
   > the first.  It's the exact same as with my simpler
   > example of just doing "next" -> "reverse-next", which I used as a
   > simpler example to explain the problem.
   > 
   > > What Carl is proposing we do is make it so GDB only needs one
   > > command.
   > 
   > I understand.  However, I am saying that that is papering over the
   > actual problem, _and_ it only works in the situation
   > where you ended up with two line entries with is-stmt for the same
   > line.  Note how the patch breaks clang, and gcc with
   > -gno-column-info...

I don't agree that I am "papering over the actual problem" rather I
think at this point that Pedro's test case is an additional case of the
reverse-next command being broken by my patch to fix the reverse-finish 
command.  The problem doesn't exist without my patch.  


So at this point, I need to go see if I can figure out how the patch to
fix the reverse-finish command causes the regression in 2) for the
reverse-next command.

Looks like we also need an to add an additional test case for 2). 
Also, will need to look at how any new fix for 2) behaves with clang.

Thanks for all the input on this issue.  It seems that there is still
work to do.  

                         Carl
  

On 2023-01-24 3:51 p.m., Carl Love wrote:

> On Tue, 2023-01-24 at 14:08 +0000, Pedro Alves wrote:

>> Wait.  That right there sounds bogus.  The source line looks like:
>>
>>    func1 ();  func2 ();
> 
> My bad, I didn't catch that you were implying func1 and func2 as being
> on the same source line.  There is an existing bugzilla for the case of
> multiple executable statements on the same line.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28426
> 
> I have worked with Luis Machado <luis.machado@arm.com> on a patch to
> address that issue.  We have posted a few versions of the patch but it
> still needs some work for finish.  I wanted to get back to that patch
> once the reverse-finish issue is done.
> 
> I need to spend some more time looking at the rest of your response to
> understand everything you are talking about.  That said, my first read
> looked like the issue in the bugzilla I mentioned.  The patch Luis and
> I have for addressing multiple statements on the same line applies on
> top of the X86 and PowerPC reverse-finish patches.  

You should fix the reverse-stepping multiple statements issue first, and
then the reverse-finish problems go away, you won't need that patch any more.
  

On 2023-01-24 3:53 p.m., Tom de Vries wrote:

> On 1/23/23 20:17, Pedro Alves wrote:
>> I'd think this was on purpose.  Note that next/step/reverse-{next/step} are line-oriented
>> stepping commands, they step/next until the previous/next line.  While "finish" is described
>> as undoing the_function call_.
>>
>> The manual says:
>>
>>   reverse-finish
>>   Just as the finish command takes you to the point where the current function returns,
>>   reverse-finish takes you to the point where it was called. Instead of ending up at the end of
>>   the current function invocation, you end up at the beginning.
> 
> As well as:
> ...
> finish
> Continue running until just after function in the selected stack frame returns. Print the returned value (if any). This command can be abbreviated as fin.
> ...
> 
> It's only now that you mention the non-line nature of finish/reverse-finish that I realize that that is the case.  So I suppose the docs could be a bit more explicit about this aspect.

I guess.

> 
> I suppose an intuitive way to make available the two approaches (so without the user losing options) would be to introduce finishi next to finish and reverse-finishi next to finish, with the new commands retaining the current functionality and the old ones being adjusted to respect line boundaries.  But having the commands change behaviour is likely to cause confusion (well, assuming users notice the difference in the first place), so I'm not sure if that's a good idea.

They sure would notice a difference.  Say for example, a source line like:

   func1 (func2 (func3 ())));

and if you break in func3, and then do "finish", you expect that "step" steps into func2, while "next" would move to the next line.

If you changed finish to be line-oriented, then a "finish" while inside "func3" would either step over the whole "func1" call line instead,
or step into func2, depending on how you implement it.  Regardless, it'd be a noticeable change, and I think a bad one.
Also, it complicates where we print the return value, as we would no longer present the stop at the instruction where we are able
to retrieve it.
  

On 2023-01-24 4:04 p.m., Bruno Larsen wrote:
> On 24/01/2023 16:06, Pedro Alves wrote:

>>> Notice how there were two "reverse-step" commands needed after the "reverse-finish" used to exit func2.
>> Yes.  And the reason you need two "reverse-step" is because there are two line ranges for line 8, and reverse-step stops
>> at the beginning of the second range instead of at the beginning of the first.  It's the exact same as with my simpler
>> example of just doing "next" -> "reverse-next", which I used as a simpler example to explain the problem.
> 
> The simplified example is not the exact same use case. 

Correct, but it's the same root cause.  Forget at reverse-finish, that is doing what it says it should do, that is,
to stop at the call to the function.  You can just put a breakpoint at that same instruction, and once there, do a
reverse-step.  GDB should then reverse step until it reaches a different line.  But it does not.  _That_ is the bug.

Vis (with gcc and column info):

 ...
 (gdb) reverse-finish 
 Run back to call of #0  func2 (i=1) at reverse.c:7
 0x0000555555555167 in main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
 ^^^^^^^^^^^^^^^^^^
 11        func1 (argc); func2 (argc);
 (gdb) start
 The program being debugged has been started already.
 Start it from the beginning? (y or n) y
 Temporary breakpoint 2 at 0x555555555158: file reverse.c, line 11.
 Starting program: /home/pedro/reverse 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
 
 Temporary breakpoint 2, main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
 11        func1 (argc); func2 (argc);
 (gdb) record
 (gdb) b *0x0000555555555167              << manually set the breakpoint where the reverse-finish would stop
 Breakpoint 4 at 0x555555555167: file reverse.c, line 11.
 (gdb) c
 Continuing.

 Breakpoint 4, 0x0000555555555167 in main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
 11        func1 (argc); func2 (argc);    << started at line 11
 (gdb) reverse-step
 11        func1 (argc); func2 (argc);    << stopped at the same line, bug!

See, reverse-step stopped in the same line you started with.  That is a bug, it should never
happen.  Just like forward "step" never steps in the same line.  Because that's how the
commands are supposed to work -- step until the line number changes!

> Looking at how the program executes forward, if we next over this line we skip everything, but if we step into func1 and use finish we will still stop in line 11 and the user can decide to step into func2 or not. That is why I assumed you were thinking of a different issue when I saw you using next and reverse-next as examples.
> 
> Looking at where GDB lands when finishing from func1 we get (code compiled with gcc -gno-column-info):
> 
> (gdb) start
> Temporary breakpoint 1 at 0x401118: file t.c, line 8.
> Starting program: /home/blarsen/Documents/downstream_build/gdb/reverse
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> 
> Temporary breakpoint 1, main () at t.c:8
> 8           f1(); f2();
> (gdb) s
> f1 () at t.c:2
> 2       }
> (gdb) finish
> Run till exit from #0  f1 () at t.c:2
> 0x0000000000401122 in main () at t.c:8
> 8           f1(); f2();
> (gdb) disas /s
> Dump of assembler code for function main:
> t.c:
> 7       int main(){
>    0x0000000000401114 <+0>:     push   %rbp
>    0x0000000000401115 <+1>:     mov    %rsp,%rbp
> 
> 8           f1(); f2();
>    0x0000000000401118 <+4>:     mov    $0x0,%eax
>    0x000000000040111d <+9>:     call   0x401106 <f1>
> => 0x0000000000401122 <+14>:    mov    $0x0,%eax
>    0x0000000000401127 <+19>:    call   0x40110d <f2>
>    0x000000000040112c <+24>:    mov    $0x0,%eax
> 
> 9       }
>    0x0000000000401131 <+29>:    pop    %rbp
>    0x0000000000401132 <+30>:    ret
> End of assembler dump.
> 
> The behavior that we should emulate when going backwards is that finishing from func2 stops us on the 'mov' instruction between the calls. Looking at the clang session that I cut off from the previous email:
> 

What is there are more instructions in between the calls?  How do you know where to stop?

> Alright, here's the gdb session, with clang, no gdb patch:
> 
>  Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
>  11        func1 (argc); func2 (argc);
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x0000555555555150 <+0>:     push   %rbp
>     0x0000555555555151 <+1>:     mov    %rsp,%rbp
>     0x0000555555555154 <+4>:     sub    $0x10,%rsp
>     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
>     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
>  
>  11        func1 (argc); func2 (argc);
>  => 0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>     0x000055555555516a <+26>:    call   0x555555555140 <func2>
>  
>  12      }
>     0x000055555555516f <+31>:    xor    %eax,%eax
>     0x0000555555555171 <+33>:    add    $0x10,%rsp
>     0x0000555555555175 <+37>:    pop    %rbp
>     0x0000555555555176 <+38>:    ret
>  End of assembler dump.
>  (gdb) record
>  (gdb) b func2
>  Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
>  (gdb) c
>  Continuing.
>  
>  Breakpoint 2, func2 (i=1) at reverse.c:7
>  7       }
>  (gdb) reverse-finish
>  Run back to call of #0  func2 (i=1) at reverse.c:7
>  0x000055555555516a in main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
>  11        func1 (argc); func2 (argc);
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x0000555555555150 <+0>:     push   %rbp
>     0x0000555555555151 <+1>:     mov    %rsp,%rbp
>     0x0000555555555154 <+4>:     sub    $0x10,%rsp
>     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
>     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
>  
>  11        func1 (argc); func2 (argc);
>     0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>  => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
>  
>  12      }
>     0x000055555555516f <+31>:    xor    %eax,%eax
>     0x0000555555555171 <+33>:    add    $0x10,%rsp
>     0x0000555555555175 <+37>:    pop    %rbp
>     0x0000555555555176 <+38>:    ret
>  End of assembler dump.
>  (gdb) reverse-step
>  func1 (i=1) at reverse.c:3
>  3       }
>  (gdb)
> 
> We can see that GDB stopped on the call instruction instead. So a user that finished from func1 or reverse-finished from func2 may see different inferior states.
> 

Seeing different inferior states is expected, as finish and reverse-finish are not the exact mirror of one another, like step
and reverse-step are.  The exact reversal of finish would be the equivalent of being stopped at a function call return insn after
a "finish" (and the command could only be used while stopped there), and stepping back into the function up until the point
where you had typed "finish" and stopping there.  Obviously impossible to implement.  So we made "reverse-finish" do something
sensible, such as stepping backwards up until the call site.

> 
>>
>>> What Carl is proposing we do is make it so GDB only needs one command.
>> I understand.  However, I am saying that that is papering over the actual problem, _and_ it only works in the situation
>> where you ended up with two line entries with is-stmt for the same line.  Note how the patch breaks clang, and gcc with
>> -gno-column-info...
>>
>>> If I compare the $pc of where GDB is stopped and the linetable, we get:
>>>
>>> (gdb) print $pc
>>> $2 = (void (*)()) 0x401127 <main+19>
>>> (gdb) maint info line-table
>>> objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct objfile *) 0x10f7750)
>>> compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
>>> symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab *) 0x116c3b0)
>>> linetable: ((struct linetable *) 0x11aec80):
>>> INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
>>> 0      1      0x0000000000401106 Y
>>> 1      2      0x000000000040110a Y
>>> 2      4      0x000000000040110d Y
>>> 3      5      0x0000000000401111 Y
>>> 4      7      0x0000000000401114 Y
>>> 5      8      0x0000000000401118 Y
>>> 6      8      0x0000000000401122 Y
>>> 7      9      0x0000000000401131 Y
>>> 8      END    0x0000000000401133 Y
>>>
>>> We can see that GDB shouldn't even be able to stop at that $pc, it isn't an is_stmt.
>> reverse-finish is not supposed to step backwards until it reaches is_stmt.  Doing so makes it
>> step backwards too much, as I've shown in my previous example.
>>
>>> We should have stopped at 0x401122, which is where the first reverse-step stops:
>> No...
>>
>>> (gdb) rs
>>> 8           f1(); f2();
>>> (gdb) p $pc
>>> $4 = (void (*)()) 0x401122 <main+14>
>> ... no because in the case where you don't have column debug info (or with clang), there won't be
>> an is-stmt entry for the f2 call/column, there will only be one single line entry for the whole of
>> line 8, so gdb would step back too much.
>>
>>> So not only are we needing an extra command to re-sync with user expectations, we are in an instruction where the inferior state might be all over the place.
>>>
>> What does that mean, the state might be all over the place?  The DWARF should describe the locations of all variables accurately at all instructions.
>>
> Unrelated to this specific issue, but I was looking at record/30025 (https://sourceware.org/bugzilla/show_bug.cgi?id=30025) and it confused me because because GDB sometimes reported the incorrect parameter, but if we continue single-stepping through instructions, GDB eventually finds the correct value when we stop at an is_stmt instruction.
> 
> If I misunderstood something about 30025 and is_stmt, please let me know! but as far as I can see, this is what happened.
> 

I can't tell what is going on in that bug without debugging it myself, but seeing SIGTRAP makes me suspect that
one thing that could be going on is that GDB didn't apply the decr-pc-after-break adjustment properly, so
then the PC is pointing into the middle of an instruction and then all hell breaks loose.
  

On 2023-01-24 6:25 p.m., Carl Love wrote:

> I have not tested with clang.  Actually I have never used clang so this
> is another thing to look at and test.
> 
> 
> Let me see if I can summarize the current situation.
> 
> 1) The goal of the current X86 reverse patch is to fix the case where
> we have called function foo () and are trying to do a reverse-finish
> from inside the function.

Wrong goal.  There's nothing to fix there.  The command is working as designed.


>  The mainline gdb code stops at the entry to
> foo () then does a single instruction in the reverse direction to get
> to the caller.  Specifically, the code that this patch removes:
> 
> -      if (ecs->event_thread->control.proceed_to_finish  
> -         && execution_direction == EXEC_REVERSE)
> -       {         
> -         struct thread_info *tp = ecs->event_thread;
> -                  
> -         /* We are finishing a function in reverse, and just hit the
> -            step-resume breakpoint at the start address of the
> -            function, and we're almost there -- just need to back up
> -            by one more single-step, which should take us back to the
> -            function call.  */
> -         tp->control.step_range_start = tp->control.step_range_end = 1;
> -         keep_going (ecs);
> -         return;
> 
> The single-step in gdb results in stopping at the last instruction in
> the line where function foo is called. The result here is that you now
> need to do two reverse-next instructions to get to the previous line. 

And that is the bug.  reverse-next/reverse-step should _never_ stop at the same line.
Same as forward step/next.  For instance, this "next" command will just continue
stepping forever:

 Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc98) at reverse.c:11
 11        while (1);
 (gdb) next

Same for "step" instead of "next".

It turns out that reverse-next/step do stop immediately!

 (gdb) reverse-next
 11        while (1);
 (gdb) reverse-next
 11        while (1);
 (gdb) 

That's probably caused by the same bug.

> The command sequence is:  reverse-finish; reverse-next; reverst-next to
> go from inside the function to the previous line in the caller.
> 
> Note, in this case you are in the function and trying to return to the
> caller with the reverse-finish command.  That is a different scenario
> from what Pedro is talking about in 2) below.
> 
> 2) The scenario that Pedro brings up is a reverse-next over a line with
> two function calls on the same source line, i.e.
> 
>> 9       int main ()
>> 10      {
>> 11        func1 (); func2 ();
>> 12      }
> 
> In this case you are in the caller and are trying to do a reverse-next
> over the two function callers.  This is a different scenario from 1).
> 

I know it's a different scenario, but my scenario is just a way to show
up that the root of the problem can be seen with a simpler scenario.  The
problem is in the reverse-step command, and you are focusing on the reverse-finish
command, incorrectly.  Please read my responses to Bruno as well.

> 3)  The bug that I mentioned earlier for the case of
> multiple executable statements on the same line.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28426
> 
> is for a case like:
> 
> int main ()
> {
>   int a, b, c, d;
>   a = 1;
>   b = 2;
>   c = 3; d = 4;
>   a = a + b + c + d;
> }
> 
> In this case the reverse-next from the last line a = a + b + c + d; is
> not stepping all the way back over the line c = 3; d = 4;.  This is a
> simpler version of 2).  Specifically the command sequence to step over
> line c = 3; d = 4; is reverse-next, reverse-next.  Only one reverse-
> next should be required.
> 

Exactly.  It's all the same bug.

> The patch that Luis and I have worked on fixes this issue, however it
> does not fix the case of the multiple statements being function calls,
> i.e. 2) above that Pedro is bringing up.

I have said it numerous times now, but it's worth repeating.  It's all
the same bug.  :-)  Fix reverse-step, and then the reverse-finish
scenario fixes itself, because you will no longer need to issue
two reverse-step commands after reverse-finish.

> 
> 
> From a subsequent message from Pedro in this thread:
> 
>    On Tue, 2023-01-24 at 15:06 +0000, Pedro Alves wrote:
>    > Yes.  And the reason you need two "reverse-step" is because there are
>    > two line ranges for line 8, and reverse-step stops
>    > at the beginning of the second range instead of at the beginning of
>    > the first.  It's the exact same as with my simpler
>    > example of just doing "next" -> "reverse-next", which I used as a
>    > simpler example to explain the problem.
>    > 
>    > > What Carl is proposing we do is make it so GDB only needs one
>    > > command.
>    > 
>    > I understand.  However, I am saying that that is papering over the
>    > actual problem, _and_ it only works in the situation
>    > where you ended up with two line entries with is-stmt for the same
>    > line.  Note how the patch breaks clang, and gcc with
>    > -gno-column-info...
> 
> I don't agree that I am "papering over the actual problem" rather I
> think at this point that Pedro's test case is an additional case of the
> reverse-next command being broken by my patch to fix the reverse-finish 
> command.  The problem doesn't exist without my patch.  
> 
> 
> So at this point, I need to go see if I can figure out how the patch to
> fix the reverse-finish command causes the regression in 2) for the
> reverse-next command.

At this point.  NAK on the reverse-finish fix.

> 
> Looks like we also need an to add an additional test case for 2). 
> Also, will need to look at how any new fix for 2) behaves with clang.
> 
> Thanks for all the input on this issue.  It seems that there is still
> work to do.
  

On 2023-01-24 7:21 p.m., Pedro Alves wrote:
> And that is the bug.  reverse-next/reverse-step should _never_ stop at the same line.
> Same as forward step/next.  For instance, this "next" command will just continue
> stepping forever:
> 
>  Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc98) at reverse.c:11
>  11        while (1);
>  (gdb) next
> 
> Same for "step" instead of "next".
> 
> It turns out that reverse-next/step do stop immediately!
> 
>  (gdb) reverse-next
>  11        while (1);
>  (gdb) reverse-next
>  11        while (1);
>  (gdb) 
> 
> That's probably caused by the same bug.


From the documentation:

 reverse-step [count]
 Run the program backward until control reaches the start of a different source line;
                                                               ^^^^^^^^^

Emphasis mine.
  

On 24/01/2023 20:12, Pedro Alves wrote:
> On 2023-01-24 4:04 p.m., Bruno Larsen wrote:
>> On 24/01/2023 16:06, Pedro Alves wrote:
>>>> Notice how there were two "reverse-step" commands needed after the "reverse-finish" used to exit func2.
>>> Yes.  And the reason you need two "reverse-step" is because there are two line ranges for line 8, and reverse-step stops
>>> at the beginning of the second range instead of at the beginning of the first.  It's the exact same as with my simpler
>>> example of just doing "next" -> "reverse-next", which I used as a simpler example to explain the problem.
>> The simplified example is not the exact same use case.
> Correct, but it's the same root cause.  Forget at reverse-finish, that is doing what it says it should do, that is,
> to stop at the call to the function.  You can just put a breakpoint at that same instruction, and once there, do a
> reverse-step.  GDB should then reverse step until it reaches a different line.  But it does not.  _That_ is the bug.
Ok, you convinced me. I played around some more with the no-column-info 
example and what you said and I agree that fixing the column info bug 
would fix Carl's bug for this patch. I'll update the relevant bug 
(https://sourceware.org/bugzilla/show_bug.cgi?id=28426) and now we have 
a way to reproduce that!
>
> Vis (with gcc and column info):
>
>   ...
>   (gdb) reverse-finish
>   Run back to call of #0  func2 (i=1) at reverse.c:7
>   0x0000555555555167 in main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
>   ^^^^^^^^^^^^^^^^^^
>   11        func1 (argc); func2 (argc);
>   (gdb) start
>   The program being debugged has been started already.
>   Start it from the beginning? (y or n) y
>   Temporary breakpoint 2 at 0x555555555158: file reverse.c, line 11.
>   Starting program: /home/pedro/reverse
>   [Thread debugging using libthread_db enabled]
>   Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>   
>   Temporary breakpoint 2, main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
>   11        func1 (argc); func2 (argc);
>   (gdb) record
>   (gdb) b *0x0000555555555167              << manually set the breakpoint where the reverse-finish would stop
>   Breakpoint 4 at 0x555555555167: file reverse.c, line 11.
>   (gdb) c
>   Continuing.
>
>   Breakpoint 4, 0x0000555555555167 in main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
>   11        func1 (argc); func2 (argc);    << started at line 11
>   (gdb) reverse-step
>   11        func1 (argc); func2 (argc);    << stopped at the same line, bug!
>
> See, reverse-step stopped in the same line you started with.  That is a bug, it should never
> happen.  Just like forward "step" never steps in the same line.  Because that's how the
> commands are supposed to work -- step until the line number changes!
>
>> Looking at how the program executes forward, if we next over this line we skip everything, but if we step into func1 and use finish we will still stop in line 11 and the user can decide to step into func2 or not. That is why I assumed you were thinking of a different issue when I saw you using next and reverse-next as examples.
>>
>> Looking at where GDB lands when finishing from func1 we get (code compiled with gcc -gno-column-info):
>>
>> (gdb) start
>> Temporary breakpoint 1 at 0x401118: file t.c, line 8.
>> Starting program: /home/blarsen/Documents/downstream_build/gdb/reverse
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib64/libthread_db.so.1".
>>
>> Temporary breakpoint 1, main () at t.c:8
>> 8           f1(); f2();
>> (gdb) s
>> f1 () at t.c:2
>> 2       }
>> (gdb) finish
>> Run till exit from #0  f1 () at t.c:2
>> 0x0000000000401122 in main () at t.c:8
>> 8           f1(); f2();
>> (gdb) disas /s
>> Dump of assembler code for function main:
>> t.c:
>> 7       int main(){
>>     0x0000000000401114 <+0>:     push   %rbp
>>     0x0000000000401115 <+1>:     mov    %rsp,%rbp
>>
>> 8           f1(); f2();
>>     0x0000000000401118 <+4>:     mov    $0x0,%eax
>>     0x000000000040111d <+9>:     call   0x401106 <f1>
>> => 0x0000000000401122 <+14>:    mov    $0x0,%eax
>>     0x0000000000401127 <+19>:    call   0x40110d <f2>
>>     0x000000000040112c <+24>:    mov    $0x0,%eax
>>
>> 9       }
>>     0x0000000000401131 <+29>:    pop    %rbp
>>     0x0000000000401132 <+30>:    ret
>> End of assembler dump.
>>
>> The behavior that we should emulate when going backwards is that finishing from func2 stops us on the 'mov' instruction between the calls. Looking at the clang session that I cut off from the previous email:
>>
> What is there are more instructions in between the calls?  How do you know where to stop?
>
>> Alright, here's the gdb session, with clang, no gdb patch:
>>
>>   Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
>>   11        func1 (argc); func2 (argc);
>>   (gdb) disassemble /s
>>   Dump of assembler code for function main:
>>   reverse.c:
>>   10      {
>>      0x0000555555555150 <+0>:     push   %rbp
>>      0x0000555555555151 <+1>:     mov    %rsp,%rbp
>>      0x0000555555555154 <+4>:     sub    $0x10,%rsp
>>      0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
>>      0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
>>   
>>   11        func1 (argc); func2 (argc);
>>   => 0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>>      0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>>      0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>>      0x000055555555516a <+26>:    call   0x555555555140 <func2>
>>   
>>   12      }
>>      0x000055555555516f <+31>:    xor    %eax,%eax
>>      0x0000555555555171 <+33>:    add    $0x10,%rsp
>>      0x0000555555555175 <+37>:    pop    %rbp
>>      0x0000555555555176 <+38>:    ret
>>   End of assembler dump.
>>   (gdb) record
>>   (gdb) b func2
>>   Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
>>   (gdb) c
>>   Continuing.
>>   
>>   Breakpoint 2, func2 (i=1) at reverse.c:7
>>   7       }
>>   (gdb) reverse-finish
>>   Run back to call of #0  func2 (i=1) at reverse.c:7
>>   0x000055555555516a in main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
>>   11        func1 (argc); func2 (argc);
>>   (gdb) disassemble /s
>>   Dump of assembler code for function main:
>>   reverse.c:
>>   10      {
>>      0x0000555555555150 <+0>:     push   %rbp
>>      0x0000555555555151 <+1>:     mov    %rsp,%rbp
>>      0x0000555555555154 <+4>:     sub    $0x10,%rsp
>>      0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
>>      0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
>>   
>>   11        func1 (argc); func2 (argc);
>>      0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>>      0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>>      0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>>   => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
>>   
>>   12      }
>>      0x000055555555516f <+31>:    xor    %eax,%eax
>>      0x0000555555555171 <+33>:    add    $0x10,%rsp
>>      0x0000555555555175 <+37>:    pop    %rbp
>>      0x0000555555555176 <+38>:    ret
>>   End of assembler dump.
>>   (gdb) reverse-step
>>   func1 (i=1) at reverse.c:3
>>   3       }
>>   (gdb)
>>
>> We can see that GDB stopped on the call instruction instead. So a user that finished from func1 or reverse-finished from func2 may see different inferior states.
>>
> Seeing different inferior states is expected, as finish and reverse-finish are not the exact mirror of one another, like step
> and reverse-step are.  The exact reversal of finish would be the equivalent of being stopped at a function call return insn after
> a "finish" (and the command could only be used while stopped there), and stepping back into the function up until the point
> where you had typed "finish" and stopping there.  Obviously impossible to implement.  So we made "reverse-finish" do something
> sensible, such as stepping backwards up until the call site.
>
>>>> What Carl is proposing we do is make it so GDB only needs one command.
>>> I understand.  However, I am saying that that is papering over the actual problem, _and_ it only works in the situation
>>> where you ended up with two line entries with is-stmt for the same line.  Note how the patch breaks clang, and gcc with
>>> -gno-column-info...
>>>
>>>> If I compare the $pc of where GDB is stopped and the linetable, we get:
>>>>
>>>> (gdb) print $pc
>>>> $2 = (void (*)()) 0x401127 <main+19>
>>>> (gdb) maint info line-table
>>>> objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct objfile *) 0x10f7750)
>>>> compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
>>>> symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab *) 0x116c3b0)
>>>> linetable: ((struct linetable *) 0x11aec80):
>>>> INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
>>>> 0      1      0x0000000000401106 Y
>>>> 1      2      0x000000000040110a Y
>>>> 2      4      0x000000000040110d Y
>>>> 3      5      0x0000000000401111 Y
>>>> 4      7      0x0000000000401114 Y
>>>> 5      8      0x0000000000401118 Y
>>>> 6      8      0x0000000000401122 Y
>>>> 7      9      0x0000000000401131 Y
>>>> 8      END    0x0000000000401133 Y
>>>>
>>>> We can see that GDB shouldn't even be able to stop at that $pc, it isn't an is_stmt.
>>> reverse-finish is not supposed to step backwards until it reaches is_stmt.  Doing so makes it
>>> step backwards too much, as I've shown in my previous example.
>>>
>>>> We should have stopped at 0x401122, which is where the first reverse-step stops:
>>> No...
>>>
>>>> (gdb) rs
>>>> 8           f1(); f2();
>>>> (gdb) p $pc
>>>> $4 = (void (*)()) 0x401122 <main+14>
>>> ... no because in the case where you don't have column debug info (or with clang), there won't be
>>> an is-stmt entry for the f2 call/column, there will only be one single line entry for the whole of
>>> line 8, so gdb would step back too much.
>>>
>>>> So not only are we needing an extra command to re-sync with user expectations, we are in an instruction where the inferior state might be all over the place.
>>>>
>>> What does that mean, the state might be all over the place?  The DWARF should describe the locations of all variables accurately at all instructions.
>>>
>> Unrelated to this specific issue, but I was looking at record/30025 (https://sourceware.org/bugzilla/show_bug.cgi?id=30025) and it confused me because because GDB sometimes reported the incorrect parameter, but if we continue single-stepping through instructions, GDB eventually finds the correct value when we stop at an is_stmt instruction.
>>
>> If I misunderstood something about 30025 and is_stmt, please let me know! but as far as I can see, this is what happened.
>>
> I can't tell what is going on in that bug without debugging it myself, but seeing SIGTRAP makes me suspect that
> one thing that could be going on is that GDB didn't apply the decr-pc-after-break adjustment properly, so
> then the PC is pointing into the middle of an instruction and then all hell breaks loose.
>
oh! This is very helpful, thank you!
  

Pedro Alves <pedro@palves.net> wrote:

> > 11        func1 (argc); func2 (argc);
> >    0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
> >    0x0000555555555162 <+18>:    call   0x555555555130 <func1>
> >    0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
> > => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
> > 
> > 12      }
> >    0x000055555555516f <+31>:    xor    %eax,%eax
> >    0x0000555555555171 <+33>:    add    $0x10,%rsp
> >    0x0000555555555175 <+37>:    pop    %rbp
> >    0x0000555555555176 <+38>:    ret
> > End of assembler dump.
> > (gdb) reverse-step
> > func1 (i=1) at reverse.c:3
> > 3       }
> > (gdb)
> >
> >We can see that GDB stopped on the call instruction instead. So a user that finished from func1 or reverse-finished from func2 may see different inferior states.

>Seeing different inferior states is expected, as finish and reverse-finish are not the exact mirror of one another, like step
>and reverse-step are.  The exact reversal of finish would be the equivalent of being stopped at a function call return insn after
>a "finish" (and the command could only be used while stopped there), and stepping back into the function up until the point
>where you had typed "finish" and stopping there.  Obviously impossible to implement.  So we made "reverse-finish" do something
>sensible, such as stepping backwards up until the call site.

Hi Pedro,

I certainly agree with you that reverse-step needs to be fixed,
and this explains at least part of the problems we were seeing.

However, I do think there is still another issue specific to
reverse-finish, along the lines Bruno pointed out.  And in fact,
I do think that there is (or at least should be!) a well-defined
symmetry between finish and reverse-finish:

The effect of "finish" is: skip until the end of the current
function and then do a "step".  (Semantically equivalently,
do a series of "next" until you leave the current function.)

Similarly, the effect of "reverse-finish" *should* be: reverse-skip
until the beginning of the current function and then do a reverse-step.
(Or semantically equivalent, do a series of "reverse-next" until you
leave the current function.)

However, the actual implementation of reverse-finish is subtly but
significantly different: it reverse-skips until the beginning of the
current function and then does a reverse-stepi (not reverse-step).

This has the at least surprising, but IMO even incorrect, effect
that when on the first line of a function, "reverse-finish" ends
up in a different place than "reverse-step".  I believe they 
ought to end up in the same place.


This was one of the primary intentions behind Carl's patch: change
reverse-finish so it does a reverse-step instead of a reverse-stepi.
(Of course, for this to be useful, reverse-step itself needs to work
correctly - which it doesn't right now as you point out.)

Bye,
Ulrich
  

On 2023-01-25 2:11 p.m., Ulrich Weigand wrote:
> Pedro Alves <pedro@palves.net> wrote:
> 
>>> 11        func1 (argc); func2 (argc);
>>>    0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
>>>    0x0000555555555162 <+18>:    call   0x555555555130 <func1>
>>>    0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
>>> => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
>>>
>>> 12      }
>>>    0x000055555555516f <+31>:    xor    %eax,%eax
>>>    0x0000555555555171 <+33>:    add    $0x10,%rsp
>>>    0x0000555555555175 <+37>:    pop    %rbp
>>>    0x0000555555555176 <+38>:    ret
>>> End of assembler dump.
>>> (gdb) reverse-step
>>> func1 (i=1) at reverse.c:3
>>> 3       }
>>> (gdb)
>>>
>>> We can see that GDB stopped on the call instruction instead. So a user that finished from func1 or reverse-finished from func2 may see different inferior states.
> 
>> Seeing different inferior states is expected, as finish and reverse-finish are not the exact mirror of one another, like step
>> and reverse-step are.  The exact reversal of finish would be the equivalent of being stopped at a function call return insn after
>> a "finish" (and the command could only be used while stopped there), and stepping back into the function up until the point
>> where you had typed "finish" and stopping there.  Obviously impossible to implement.  So we made "reverse-finish" do something
>> sensible, such as stepping backwards up until the call site.
> 
> Hi Pedro,
> 
> I certainly agree with you that reverse-step needs to be fixed,
> and this explains at least part of the problems we were seeing.
> 
> However, I do think there is still another issue specific to
> reverse-finish, along the lines Bruno pointed out.  And in fact,
> I do think that there is (or at least should be!) a well-defined
> symmetry between finish and reverse-finish:
> 
> The effect of "finish" is: skip until the end of the current
> function and then do a "step".  (Semantically equivalently,
> do a series of "next" until you leave the current function.)

But that this is not actually true -- this is not how finish is implemented.

Instead, we unwind to the caller frame, and put a breakpoint at that
frame'c PC, and run freely to that.  A frame's PC is defined as the address
of the instruction that is executed once the callee returns.

OTOH, a (forward) step at the return line of a function steps until a different
line is reached, and in the case we step out of a function, once it reaches the
caller it notices we ended up in the middle of a line so it continues stepping until
the next line.

Note that finish vs step difference visible here:

(gdb) list 1
1       int func1 (int i)
2       {
3         return i + 1;
4       }
5
6       int func2 (int i)
7       {
8         return i + 1;
9       }
10
11      int main (int argc, char **argv)
12      {
13        func1 (func2 (argc));
14        return 0;
15      }
(gdb) 

(gdb) b func2
Breakpoint 1 at 0x1147: file finish.c, line 8.
(gdb) r
Starting program: /home/pedro/finish 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, func2 (i=1) at finish.c:8
8         return i + 1;
(gdb) finish
Run till exit from #0  func2 (i=1) at finish.c:8
0x000055555555516c in main (argc=1, argv=0x7fffffffdcb8) at finish.c:13    <<< stopped in the middle of the line, as indicated by address on the left
13        func1 (func2 (argc));
Value returned is $1 = 2                                                   <<< at this location we can use ABI knowledge to retrieve the return value
(gdb) p $pc
$2 = (void (*)()) 0x55555555516c <main+29>                                 <<< matches the "0x000055555555516c in main" above, of course

(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/pedro/finish 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, func2 (i=1) at finish.c:8
8         return i + 1;
(gdb) step
9       }                                                                  <<< stopped a line bounary, and stepped over the func1 call as well!
(gdb) p $pc
$3 = (void (*)()) 0x55555555514d <func2+17>


> 
> Similarly, the effect of "reverse-finish" *should* be: reverse-skip
> until the beginning of the current function and then do a reverse-step.
> (Or semantically equivalent, do a series of "reverse-next" until you
> leave the current function.)

I disagree, because that reverse-step would step backwards too much.  Instead of stepping back to the
call of the function, it would step back further, as reverse-step will try to stop at the
beginning of a line.  By doing that, you'll potentially step backwards more statements that
exist in the same line.  Like:

  a = 1; b = 2; func();

stepping back from inside func should stop at the first instruction of that whole line.
But finishing backwards should stop at the call to func(), without considering line
boundaries, just like forward finish does not.

From the manual:

   "Like the step command, reverse-step will only stop at the beginning of a source line. It “un-executes” the previously executed source line."

> 
> However, the actual implementation of reverse-finish is subtly but
> significantly different: it reverse-skips until the beginning of the
> current function and then does a reverse-stepi (not reverse-step).
> 
> This has the at least surprising, but IMO even incorrect, effect
> that when on the first line of a function, "reverse-finish" ends
> up in a different place than "reverse-step".  I believe they 
> ought to end up in the same place.

I disagree.

Pedro Alves

> 
> 
> This was one of the primary intentions behind Carl's patch: change
> reverse-finish so it does a reverse-step instead of a reverse-stepi.
> (Of course, for this to be useful, reverse-step itself needs to work
> correctly - which it doesn't right now as you point out.)
> 
> Bye,
> Ulrich
>
  

Pedro Alves <pedro@palves.net> wrote:
On 2023-01-25 2:11 p.m., Ulrich Weigand wrote:
> >The effect of "finish" is: skip until the end of the current
> >function and then do a "step".  (Semantically equivalently,
> >do a series of "next" until you leave the current function.)
>
>But that this is not actually true -- this is not how finish is implemented.

Ah, you're right.  I misremembered - I thought we were still stepping
after the step-resume breakpoint, but looks like we are not.

>Breakpoint 1, func2 (i=1) at finish.c:8
>8         return i + 1;
>(gdb) step
>9       }                                                                  <<< stopped a line bounary, and stepped over the func1 call as well!
>(gdb) p $pc
>$3 = (void (*)()) 0x55555555514d <func2+17>

So this is still at the return in func2, but you're right,
the next step brings us directly to func1 without stopping
in main at all.  This is indeed unlike finish.

>I disagree.

OK, you've convinced me as well.  Sorry for the confusion ...

Bye,
Ulrich
  

On 2023-01-25 5:13 p.m., Ulrich Weigand wrote:
>> Breakpoint 1, func2 (i=1) at finish.c:8
>> 8         return i + 1;
>> (gdb) step
>> 9       }                                                                  <<< stopped a line bounary, and stepped over the func1 call as well!
>> (gdb) p $pc
>> $3 = (void (*)()) 0x55555555514d <func2+17>
> So this is still at the return in func2, 

Whoops, the example that I quickly typed up didn't actually have an explicit return
line when I first tried it (it returned void), and then I thought of adding one and I added it
I missed that I now needed one extra step to get out of func, thinking I was at the last
line of main already.

> but you're right,
> the next step brings us directly to func1 without stopping
> in main at all.  This is indeed unlike finish.
> 

Exactly.

>> I disagree.
> OK, you've convinced me as well.  Sorry for the confusion ...

No worries!
  

Pedro, Ulrich, Tom, Bruno:

On Wed, 2023-01-25 at 17:24 +0000, Pedro Alves wrote:
> On 2023-01-25 5:13 p.m., Ulrich Weigand wrote:
> > > Breakpoint 1, func2 (i=1) at finish.c:8
> > > 8         return i + 1;
> > > (gdb) step
> > > 9       }                                                        
> > >           <<< stopped a line bounary, and stepped over the func1
> > > call as well!
> > > (gdb) p $pc
> > > $3 = (void (*)()) 0x55555555514d <func2+17>
> > So this is still at the return in func2, 
> 
> Whoops, the example that I quickly typed up didn't actually have an
> explicit return
> line when I first tried it (it returned void), and then I thought of
> adding one and I added it
> I missed that I now needed one extra step to get out of func,
> thinking I was at the last
> line of main already.
> 
> > but you're right,
> > the next step brings us directly to func1 without stopping
> > in main at all.  This is indeed unlike finish.
> > 
> 
> Exactly.
> 
> > > I disagree.
> > OK, you've convinced me as well.  Sorry for the confusion ...
> 
> No worries!

OK, so there seems to be agreement that we do not want to move forward
with this patch to change the behavior of the reverse-finish command. 
The consensus is the reverse-finish command is working as expected. So,
I formally withdraw patch 1 of 2 from consideration.

I worked on a patch with Luis, who is now busy with other things, to
fix the issue of two "simple" executable statements on a line not
working.  The patch fixed that issue, but does not fix the example from
Pedro of two function calls in the same source line.  So, I will pickup
this patch again and see if I can get the patch to correctly handle
column debug info and the is-stmt entry stuff and not break clang.

The second patch in this series address the reverse-finish command on
PowerPC which does not work due to the multiple entry points used in
PowerPC.  I will rework the PowerPC patch to apply on mainline to
address the PowerPC issues so the reverse-finish command will behave
the same as on X86.  This is probably the simpler/easier of the two
patches to do.

Thanks for all the discussion and help with this issue.

                                Carl