[PATCH v2 0/2] Fix reverse nexting over recursions

[PATCH v2 0/2] Fix reverse nexting over recursions

[Bruno Larsen via Gdb-patches]

This patch series fixes gdb record/16678, GDB being unable to
reverse-next over a recursive function.  However, the simple way to fix
it hit a snag when I discovered that the amd64 epilogue unwinder would
give a different frame id than the dwarf2 unwinder would in the rest of
the function.  This patch series first change this discrepancy, then
fixes the bug.

Changelog for v2:
    * Implemented Pedro Alves's suggestion to simplify the fix
    * Added the first patch to fix a regression that the simple fix
      would introduce.

Bruno Larsen (2):
  Change calculation of frame_id by amd64 epilogue unwinder
  gdb/reverse: Fix stepping over recursive functions

 gdb/amd64-tdep.c                            |  4 +-
 gdb/infrun.c                                |  2 +-
 gdb/testsuite/gdb.reverse/step-precsave.exp |  6 ++-
 gdb/testsuite/gdb.reverse/step-reverse.c    | 16 +++++-
 gdb/testsuite/gdb.reverse/step-reverse.exp  | 59 +++++++++++++++++++--
 5 files changed, 78 insertions(+), 9 deletions(-)

-- 
2.37.2

[PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Bruno Larsen via Gdb-patches]

When GDB is stopped at a ret instruction and no debug information is
available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
be able to generate a decent backtrace. However, when calculating the
frame id, the epilogue unwinder generates information as if the return
instruction was the whole frame.

This was an issue especially when attempting to reverse debug, as GDB
would place a step_resume_breakpoint from the epilogue of a function if
we were to attempt to skip that function, and this breakpoint should
ideally have the current function's frame_id to avoid other problems
such as PR record/16678.

This commit changes the frame_id calculation for the amd64 epilogue,
so that it is always the same as the dwarf2 unwinder's frame_id.
---
 gdb/amd64-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index d89e06d27cb..17c82ac919c 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2943,7 +2943,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
 					      byte_order) + cache->sp_offset;
 
       /* Cache pc will be the frame func.  */
-      cache->pc = get_frame_pc (this_frame);
+      cache->pc = get_frame_func (this_frame);
 
       /* The saved %esp will be at cache->base plus 16.  */
       cache->saved_sp = cache->base + 16;
@@ -2986,7 +2986,7 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
   if (!cache->base_p)
     (*this_id) = frame_id_build_unavailable_stack (cache->pc);
   else
-    (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+    (*this_id) = frame_id_build (cache->saved_sp, cache->pc);
 }
 
 static const struct frame_unwind amd64_epilogue_frame_unwind =
-- 
2.37.2

[PATCH v2 2/2] gdb/reverse: Fix stepping over recursive functions

[Bruno Larsen via Gdb-patches]

Currently, when using GDB to do reverse debugging, if we try to use the
command "reverse next" to skip a recursive function, instead of skipping
all of the recursive calls and stopping in the previous line, we stop at
the second to last recursive call, and need to manually step backwards
until we leave the first call.  This is well documented in PR gdb/16678.

This bug happens because when GDB notices that a reverse step has
entered into a function, GDB will add a step_resume_breakpoint at the
start of the function, then single step out of the prologue once that
breakpoint is hit.  The problem was happening because GDB wouldn't give
that step_resume_breakpoint a frame-id, so the first time the breakpoint
was hit, the inferior would be stopped.  This is fixed by giving the
current frame-id to the breakpoint.

This commit also changes gdb.reverse/step-reverse.c to contain a
recursive function and attempt to both, skip it altogether, and to skip
the second call from inside the first call, as this setup broke a
previous version of the patch.
---
 gdb/infrun.c                                |  2 +-
 gdb/testsuite/gdb.reverse/step-precsave.exp |  6 ++-
 gdb/testsuite/gdb.reverse/step-reverse.c    | 16 +++++-
 gdb/testsuite/gdb.reverse/step-reverse.exp  | 59 +++++++++++++++++++--
 4 files changed, 76 insertions(+), 7 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 033699bc3f7..679a0c83ece 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7133,7 +7133,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 		  sr_sal.pc = ecs->stop_func_start;
 		  sr_sal.pspace = get_frame_program_space (frame);
 		  insert_step_resume_breakpoint_at_sal (gdbarch,
-							sr_sal, null_frame_id);
+							sr_sal, get_stack_frame_id (frame));
 		}
 	    }
 	  else
diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp
index 0836ed2629f..54df9209c96 100644
--- a/gdb/testsuite/gdb.reverse/step-precsave.exp
+++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
@@ -86,7 +86,8 @@ gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
 
 # step over call
 
-gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
+gdb_test "step" ".*NEXT OVER THIS RECURSION.*" "step up to call"
+gdb_test "next" ".*NEXT OVER THIS CALL.*" "skip recursive call"
 gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
 
 # step into call
@@ -280,9 +281,10 @@ gdb_test_multiple "step" "$test_message" {
     }
 }
 
-# next backward over call
+# next backward over calls
 
 gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
+gdb_test "next" ".*NEXT OVER THIS RECURSION.*" "reverse next over recursive call"
 
 # step/next backward with count
 
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
index aea2a98541d..a390ac2580c 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.c
+++ b/gdb/testsuite/gdb.reverse/step-reverse.c
@@ -26,6 +26,17 @@ int callee() {		/* ENTER CALLEE */
   return myglob++;	/* ARRIVED IN CALLEE */
 }			/* RETURN FROM CALLEE */
 
