Re: [PATCH 3/3] Fix GDB reverse execution behavior

[Luis Machado <luis.machado@arm.com>]

On 11/22/23 23:33, Carl Love wrote:
> GDB maintainers:
> 
> The following patch changes the behavior of GDB when executing the next
> command in reverse over a line that contains multiple function calls on
> the same source code line.  Currently, GDB stops in the middle of the
> line not at the beginning of the line.  This patch causes GDB to stop
> at the beginning of the line.  The incorrect behavior was pointed out
> by Pedro Alves in 
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
> 
> A new testcase, func-map-to-same-line.exp, is added to verify the
> behavior of GDB.  
> 
> The fix requires updating two testcases, gdb/testsuite/gdb.mi/mi-
> reverse.exp and gdb.reverse/finish-reverse-next.exp.  The testcases
> have to issue two reverse next instructions to reach the previous
> source code line.  With this patch, they only require a single reverse
> next instruction to reach the previous line.
> 
> The patch has been tested on PowerPC and X86-64 with no regression
> errors.  
> 
> Please let me know if the patch is acceptable for mainline.   Thanks.
> 
>                          Carl 
> ----------------------------------------------------
> 
> Fix GDB reverse-step and reverse-next command behavior
> 
> Currently GDB when executing in reverse over multiple statements in a single
> line of source code, GDB stops in the middle of the line.  Thus requiring
> multiple commands to reach the previous line.  GDB should stop at the first
> instruction of the line, not in the middle of the line.
> 
> The following description of the incorrect behavior was taken from an
> earlier message by Pedro Alves <pedro@palves.net>:
> 
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
> 
> ---------------------------------
> 
> The source line looks like:
> 
>    func1 ();  func2 ();
> 
> in the test case:
> 
> (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),
> 
>  $ 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.
> 
> --------------------
> 
> This patch fixes the incorrect GDB behavior by ensuring that GDB  stops at
> the first instruction in the line.
> 
> The test case gdb.reverse/func-map-to-same-line.exp is added to testsuite
> to verify this fix when the line table information is and is not available.
> ---
>  gdb/infcmd.c                                  |  13 ++
>  gdb/testsuite/gdb.mi/mi-reverse.exp           |   5 +-
>  .../gdb.reverse/finish-reverse-next.exp       |  40 ++---
>  .../gdb.reverse/func-map-to-same-line.c       |  37 +++++
>  .../gdb.reverse/func-map-to-same-line.exp     | 140 ++++++++++++++++++
>  5 files changed, 201 insertions(+), 34 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
>  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index cf8cd527955..1415fe55163 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -975,6 +975,19 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
>  				 &tp->control.step_range_start,
>  				 &tp->control.step_range_end);
>  
> +	  if (execution_direction == EXEC_REVERSE)
> +	    {
> +	      symtab_and_line sal = find_pc_line (pc, 0);
> +	      symtab_and_line sal_start
> +		= find_pc_line (tp->control.step_range_start, 0);
> +
> +	      if (sal.line == sal_start.line)
> +		/* Executing in reverse, the step_range_start address is in
> +		   the same line.  We want to stop in the previous line so
> +		   move step_range_start before the current line.  */
> +		tp->control.step_range_start--;
> +	    }
> +
>  	  /* There's a problem in gcc (PR gcc/98780) that causes missing line
>  	     table entries, which results in a too large stepping range.
>  	     Use inlined_subroutine info to make the range more narrow.  */
> diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
> index baa53a495d7..0630b8b6c7f 100644
> --- a/gdb/testsuite/gdb.mi/mi-reverse.exp
> +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
> @@ -96,11 +96,8 @@ proc test_controlled_execution_reverse {} {
>  	"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"
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> index 1f53b649a7d..6488f66b107 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -76,14 +76,10 @@ gdb_continue_to_breakpoint \
>  repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100"
>  
>  # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line.  A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line.  Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line.  Another reverse-next
> +# instruction stops at the previous source code line.
>  gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
>      "reverse-finish function1 LEP call from LEP "
> -gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP" \
> -    "reverse next 1 LEP entry point function call from LEP"
>  gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call from LEP"
>  
>  
> @@ -109,14 +105,10 @@ gdb_continue_to_breakpoint \
>  gdb_test "step" ".*int ret = 0;.*" "step test 1"
>  
>  # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line.  A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line.  Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line.  Another reverse-next
> +# instruction stops at the previous source code line.
>  gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
>      "reverse-finish function1 LEP call from function body"
> -gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
> -    "reverse next 1 LEP from function body"
>  gdb_test "reverse-next" ".*b = 5;.*" \
>      "reverse next 2 at b = 5, from function body"
>  
> @@ -144,14 +136,10 @@ gdb_continue_to_breakpoint \
>  repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
>  
>  # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line.  A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line.  Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line.  Another reverse-next
> +# instruction stops at the previous source code line.
>  gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
>      "function1 GEP call call from GEP"
> -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> -    "reverse next 1 GEP entry point function call from GEP"
>  gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50, call from GEP"
>  
>  gdb_test "reverse-continue" ".*" "setup for test 4"
> @@ -180,14 +168,10 @@ repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call again"
>  gdb_test "stepi" "{" "stepi to between GEP and LEP"
>  
>  # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line.  A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line.  Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line.  Another reverse-next
> +# instruction stops at the previous source code line.
>  gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
>      "function1 GEP call call from GEP again"
> -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> -    "reverse next 1 GEP entry point function call from GEP again"
>  gdb_test "reverse-next" ".*b = 50;.*" \
>      "reverse next 2 at b = 50, call from GEP again"
>  
> @@ -212,13 +196,9 @@ gdb_continue_to_breakpoint \
>  gdb_test "step" ".*int ret = 0;.*" "step test 2"
>  
>  # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line.  A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line.  Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line.  Another reverse-next
> +# instruction stops at the previous source code line.
>  gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
>      "reverse-finish function1 GEP call, from function body  "
> -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> -    "reverse next 1 GEP call from function body"
>  gdb_test "reverse-next" ".*b = 50;.*" \
>      "reverse next 2 at b = 50 from function body"
> diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.c b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> new file mode 100644
> index 00000000000..17fe17af267
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> @@ -0,0 +1,37 @@
> +/* Copyright 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 test is used to test the reverse-step and reverse-next instruction
> +   execution for a source line that contains multiple function calls.  */
> +
> +void
> +func1 (void)
> +{
> +} /* END FUNC1 */
> +
> +void
> +func2 (void)
> +{
> +} /* END FUNC2 */
> +
> +int
> +main (void)
> +{
> +  int a, b;
> +  a = 1;
> +  b = 2;
> +  func1 (); func2 ();
> +  a = a + b;     /* START REVERSE TEST */
> +}
> diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> new file mode 100644
> index 00000000000..1b23233b43e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> @@ -0,0 +1,140 @@
> +# Copyright 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".
> +
> +# This test checks to make sure there is no regression failures for
> +# the reverse-next command when stepping back over two functions in
> +# the same line.
> +
> +require supports_reverse
> +
> +# This test uses the gcc no-column-info command which was added in gcc 7.1.
> +
> +proc run_tests {} {
> +    global testfile
> +
> +    clean_restart ${testfile}
> +
> +    if { ![runto_main] } {
> +	return
> +    }
> +
> +    with_test_prefix "next-test" {
> +	gdb_test_no_output "record" "turn on process record"
> +
> +	# This regression test verifies the reverse-step and reverse-next
> +	# commands work properly when executing backwards thru a source line
> +	# containing two function calls on the same source line, i.e.
> +	# func1 (); func2 ();.  This test is compiled so the dwarf info
> +	# does not contain the line table information.
> +
> +	# Test 1, reverse-next command
> +	# Set breakpoint at the line after the function calls.
> +	set bp_start_reverse_test [gdb_get_line_number "START REVERSE TEST"]
> +
> +	gdb_breakpoint $bp_start_reverse_test temporary
> +
> +	# Continue to break point for reverse-next test.
> +	# Command definition:  reverse-next [count]
> +	#   Run backward to the beginning of the previous line executed in
> +	#   the current (innermost) stack frame.  If the line contains
> +	#   function calls,they will be “un-executed” without stopping.

space after comma.

> +	#   Starting from the first line of a function, reverse-next will
> +	#   take you back to the caller of that function, before the
> +	#   function was called, just as the normal next command would take
> +	#   you from the last line of a function back to its return to its
> +	#   caller 2.
> +
> +	gdb_continue_to_breakpoint \
> +	"stopped at command reverse-next test start location" \
> +	".*$bp_start_reverse_test\r\n.*"
> +
> +	# The reverse-next should step all the way back to the beginning of
> +	# the line, i.e. at the beginning of the func1 call.
> +	gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	    " reverse-next to line with two functions"
> +
> +	# We should be stopped at the first instruction of the line.  A
> +	# reverse-step should step back and stop at the beginning of the
> +	# previous line b = 2, i.e. not in func1 ().
> +	gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	    "reverse-stepi to previous line b = 2"
> +    }
> +
> +    # Setup for test 2
> +    clean_restart ${testfile}
> +
> +    if { ![runto_main] } {
> +	return
> +    }
> +
> +    with_test_prefix "step-test" {
> +	gdb_test_no_output "record" "turn on process record"
> +
> +	# Test 2, reverse-step command
> +	# Set breakpoint at the line after the function calls.
> +	gdb_breakpoint $bp_start_reverse_test temporary
> +
> +	# Continue to the start of the reverse-step test.
> +	# Command definition:  reverse-step [count]
> +	#   Run the program backward until control reaches the start of a
> +	#   different source line; then stop it, and return control to gdb.
> +	#   Like the step command, reverse-step will only stop at the
> +	#   beginning of a source line.  It “un-executes” the previously
> +	#   executed source line.  If the previous source line included calls
> +	#   to debuggable functions, reverse-step will step (backward) into
> +	#   the called function, stopping at the beginning of the last
> +	#   statement in the called function (typically a return statement).
> +	#   Also, as with the step command, if non-debuggable functions are
> +	#   called, reverse-step will run thru them backward without
> +	#   stopping.
> +
> +	gdb_continue_to_breakpoint \
> +	    "stopped at command reverse-step test start location" \
> +	    ".*$bp_start_reverse_test\r\n.*"
> +
> +	# The first reverse step should take us call of func2 ().
> +	gdb_test "reverse-step" ".*END FUNC2.*" \
> +	    "reverse-step into func2 "
> +
> +	# The second reverse step should take us into func1 ().
> +	gdb_test "reverse-step" ".*END FUNC1.*" \
> +	    "reverse-step into func1 "
> +
> +	# The third reverse step should take us call of func1 ().
> +	gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	    "reverse-step to line func1(); func2(), at call for func1 "
> +	# We should be stopped at the first instruction of the line.  A
> +	# reversee stepi should take us to b = 2 ().
> +	gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	    "reverse-stepi to line b = 2 "
> +    }
> +}
> +
> +standard_testfile  .c
> +
> +# test with and without gcc column info enabled
> +foreach_with_prefix column_info_flag {column-info no-column-info} {
> +    set options [list debug $column_info_flag]
> +
> +    if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +	     $options]} {
> +	return -1
> +   }
> +
> +    run_tests
> +}

I haven't followed the discussions around this bug, but it looks like something that was
already there, and not a regression I suppose.

In any case, I've tested this on aarch64-linux and arm-linux. Works as expected and the
code looks OK to me.

Tested-By: Luis Machado <luis.machado@arm.com>
Reviewed-By: Luis Machado <luis.machado@arm.com>