+/* We need to make this function take more than a single instruction
+   to run, otherwise it could hide PR gdb/16678, as reverse execution can
+   step over a single-instruction function.  */
+int recursive_callee (int val){
+    if (val == 0) return 0;
+    val /= 2;
+    if (val>1)
+	val++;
+    return recursive_callee (val);	/* RECURSIVE CALL */
+} /* EXIT RECURSIVE FUNCTION */
+
 /* A structure which, we hope, will need to be passed using memcpy.  */
 struct rhomboidal {
   int rather_large[100];
@@ -51,6 +62,9 @@ int main () {
    y = y + 4;
    z = z + 5;	/* STEP TEST 2 */
 
+   /* Test that next goes over recursive calls too */
+   recursive_callee (32); /* NEXT OVER THIS RECURSION */
+
    /* Test that "next" goes over a call */
    callee();	/* NEXT OVER THIS CALL */
 
@@ -60,7 +74,7 @@ int main () {
    /* Test "stepi" */
    a[5] = a[3] - a[4]; /* FINISH TEST */
    callee();	/* STEPI TEST */
-   
+
    /* Test "nexti" */
    callee();	/* NEXTI TEST */
 
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index 997b62604d5..4f56b4785ca 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
 gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
 gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
 
+# next through a recursive function call
+gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
+
 # step over call
 
-gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
 gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
 
 # step into call
@@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
 
 set test_message "stepi back from function call"
 gdb_test_multiple "stepi" "$test_message" {
-    -re "NEXTI TEST.*$gdb_prompt $" {
+    -re -wrap "NEXTI TEST.*" {
 	pass "$test_message"
     }
     -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
@@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
 ###
 
 # Set reverse execution direction
-
 gdb_test_no_output "set exec-dir reverse" "set reverse execution"
 
 # stepi backward thru return and into a function
@@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
 
 gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
 
+# Dont reverse the execution direction yet, as we will need another
+# forward step after this test
+
+set step_out 0
+gdb_test_multiple "next" "reverse next over recursion" {
+    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
+	pass "reverse next over recursion"
+    }
+    -re -wrap ".*RECURSIVE CALL.*" {
+	fail "reverse next over recursion"
+	set step_out 1
+    }
+}
+if { "$step_out" == 1 } {
+    gdb_test_multiple "next" "stepping out of recursion" {
+	-re -wrap "NEXT OVER THIS RECURSION.*" {
+	    set step_out 0
+	}
+	-re -wrap ".*" {
+	    send_gdb "reverse-next\n"
+	    exp_continue
+	}
+    }
+}
+
+# Step forward over recursion again so we can test stepping over calls
+# inside the recursion itself.
+gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
+gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
+gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
+
+gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
+set step_pass 1
+gdb_test_multiple "next" "step over recursion inside the recursion" {
+    -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
+	set step_pass 0
+	send_gdb "next\n"
+	exp_continue
+    }
+    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
+	if {$step_pass} {
+	    pass "step over recursion inside the recursion"
+	} else {
+	    fail "step over recursion inside the recursion"
+	}
+    }
+    -re -wrap ".*" {
+	send_gdb "next\n"
+	exp_continue
+    }
+}
+
 # step/next backward with count
 
 gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
-- 
2.37.2

Re: [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Andrew Burgess via Gdb-patches]

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> When GDB is stopped at a ret instruction and no debug information is
> available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
> be able to generate a decent backtrace. However, when calculating the
> frame id, the epilogue unwinder generates information as if the return
> instruction was the whole frame.
>
> This was an issue especially when attempting to reverse debug, as GDB
> would place a step_resume_breakpoint from the epilogue of a function if
> we were to attempt to skip that function, and this breakpoint should
> ideally have the current function's frame_id to avoid other problems
> such as PR record/16678.
>
> This commit changes the frame_id calculation for the amd64 epilogue,
> so that it is always the same as the dwarf2 unwinder's frame_id.
> ---
>  gdb/amd64-tdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index d89e06d27cb..17c82ac919c 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2943,7 +2943,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
>  					      byte_order) + cache->sp_offset;
>  
>        /* Cache pc will be the frame func.  */
> -      cache->pc = get_frame_pc (this_frame);
> +      cache->pc = get_frame_func (this_frame);
>  
>        /* The saved %esp will be at cache->base plus 16.  */
>        cache->saved_sp = cache->base + 16;

This change is fine, though I wonder if you would mind updating the
comments in this function.  These comments have clearly been copied over
from the i386 code, and still refer to %esp instead of %rsp, etc.

Additionally, this comment:

  /* The saved %esp will be at cache->base plus 16.  */

I believe is completely wrong.  The previous %rsp value is not _at_
anything, instead:

  /* The previous value of %rsp is cache->base plus 16.  */

This change of wording aligns with similar wording in
amd64_frame_cache_1 where the saved_sp is calculated.

> @@ -2986,7 +2986,7 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
>    if (!cache->base_p)
>      (*this_id) = frame_id_build_unavailable_stack (cache->pc);
>    else
> -    (*this_id) = frame_id_build (cache->base + 8, cache->pc);
> +    (*this_id) = frame_id_build (cache->saved_sp, cache->pc);
>  }

I honestly don't understand the logic behind how cache->base and
cache->sp_offset are used for the x86-64 prologue based unwinders.

The sp_offset starts out as -8, which makes sense, the call instruction
will have pushed an 8-byte address to the stack.

Personally, at this point, I would think that:

  cache->base = current_sp - cache->sp_offset;

would be the correct way to calculate cache->base.  This would set the
cache->base to be the frame base address.  However, we actually
calculate the cache->base as:

  cache->base = current_sp + cache->sp_offset;

notice the '+' instead of the '-' I proposed.  As a consequence, the
frame base address ends up being:

  cache->base + 16

which is exactly what we use in amd64_frame_this_id and
amd64_sigtramp_frame_this_id.

Maybe I'm missing some super subtle logic here for why we do things the
way that we do.  But honestly, my guess is that, at some point, a logic
bug was introduced (using '+' instead of '-'), and the rest of the code
has just grown to work around that problem, e.g. the '+ 16' when
calculating the frame base address.

Now, in your change, you propose using saved_sp instead of '+ 8'.

I'm thinking that we would be better off just using 'cache->base + 16'
here instead.  My reasoning would be:

  1. A '+ 16' here is inline with the other frame_id function on x86-64,
  which is probably a good idea, and

  2. Though we can see that the frame base address _is_ the same as the
  previous stack pointer value, they don't have to be.  For me at least,
  these two things are different concepts, and just because the actual
  value is the same, doesn't mean we should use the two variables
  interchangably.  So, I'd prefer to see cache->base used like we do be
  the other frame_id functions.  Who knows, one day maybe we could even
  change things so the '+ 16' is not needed, and cache->base will
  contain the "correct" value ... but not today I think.

Finally, on this topic, I took a look at i386_epilogue_frame_this_id to
see if the same bug was present there, and I notice that code uses '+ 8'
just like the amd64 code does, but in that case addresses are only
4-bytes, and indeed, the initial sp_offset is -4, so '+ 8' here (i386)
is really '+ 2 * sizeof(void*)', which means there's no i386 bug, but I
think this another indication that using '+16' in the amd64 code is the
way to go.

Finally, I wondered if it is worth adding a test that covers
specifically this functionality, without relying on the gdb.reverse test
you mention - as that test need futher fixes before it can exercise this
code.  I've included a test at the end of this email which I believe
covers this functionality, if you agree this would be worthwhile, then
feel free to include this test, or to use any parts of this test that
are helpful in writing your own test.

Thanks,
Andrew

--

diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
new file mode 100644
index 00000000000..e5e5ebfd013
--- /dev/null
+++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 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/>.  */
+
+void
+foo ()
+{
+  /* Nothing.  */
+}
diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.c b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
new file mode 100644
index 00000000000..3e4cc2675f2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 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/>.  */
+
+extern void foo ();
+
+int
+main ()
+{
+  foo ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
new file mode 100644
index 00000000000..46bea800c1b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
@@ -0,0 +1,154 @@
+# Copyright 2022 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/>.  */
+
+# Single step through a simple (empty) function that was compiled
+# without DWARF debug information.
+#
+# At each instruction check that the frame-id, and frame base address,
+# are calculated correctly.
+#
+# Additionally, check we can correctly unwind to the previous frame,
+# and that the previous stack-pointer value, and frame base address
+# value, can be calculated correctly.
+
+standard_testfile .c -foo.c
+
+if {[prepare_for_testing_full "failed to prepare" \
+	 [list ${testfile} debug \
+	      $srcfile {debug} $srcfile2 {nodebug}]]} {
+    return -1
+}
+
+if ![runto_main] then {
+    return 0
+}
+
+# Return a two element list, the first element is the stack-pointer
+# value (from the $sp register), and the second element is the frame
+# base address (from the 'info frame' output).
+proc get_sp_and_fba { testname } {
+    with_test_prefix "get \$sp and frame base $testname" {
+	set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
+
+	set fba ""
+	gdb_test_multiple "info frame" "" {
+	    -re "^info frame\r\n" {
+		exp_continue
+	    }
+
+	    -re "^Stack level ${::decimal}, frame at ($::hex):\r\n.*$::gdb_prompt $" {
+		set fba $expect_out(1,string)
+	    }
+	}
+
+	return [list $sp $fba]
+    }
+}
+
+# Return the frame-id of the current frame, collected using the 'maint
+# print frame-id' command.
+proc get_fid { } {
+    set fid ""
+    gdb_test_multiple "maint print frame-id" "" {
+	-re "^maint print frame-id\r\n" {
+	    exp_continue
+	}
+
+	-re "^frame-id for frame #${::decimal}: (\[^\r\n\]+).*" {
+	    set fid $expect_out(1,string)
+	}
+    }
+    return $fid
+}
+
+# Record the current stack-pointer, and the frame base address.
+lassign [get_sp_and_fba "in main"] main_sp main_fba
+set main_fid [get_fid]
+
+# Now enter the foo function.
+gdb_breakpoint "*foo"
+gdb_continue_to_breakpoint "enter foo"
+
+# Figure out the range of addresses covered by this function.
+set last_addr_in_foo ""
+gdb_test_multiple "disassemble foo" "" {
+    -re "^disassemble foo\r\n" {
+	exp_continue
+    }
+
+    -re "^Dump of assembler code for function foo:\r\n" {
+	exp_continue
+    }
+
+    -re "^...($hex) \[^\r\n\]+\r\n" {
+	set last_addr_in_foo $expect_out(1,string)
+	exp_continue
+    }
+
+    -wrap -re "^End of assembler dump\\." {
+	gdb_assert { ![string equal $last_addr_in_foo ""] } \
+	    "found some addresses in foo"
+	pass $gdb_test_name
+    }
+}
+
+# Record the current stack-pointer, and the frame base address.
+lassign [get_sp_and_fba "in foo"] foo_sp foo_fba
+set foo_fid [get_fid]
+
+for { set i_count 1 } { true } { incr i_count } {
+    with_test_prefix "instruction ${i_count}" {
+
+	# The current stack-pointer value can legitimately change
+	# throughout the lifetime of a function, so we don't check the
+	# current stack-pointer value.  But the frame base address
+	# should not change, so we do check for that.
+	lassign [get_sp_and_fba "for foo"] sp_value fba_value
+	gdb_assert { $fba_value == $foo_fba }
+
+	# The frame-id should never change within a function, so check
+	# that now.
+	set fid [get_fid]
+	gdb_assert { [string equal $fid $foo_fid] } \
+	    "check frame-id matches"
+
+	# Check that the previous frame is 'main'.
+	gdb_test "bt 2" "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
+
+	# Move up the stack (to main).
+	gdb_test "up" \
+	    "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
+
+	# Check we can unwind the stack-pointer and the frame base
+	# address correctly.
+	lassign [get_sp_and_fba "for main"] sp_value fba_value
+	gdb_assert { $sp_value == $main_sp }
+	gdb_assert { $fba_value == $main_fba }
+
+	# Check we have a consistent value for main's frame-id.
+	set fid [get_fid]
+	gdb_assert { [string equal $fid $main_fid] }
+
+	# Move back to the inner most frame.
+	gdb_test "frame 0" ".*"
+
+	set pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*"]
+	if { $pc == $last_addr_in_foo } {
+	    break
+	}
+
+	gdb_test "stepi" ".*"
+    }
+}

Re: [PATCH v2 2/2] gdb/reverse: Fix stepping over recursive functions

[Andrew Burgess via Gdb-patches]

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> Currently, when using GDB to do reverse debugging, if we try to use the
> command "reverse next" to skip a recursive function, instead of skipping
> all of the recursive calls and stopping in the previous line, we stop at
> the second to last recursive call, and need to manually step backwards
> until we leave the first call.  This is well documented in PR gdb/16678.
>
> This bug happens because when GDB notices that a reverse step has
> entered into a function, GDB will add a step_resume_breakpoint at the
> start of the function, then single step out of the prologue once that
> breakpoint is hit.  The problem was happening because GDB wouldn't give
> that step_resume_breakpoint a frame-id, so the first time the breakpoint
> was hit, the inferior would be stopped.  This is fixed by giving the
> current frame-id to the breakpoint.
>
> This commit also changes gdb.reverse/step-reverse.c to contain a
> recursive function and attempt to both, skip it altogether, and to skip
> the second call from inside the first call, as this setup broke a
> previous version of the patch.

Thanks for this.  The GDB fix looks fine.  I had a couple of really
minor style nits relating to the test, see inline below.

> ---
>  gdb/infrun.c                                |  2 +-
>  gdb/testsuite/gdb.reverse/step-precsave.exp |  6 ++-
>  gdb/testsuite/gdb.reverse/step-reverse.c    | 16 +++++-
>  gdb/testsuite/gdb.reverse/step-reverse.exp  | 59 +++++++++++++++++++--
>  4 files changed, 76 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 033699bc3f7..679a0c83ece 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7133,7 +7133,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  		  sr_sal.pc = ecs->stop_func_start;
>  		  sr_sal.pspace = get_frame_program_space (frame);
>  		  insert_step_resume_breakpoint_at_sal (gdbarch,
> -							sr_sal, null_frame_id);
> +							sr_sal, get_stack_frame_id (frame));
>  		}
>  	    }
>  	  else
> diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp
> index 0836ed2629f..54df9209c96 100644
> --- a/gdb/testsuite/gdb.reverse/step-precsave.exp
> +++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
> @@ -86,7 +86,8 @@ gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>  
>  # step over call
>  
> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
> +gdb_test "step" ".*NEXT OVER THIS RECURSION.*" "step up to call"
> +gdb_test "next" ".*NEXT OVER THIS CALL.*" "skip recursive call"
>  gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>  
>  # step into call
> @@ -280,9 +281,10 @@ gdb_test_multiple "step" "$test_message" {
>      }
>  }
>  
> -# next backward over call
> +# next backward over calls
>  
>  gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
> +gdb_test "next" ".*NEXT OVER THIS RECURSION.*" "reverse next over recursive call"
>  
>  # step/next backward with count
>  
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
> index aea2a98541d..a390ac2580c 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -26,6 +26,17 @@ int callee() {		/* ENTER CALLEE */
>    return myglob++;	/* ARRIVED IN CALLEE */
>  }			/* RETURN FROM CALLEE */
>  
> +/* We need to make this function take more than a single instruction
> +   to run, otherwise it could hide PR gdb/16678, as reverse execution can
> +   step over a single-instruction function.  */
> +int recursive_callee (int val){
> +    if (val == 0) return 0;
> +    val /= 2;
> +    if (val>1)

Unless required by the test, this code should follow GNU coding
standard, so

  int
  recursive_callee (int val)
  {

and

  if (val > 1)

If the non-standard layout is required, then the comment above the
function should explain why.

> +	val++;
> +    return recursive_callee (val);	/* RECURSIVE CALL */
> +} /* EXIT RECURSIVE FUNCTION */
> +
>  /* A structure which, we hope, will need to be passed using memcpy.  */
>  struct rhomboidal {
>    int rather_large[100];
> @@ -51,6 +62,9 @@ int main () {
>     y = y + 4;
>     z = z + 5;	/* STEP TEST 2 */
>  
> +   /* Test that next goes over recursive calls too */
> +   recursive_callee (32); /* NEXT OVER THIS RECURSION */
> +
>     /* Test that "next" goes over a call */
>     callee();	/* NEXT OVER THIS CALL */
>  
> @@ -60,7 +74,7 @@ int main () {
>     /* Test "stepi" */
>     a[5] = a[3] - a[4]; /* FINISH TEST */
>     callee();	/* STEPI TEST */
> -   
> +
>     /* Test "nexti" */
>     callee();	/* NEXTI TEST */
>  
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index 997b62604d5..4f56b4785ca 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>  gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>  gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>  
> +# next through a recursive function call

Comments should be proper sentences, so capital 'N', and end with '.'.

> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
> +
>  # step over call
>  
> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>  gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>  
>  # step into call
> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>  
>  set test_message "stepi back from function call"
>  gdb_test_multiple "stepi" "$test_message" {
> -    -re "NEXTI TEST.*$gdb_prompt $" {
> +    -re -wrap "NEXTI TEST.*" {
>  	pass "$test_message"
>      }
>      -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>  ###
>  
>  # Set reverse execution direction
> -
>  gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>  
>  # stepi backward thru return and into a function
> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>  
>  gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>  
> +# Dont reverse the execution direction yet, as we will need another
> +# forward step after this test

Missing '.'.

> +
> +set step_out 0
> +gdb_test_multiple "next" "reverse next over recursion" {
> +    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> +	pass "reverse next over recursion"
> +    }
> +    -re -wrap ".*RECURSIVE CALL.*" {
> +	fail "reverse next over recursion"
> +	set step_out 1
> +    }
> +}
> +if { "$step_out" == 1 } {
> +    gdb_test_multiple "next" "stepping out of recursion" {
> +	-re -wrap "NEXT OVER THIS RECURSION.*" {
> +	    set step_out 0
> +	}
> +	-re -wrap ".*" {
> +	    send_gdb "reverse-next\n"
> +	    exp_continue
> +	}
> +    }
> +}
> +
> +# Step forward over recursion again so we can test stepping over calls
> +# inside the recursion itself.
> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
> +
> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
> +set step_pass 1
> +gdb_test_multiple "next" "step over recursion inside the recursion" {
> +    -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
> +	set step_pass 0
> +	send_gdb "next\n"
> +	exp_continue
> +    }
> +    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> +	if {$step_pass} {
> +	    pass "step over recursion inside the recursion"
> +	} else {
> +	    fail "step over recursion inside the recursion"

You can replace the strings used with pass/fail with the magic variable
$gdb_test_name.  This variable is magically set within the code blocks
of a gdb_test_multiple to the name of the test.

Thanks,
Andrew

> +	}
> +    }
> +    -re -wrap ".*" {
> +	send_gdb "next\n"
> +	exp_continue
> +    }
> +}
> +
>  # step/next backward with count
>  
>  gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
> -- 
> 2.37.2

Re: [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Andrew Burgess via Gdb-patches]

Andrew Burgess <aburgess@redhat.com> writes:

> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> When GDB is stopped at a ret instruction and no debug information is
>> available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
>> be able to generate a decent backtrace. However, when calculating the
>> frame id, the epilogue unwinder generates information as if the return
>> instruction was the whole frame.
>>
>> This was an issue especially when attempting to reverse debug, as GDB
>> would place a step_resume_breakpoint from the epilogue of a function if
>> we were to attempt to skip that function, and this breakpoint should
>> ideally have the current function's frame_id to avoid other problems
>> such as PR record/16678.
>>
>> This commit changes the frame_id calculation for the amd64 epilogue,
>> so that it is always the same as the dwarf2 unwinder's frame_id.
>> ---
>>  gdb/amd64-tdep.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index d89e06d27cb..17c82ac919c 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -2943,7 +2943,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
>>  					      byte_order) + cache->sp_offset;
>>  
>>        /* Cache pc will be the frame func.  */
>> -      cache->pc = get_frame_pc (this_frame);
>> +      cache->pc = get_frame_func (this_frame);
>>  
>>        /* The saved %esp will be at cache->base plus 16.  */
>>        cache->saved_sp = cache->base + 16;
>
> This change is fine, though I wonder if you would mind updating the
> comments in this function.  These comments have clearly been copied over
> from the i386 code, and still refer to %esp instead of %rsp, etc.
>
> Additionally, this comment:
>
>   /* The saved %esp will be at cache->base plus 16.  */
>
> I believe is completely wrong.  The previous %rsp value is not _at_
> anything, instead:
>
>   /* The previous value of %rsp is cache->base plus 16.  */
>
> This change of wording aligns with similar wording in
> amd64_frame_cache_1 where the saved_sp is calculated.
>
>> @@ -2986,7 +2986,7 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
>>    if (!cache->base_p)
>>      (*this_id) = frame_id_build_unavailable_stack (cache->pc);
>>    else
>> -    (*this_id) = frame_id_build (cache->base + 8, cache->pc);
>> +    (*this_id) = frame_id_build (cache->saved_sp, cache->pc);
>>  }
>
> I honestly don't understand the logic behind how cache->base and
> cache->sp_offset are used for the x86-64 prologue based unwinders.
>
> The sp_offset starts out as -8, which makes sense, the call instruction
> will have pushed an 8-byte address to the stack.
>
> Personally, at this point, I would think that:
>
>   cache->base = current_sp - cache->sp_offset;
>
> would be the correct way to calculate cache->base.  This would set the
> cache->base to be the frame base address.  However, we actually
> calculate the cache->base as:
>
>   cache->base = current_sp + cache->sp_offset;
>
> notice the '+' instead of the '-' I proposed.  As a consequence, the
> frame base address ends up being:
>
>   cache->base + 16
>
> which is exactly what we use in amd64_frame_this_id and
> amd64_sigtramp_frame_this_id.
>
> Maybe I'm missing some super subtle logic here for why we do things the
> way that we do.  But honestly, my guess is that, at some point, a logic
> bug was introduced (using '+' instead of '-'), and the rest of the code
> has just grown to work around that problem, e.g. the '+ 16' when
> calculating the frame base address.
>
> Now, in your change, you propose using saved_sp instead of '+ 8'.
>
> I'm thinking that we would be better off just using 'cache->base + 16'
> here instead.  My reasoning would be:
>
>   1. A '+ 16' here is inline with the other frame_id function on x86-64,
>   which is probably a good idea, and
>
>   2. Though we can see that the frame base address _is_ the same as the
>   previous stack pointer value, they don't have to be.  For me at least,
>   these two things are different concepts, and just because the actual
>   value is the same, doesn't mean we should use the two variables
>   interchangably.  So, I'd prefer to see cache->base used like we do be
>   the other frame_id functions.  Who knows, one day maybe we could even
>   change things so the '+ 16' is not needed, and cache->base will
>   contain the "correct" value ... but not today I think.
>
> Finally, on this topic, I took a look at i386_epilogue_frame_this_id to
> see if the same bug was present there, and I notice that code uses '+ 8'
> just like the amd64 code does, but in that case addresses are only
> 4-bytes, and indeed, the initial sp_offset is -4, so '+ 8' here (i386)
> is really '+ 2 * sizeof(void*)', which means there's no i386 bug, but I
> think this another indication that using '+16' in the amd64 code is the
> way to go.
>
> Finally, I wondered if it is worth adding a test that covers
> specifically this functionality, without relying on the gdb.reverse test
> you mention - as that test need futher fixes before it can exercise this
> code.  I've included a test at the end of this email which I believe
> covers this functionality, if you agree this would be worthwhile, then
> feel free to include this test, or to use any parts of this test that
> are helpful in writing your own test.
>
> Thanks,
> Andrew
>
> --
>
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
> new file mode 100644
> index 00000000000..e5e5ebfd013
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +void
> +foo ()
> +{
> +  /* Nothing.  */
> +}
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.c b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
> new file mode 100644
> index 00000000000..3e4cc2675f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +extern void foo ();
> +
> +int
> +main ()
> +{
> +  foo ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> new file mode 100644
> index 00000000000..46bea800c1b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> @@ -0,0 +1,154 @@
> +# Copyright 2022 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/>.  */
> +
> +# Single step through a simple (empty) function that was compiled
> +# without DWARF debug information.
> +#
> +# At each instruction check that the frame-id, and frame base address,
> +# are calculated correctly.
> +#
> +# Additionally, check we can correctly unwind to the previous frame,
> +# and that the previous stack-pointer value, and frame base address
> +# value, can be calculated correctly.
> +
> +standard_testfile .c -foo.c
> +
> +if {[prepare_for_testing_full "failed to prepare" \
> +	 [list ${testfile} debug \
> +	      $srcfile {debug} $srcfile2 {nodebug}]]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +# Return a two element list, the first element is the stack-pointer
> +# value (from the $sp register), and the second element is the frame
> +# base address (from the 'info frame' output).
> +proc get_sp_and_fba { testname } {
> +    with_test_prefix "get \$sp and frame base $testname" {
> +	set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
> +
> +	set fba ""
> +	gdb_test_multiple "info frame" "" {
> +	    -re "^info frame\r\n" {
> +		exp_continue
> +	    }
> +
> +	    -re "^Stack level ${::decimal}, frame at ($::hex):\r\n.*$::gdb_prompt $" {
> +		set fba $expect_out(1,string)
> +	    }
> +	}
> +
> +	return [list $sp $fba]
> +    }
> +}
> +
> +# Return the frame-id of the current frame, collected using the 'maint
> +# print frame-id' command.
> +proc get_fid { } {
> +    set fid ""
> +    gdb_test_multiple "maint print frame-id" "" {
> +	-re "^maint print frame-id\r\n" {
> +	    exp_continue
> +	}
> +
> +	-re "^frame-id for frame #${::decimal}: (\[^\r\n\]+).*" {
> +	    set fid $expect_out(1,string)
> +	}
> +    }
> +    return $fid
> +}

I remembered that this test relies on my 'maint print frame-id' patch.
I think I'll go and merge that now.

Thanks,
Andrew


> +
> +# Record the current stack-pointer, and the frame base address.
> +lassign [get_sp_and_fba "in main"] main_sp main_fba
> +set main_fid [get_fid]
> +
> +# Now enter the foo function.
> +gdb_breakpoint "*foo"
> +gdb_continue_to_breakpoint "enter foo"
> +
> +# Figure out the range of addresses covered by this function.
> +set last_addr_in_foo ""
> +gdb_test_multiple "disassemble foo" "" {
> +    -re "^disassemble foo\r\n" {
> +	exp_continue
> +    }
> +
> +    -re "^Dump of assembler code for function foo:\r\n" {
> +	exp_continue
> +    }
> +
> +    -re "^...($hex) \[^\r\n\]+\r\n" {
> +	set last_addr_in_foo $expect_out(1,string)
> +	exp_continue
> +    }
> +
> +    -wrap -re "^End of assembler dump\\." {
> +	gdb_assert { ![string equal $last_addr_in_foo ""] } \
> +	    "found some addresses in foo"
> +	pass $gdb_test_name
> +    }
> +}
> +
> +# Record the current stack-pointer, and the frame base address.
> +lassign [get_sp_and_fba "in foo"] foo_sp foo_fba
> +set foo_fid [get_fid]
> +
> +for { set i_count 1 } { true } { incr i_count } {
> +    with_test_prefix "instruction ${i_count}" {
> +
> +	# The current stack-pointer value can legitimately change
> +	# throughout the lifetime of a function, so we don't check the
> +	# current stack-pointer value.  But the frame base address
> +	# should not change, so we do check for that.
> +	lassign [get_sp_and_fba "for foo"] sp_value fba_value
> +	gdb_assert { $fba_value == $foo_fba }
> +
> +	# The frame-id should never change within a function, so check
> +	# that now.
> +	set fid [get_fid]
> +	gdb_assert { [string equal $fid $foo_fid] } \
> +	    "check frame-id matches"
> +
> +	# Check that the previous frame is 'main'.
> +	gdb_test "bt 2" "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
> +
> +	# Move up the stack (to main).
> +	gdb_test "up" \
> +	    "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
> +
> +	# Check we can unwind the stack-pointer and the frame base
> +	# address correctly.
> +	lassign [get_sp_and_fba "for main"] sp_value fba_value
> +	gdb_assert { $sp_value == $main_sp }
> +	gdb_assert { $fba_value == $main_fba }
> +
> +	# Check we have a consistent value for main's frame-id.
> +	set fid [get_fid]
> +	gdb_assert { [string equal $fid $main_fid] }
> +
> +	# Move back to the inner most frame.
> +	gdb_test "frame 0" ".*"
> +
> +	set pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*"]
> +	if { $pc == $last_addr_in_foo } {
> +	    break
> +	}
> +
> +	gdb_test "stepi" ".*"
> +    }
> +}

Re: [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Andrew Burgess via Gdb-patches]

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> When GDB is stopped at a ret instruction and no debug information is
> available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
> be able to generate a decent backtrace. However, when calculating the
> frame id, the epilogue unwinder generates information as if the return
> instruction was the whole frame.
>
> This was an issue especially when attempting to reverse debug, as GDB
> would place a step_resume_breakpoint from the epilogue of a function if
> we were to attempt to skip that function, and this breakpoint should
> ideally have the current function's frame_id to avoid other problems
> such as PR record/16678.
>
> This commit changes the frame_id calculation for the amd64 epilogue,
> so that it is always the same as the dwarf2 unwinder's frame_id.
> ---
>  gdb/amd64-tdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index d89e06d27cb..17c82ac919c 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2943,7 +2943,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
>  					      byte_order) + cache->sp_offset;
>  
>        /* Cache pc will be the frame func.  */
> -      cache->pc = get_frame_pc (this_frame);
> +      cache->pc = get_frame_func (this_frame);
>  

Not for fixing in this patch, but I notice that
amd64_sigtramp_frame_this_id also uses get_frame_pc, and as a
consequence, has a non-stable frame-id.

It doesn't cause many problems because the sigtramp frame is pretty
small, and I guess folk don't usually debug in that frame.

But maybe worth fixing at some point...

Thanks,
Andrew

>        /* The saved %esp will be at cache->base plus 16.  */
>        cache->saved_sp = cache->base + 16;
> @@ -2986,7 +2986,7 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
>    if (!cache->base_p)
>      (*this_id) = frame_id_build_unavailable_stack (cache->pc);
>    else
> -    (*this_id) = frame_id_build (cache->base + 8, cache->pc);
> +    (*this_id) = frame_id_build (cache->saved_sp, cache->pc);
>  }
>  
>  static const struct frame_unwind amd64_epilogue_frame_unwind =
> -- 
> 2.37.2

Re: [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Bruno Larsen via Gdb-patches]

On 24/08/2022 15:11, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> When GDB is stopped at a ret instruction and no debug information is
>> available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
>> be able to generate a decent backtrace. However, when calculating the
>> frame id, the epilogue unwinder generates information as if the return
>> instruction was the whole frame.
>>
>> This was an issue especially when attempting to reverse debug, as GDB
>> would place a step_resume_breakpoint from the epilogue of a function if
>> we were to attempt to skip that function, and this breakpoint should
>> ideally have the current function's frame_id to avoid other problems
>> such as PR record/16678.
>>
>> This commit changes the frame_id calculation for the amd64 epilogue,
>> so that it is always the same as the dwarf2 unwinder's frame_id.
>> ---
>>   gdb/amd64-tdep.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index d89e06d27cb..17c82ac919c 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -2943,7 +2943,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
>>   					      byte_order) + cache->sp_offset;
>>   
>>         /* Cache pc will be the frame func.  */
>> -      cache->pc = get_frame_pc (this_frame);
>> +      cache->pc = get_frame_func (this_frame);
>>   
>>         /* The saved %esp will be at cache->base plus 16.  */
>>         cache->saved_sp = cache->base + 16;
> This change is fine, though I wonder if you would mind updating the
> comments in this function.  These comments have clearly been copied over
> from the i386 code, and still refer to %esp instead of %rsp, etc.
>
> Additionally, this comment:
>
>    /* The saved %esp will be at cache->base plus 16.  */
>
> I believe is completely wrong.  The previous %rsp value is not _at_
> anything, instead:
>
>    /* The previous value of %rsp is cache->base plus 16.  */
>
> This change of wording aligns with similar wording in
> amd64_frame_cache_1 where the saved_sp is calculated.
Sure!
>
>> @@ -2986,7 +2986,7 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
>>     if (!cache->base_p)
>>       (*this_id) = frame_id_build_unavailable_stack (cache->pc);
>>     else
>> -    (*this_id) = frame_id_build (cache->base + 8, cache->pc);
>> +    (*this_id) = frame_id_build (cache->saved_sp, cache->pc);
>>   }
> I honestly don't understand the logic behind how cache->base and
> cache->sp_offset are used for the x86-64 prologue based unwinders.
>
> The sp_offset starts out as -8, which makes sense, the call instruction
> will have pushed an 8-byte address to the stack.
>
> Personally, at this point, I would think that:
>
>    cache->base = current_sp - cache->sp_offset;
>
> would be the correct way to calculate cache->base.  This would set the
> cache->base to be the frame base address.  However, we actually
> calculate the cache->base as:
>
>    cache->base = current_sp + cache->sp_offset;
>
> notice the '+' instead of the '-' I proposed.  As a consequence, the
> frame base address ends up being:
>
>    cache->base + 16
>
> which is exactly what we use in amd64_frame_this_id and
> amd64_sigtramp_frame_this_id.
>
> Maybe I'm missing some super subtle logic here for why we do things the
> way that we do.  But honestly, my guess is that, at some point, a logic
> bug was introduced (using '+' instead of '-'), and the rest of the code
> has just grown to work around that problem, e.g. the '+ 16' when
> calculating the frame base address.

As I mentioned off-list, I think the cache->base is being used like this 
because of the register %rbp, which is called the "base pointer", and it 
would have the value that is being saved on cache->base.

I'm not sure if this is the best way to go about this, though. Keeping 
it this way, x86_64 programmers would be used to this language, but 
means that other architecture's programmers would have this exact 
confusion, so I'm divided.

>
> Now, in your change, you propose using saved_sp instead of '+ 8'.
>
> I'm thinking that we would be better off just using 'cache->base + 16'
> here instead.  My reasoning would be:
>
>    1. A '+ 16' here is inline with the other frame_id function on x86-64,
>    which is probably a good idea, and
>
>    2. Though we can see that the frame base address _is_ the same as the
>    previous stack pointer value, they don't have to be.  For me at least,
>    these two things are different concepts, and just because the actual
>    value is the same, doesn't mean we should use the two variables
>    interchangably.  So, I'd prefer to see cache->base used like we do be
>    the other frame_id functions.  Who knows, one day maybe we could even
>    change things so the '+ 16' is not needed, and cache->base will
>    contain the "correct" value ... but not today I think.
I'm fine with this.
>
> Finally, on this topic, I took a look at i386_epilogue_frame_this_id to
> see if the same bug was present there, and I notice that code uses '+ 8'
> just like the amd64 code does, but in that case addresses are only
> 4-bytes, and indeed, the initial sp_offset is -4, so '+ 8' here (i386)
> is really '+ 2 * sizeof(void*)', which means there's no i386 bug, but I
> think this another indication that using '+16' in the amd64 code is the
> way to go.
This  cements it even harder that they are using rbp (and ebp on i386) 
on purpose on my mind, but if you want to standardize it across 
architectures, I'm ok too.
>
> Finally, I wondered if it is worth adding a test that covers
> specifically this functionality, without relying on the gdb.reverse test
> you mention - as that test need futher fixes before it can exercise this
> code.  I've included a test at the end of this email which I believe
> covers this functionality, if you agree this would be worthwhile, then
> feel free to include this test, or to use any parts of this test that
> are helpful in writing your own test.
I'll just add this test as a third patch for my next patch version, 
thanks for this!

-- 
Cheers,
Bruno

>
> Thanks,
> Andrew
>
> --
>
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
> new file mode 100644
> index 00000000000..e5e5ebfd013
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +void
> +foo ()
> +{
> +  /* Nothing.  */
> +}
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.c b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
> new file mode 100644
> index 00000000000..3e4cc2675f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +extern void foo ();
> +
> +int
> +main ()
> +{
> +  foo ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> new file mode 100644
> index 00000000000..46bea800c1b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> @@ -0,0 +1,154 @@
> +# Copyright 2022 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/>.  */
> +
> +# Single step through a simple (empty) function that was compiled
> +# without DWARF debug information.
> +#
> +# At each instruction check that the frame-id, and frame base address,
> +# are calculated correctly.
> +#
> +# Additionally, check we can correctly unwind to the previous frame,
> +# and that the previous stack-pointer value, and frame base address
> +# value, can be calculated correctly.
> +
> +standard_testfile .c -foo.c
> +
> +if {[prepare_for_testing_full "failed to prepare" \
> +	 [list ${testfile} debug \
> +	      $srcfile {debug} $srcfile2 {nodebug}]]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +# Return a two element list, the first element is the stack-pointer
> +# value (from the $sp register), and the second element is the frame
> +# base address (from the 'info frame' output).
> +proc get_sp_and_fba { testname } {
> +    with_test_prefix "get \$sp and frame base $testname" {
> +	set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
> +
> +	set fba ""
> +	gdb_test_multiple "info frame" "" {
> +	    -re "^info frame\r\n" {
> +		exp_continue
> +	    }
> +
> +	    -re "^Stack level ${::decimal}, frame at ($::hex):\r\n.*$::gdb_prompt $" {
> +		set fba $expect_out(1,string)
> +	    }
> +	}
> +
> +	return [list $sp $fba]
> +    }
> +}
> +
> +# Return the frame-id of the current frame, collected using the 'maint
> +# print frame-id' command.
> +proc get_fid { } {
> +    set fid ""
> +    gdb_test_multiple "maint print frame-id" "" {
> +	-re "^maint print frame-id\r\n" {
> +	    exp_continue
> +	}
> +
> +	-re "^frame-id for frame #${::decimal}: (\[^\r\n\]+).*" {
> +	    set fid $expect_out(1,string)
> +	}
> +    }
> +    return $fid
> +}
> +
> +# Record the current stack-pointer, and the frame base address.
> +lassign [get_sp_and_fba "in main"] main_sp main_fba
> +set main_fid [get_fid]
> +
> +# Now enter the foo function.
> +gdb_breakpoint "*foo"
> +gdb_continue_to_breakpoint "enter foo"
> +
> +# Figure out the range of addresses covered by this function.
> +set last_addr_in_foo ""
> +gdb_test_multiple "disassemble foo" "" {
> +    -re "^disassemble foo\r\n" {
> +	exp_continue
> +    }
> +
> +    -re "^Dump of assembler code for function foo:\r\n" {
> +	exp_continue
> +    }
> +
> +    -re "^...($hex) \[^\r\n\]+\r\n" {
> +	set last_addr_in_foo $expect_out(1,string)
> +	exp_continue
> +    }
> +
> +    -wrap -re "^End of assembler dump\\." {
> +	gdb_assert { ![string equal $last_addr_in_foo ""] } \
> +	    "found some addresses in foo"
> +	pass $gdb_test_name
> +    }
> +}
> +
> +# Record the current stack-pointer, and the frame base address.
> +lassign [get_sp_and_fba "in foo"] foo_sp foo_fba
> +set foo_fid [get_fid]
> +
> +for { set i_count 1 } { true } { incr i_count } {
> +    with_test_prefix "instruction ${i_count}" {
> +
> +	# The current stack-pointer value can legitimately change
> +	# throughout the lifetime of a function, so we don't check the
> +	# current stack-pointer value.  But the frame base address
> +	# should not change, so we do check for that.
> +	lassign [get_sp_and_fba "for foo"] sp_value fba_value
> +	gdb_assert { $fba_value == $foo_fba }
> +
> +	# The frame-id should never change within a function, so check
> +	# that now.
> +	set fid [get_fid]
> +	gdb_assert { [string equal $fid $foo_fid] } \
> +	    "check frame-id matches"
> +
> +	# Check that the previous frame is 'main'.
> +	gdb_test "bt 2" "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
> +
> +	# Move up the stack (to main).
> +	gdb_test "up" \
> +	    "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
> +
> +	# Check we can unwind the stack-pointer and the frame base
> +	# address correctly.
> +	lassign [get_sp_and_fba "for main"] sp_value fba_value
> +	gdb_assert { $sp_value == $main_sp }
> +	gdb_assert { $fba_value == $main_fba }
> +
> +	# Check we have a consistent value for main's frame-id.
> +	set fid [get_fid]
> +	gdb_assert { [string equal $fid $main_fid] }
> +
> +	# Move back to the inner most frame.
> +	gdb_test "frame 0" ".*"
> +
> +	set pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*"]
> +	if { $pc == $last_addr_in_foo } {
> +	    break
> +	}
> +
> +	gdb_test "stepi" ".*"
> +    }
> +}