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

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

[Bruno Larsen]

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 v4:
    * more style fixes, thanks Pedro.
    * Reworked part of the test, as it was not detecting failures
      correctly.

Changelog for v3:
    * fix some comments on the amd64_epilogue_unwinder
    * fix style for the step-reverse test

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                              |  10 +-
 gdb/infrun.c                                  |   2 +-
 .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
 gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
 .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++
 gdb/testsuite/gdb.reverse/step-precsave.exp   |   6 +-
 gdb/testsuite/gdb.reverse/step-reverse.c      |  19 ++-
 gdb/testsuite/gdb.reverse/step-reverse.exp    |  55 ++++++-
 8 files changed, 280 insertions(+), 13 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp

-- 
2.37.3

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

[Bruno Larsen]

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.

It also adds a test to confirm that the frame_id will be the same,
regardless of using the epilogue unwinder or not, thanks to Andrew
Burgess.

Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/amd64-tdep.c                              |  10 +-
 .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
 gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
 .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++
 4 files changed, 206 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index ea2b3b1ecc3..a2cd6bbb106 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2937,18 +2937,18 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
 
   try
     {
-      /* Cache base will be %esp plus cache->sp_offset (-8).  */
+      /* Cache base will be %rsp plus cache->sp_offset (-8).  */
       get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
       cache->base = extract_unsigned_integer (buf, 8,
 					      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.  */
+      /* The previous value of %rsp is cache->base plus 16.  */
       cache->saved_sp = cache->base + 16;
 
-      /* The saved %eip will be at cache->base plus 8.  */
+      /* The saved %rip will be at cache->base plus 8.  */
       cache->saved_regs[AMD64_RIP_REGNUM] = cache->base + 8;
 
       cache->base_p = 1;
@@ -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->base + 16, cache->pc);
 }
 
 static const struct frame_unwind amd64_epilogue_frame_unwind =
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" ".*"
+    }
+}
-- 
2.37.3

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

[Bruno Larsen]

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    | 19 ++++++-
 gdb/testsuite/gdb.reverse/step-reverse.exp  | 55 +++++++++++++++++++--
 4 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1957e8020dd..ba0a9ceba63 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..3279b6ce879 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..809c7d16dc9 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.c
+++ b/gdb/testsuite/gdb.reverse/step-reverse.c
@@ -26,6 +26,20 @@ 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 +65,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 +77,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..c28e1f6db4f 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
@@ -243,10 +244,56 @@ gdb_test_multiple "step" "$test_message" {
     }
 }
 
-# next backward over call
+# Next backward over call.
 
 gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
 
+set step_out 0
+gdb_test_multiple "next" "reverse next over recursion" {
+    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
+	pass "$gdb_test_name"
+    }
+    -re -wrap ".*RECURSIVE CALL.*" {
+	fail "$gdb_test_name"
+	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 "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 seen_recursive_call 0
+gdb_test_multiple "next" "step over recursion inside the recursion" {
+    -re -wrap ".*RECURSIVE CALL.*" {
+	incr seen_recursive_call
+	send_gdb "next\n"
+	exp_continue
+    }
+    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
+	gdb_assert {"$seen_recursive_call" == 1} \
+	    "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.3

[PING][PATCH v4 0/2] Fix reverse nexting over recursions

[Bruno Larsen]

ping!

Cheers,
Bruno

On 05/10/2022 12:38, Bruno Larsen wrote:
> 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 v4:
>      * more style fixes, thanks Pedro.
>      * Reworked part of the test, as it was not detecting failures
>        correctly.
>
> Changelog for v3:
>      * fix some comments on the amd64_epilogue_unwinder
>      * fix style for the step-reverse test
>
> 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                              |  10 +-
>   gdb/infrun.c                                  |   2 +-
>   .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>   gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>   .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++
>   gdb/testsuite/gdb.reverse/step-precsave.exp   |   6 +-
>   gdb/testsuite/gdb.reverse/step-reverse.c      |  19 ++-
>   gdb/testsuite/gdb.reverse/step-reverse.exp    |  55 ++++++-
>   8 files changed, 280 insertions(+), 13 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp
>

Re: [PING][PATCH v4 0/2] Fix reverse nexting over recursions

[Tom Tromey]

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

Bruno> ping!

Thank you.  These look ok to me.

Tom

Re: [PING][PATCH v4 0/2] Fix reverse nexting over recursions

[Bruno Larsen]

On 20/10/2022 20:56, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> Bruno> ping!
>
> Thank you.  These look ok to me.
Thanks! pushed
>
> Tom
>

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

[Simon Marchi]

On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
> 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.
> 
> It also adds a test to confirm that the frame_id will be the same,
> regardless of using the epilogue unwinder or not, thanks to Andrew
> Burgess.
> 
> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
> ---
>  gdb/amd64-tdep.c                              |  10 +-
>  .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>  gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>  .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++
>  4 files changed, 206 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>  create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>  create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp

Hi Bruno,

On Ubuntu 22.04, I can get this new test to fail quite reliably with:

    $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"

Can you give it a try?

Simon

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

[Bruno Larsen]

On 25/10/2022 15:44, Simon Marchi wrote:
> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>> 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.
>>
>> It also adds a test to confirm that the frame_id will be the same,
>> regardless of using the epilogue unwinder or not, thanks to Andrew
>> Burgess.
>>
>> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
>> ---
>>   gdb/amd64-tdep.c                              |  10 +-
>>   .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>>   gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>>   .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++
>>   4 files changed, 206 insertions(+), 5 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> Hi Bruno,
>
> On Ubuntu 22.04, I can get this new test to fail quite reliably with:
>
>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>
> Can you give it a try?

Hi Simon,

I'll take a look, thanks for bringing this to my attention!

Cheers,
Bruno

>
> Simon
>

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

[Simon Marchi]

On 10/25/22 09:44, Simon Marchi wrote:
> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>> 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.
>>
>> It also adds a test to confirm that the frame_id will be the same,
>> regardless of using the epilogue unwinder or not, thanks to Andrew
>> Burgess.
>>
>> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
>> ---
>>  gdb/amd64-tdep.c                              |  10 +-
>>  .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>>  gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>>  .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++/usr/lib/x86_64-linux-gnu/libasan.so.6
>>  4 files changed, 206 insertions(+), 5 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>>  create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>>  create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> 
> Hi Bruno,
> 
> On Ubuntu 22.04, I can get this new test to fail quite reliably with:
> 
>     $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
> 
> Can you give it a try?
> 
> Simon

Actually, I took the time to look into it, it turns out the problem is
simple.  Here's a patch below.


From 7090bf701b2f1cca89985ea1b45b0a2e3859e19e Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Tue, 25 Oct 2022 09:50:56 -0400
Subject: [PATCH] gdb/testsuite: make sure to consume the prompt in
 gdb.base/unwind-on-each-insn.exp

This test fails quite reliably for me when ran as:

    $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"

or more simply:

    $ make check-read1 TESTS="gdb.base/unwind-on-each-insn.exp"

The problem is that the that grabs the frame id from "maint print
frame-id" does not consume the prompt.  Well, it does sometimes due to
the trailing .*, but not always.  If the prompt is not consumed, the
following tests get confused:

    FAIL: gdb.base/unwind-on-each-insn.exp: gdb_breakpoint: set breakpoint at *foo
    FAIL: gdb.base/unwind-on-each-insn.exp: disassemble foo
    FAIL: gdb.base/unwind-on-each-insn.exp: get $sp and frame base in foo: get hexadecimal valueof "$sp"

Use -wrap to make gdb_test_multiple consume the prompt.

While at it, remove the bit that consumes the command name and do
exp_continue, it's not really necessary.  And for consistency, do the
same changes to the gdb_test_multiple that consumes the stack address,
although that one was fine, it did consume the prompt explicitly.

Change-Id: I2b7328c8844c7e98921ea494c4c05107162619fc
---
 gdb/testsuite/gdb.base/unwind-on-each-insn.exp | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
index faa6a1a3f064..3b48805cff83 100644
--- a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
+++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
@@ -44,11 +44,7 @@ proc get_sp_and_fba { testname } {

 	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 $" {
+	    -re -wrap ".*Stack level ${::decimal}, frame at ($::hex):.*" {
 		set fba $expect_out(1,string)
 	    }
 	}
@@ -62,11 +58,7 @@ proc get_sp_and_fba { testname } {
 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\]+).*" {
+	-re -wrap ".*frame-id for frame #${::decimal}: (.*)" {
 	    set fid $expect_out(1,string)
 	}
     }
-- 
2.38.0

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

[Bruno Larsen]

On 25/10/2022 15:59, Simon Marchi wrote:
> On 10/25/22 09:44, Simon Marchi wrote:
>> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>>> 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.
>>>
>>> It also adds a test to confirm that the frame_id will be the same,
>>> regardless of using the epilogue unwinder or not, thanks to Andrew
>>> Burgess.
>>>
>>> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
>>> ---
>>>   gdb/amd64-tdep.c                              |  10 +-
>>>   .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>>>   gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>>>   .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++/usr/lib/x86_64-linux-gnu/libasan.so.6
>>>   4 files changed, 206 insertions(+), 5 deletions(-)
>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp
>> Hi Bruno,
>>
>> On Ubuntu 22.04, I can get this new test to fail quite reliably with:
>>
>>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>
>> Can you give it a try?
>>
>> Simon
> Actually, I took the time to look into it, it turns out the problem is
> simple.  Here's a patch below.
Ah great! I didn't even have time to finish making an Ubuntu VM lol. I 
just noticed one typo:
>
>
>  From 7090bf701b2f1cca89985ea1b45b0a2e3859e19e Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Tue, 25 Oct 2022 09:50:56 -0400
> Subject: [PATCH] gdb/testsuite: make sure to consume the prompt in
>   gdb.base/unwind-on-each-insn.exp
>
> This test fails quite reliably for me when ran as:
>
>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>
> or more simply:
>
>      $ make check-read1 TESTS="gdb.base/unwind-on-each-insn.exp"
>
> The problem is that the that grabs the frame id from "maint print

Missing word here. "The problem is that the __proc__" ?


With this fixed, LGTM.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Cheers,
Bruno


> frame-id" does not consume the prompt.  Well, it does sometimes due to
> the trailing .*, but not always.  If the prompt is not consumed, the
> following tests get confused:
>
>      FAIL: gdb.base/unwind-on-each-insn.exp: gdb_breakpoint: set breakpoint at *foo
>      FAIL: gdb.base/unwind-on-each-insn.exp: disassemble foo
>      FAIL: gdb.base/unwind-on-each-insn.exp: get $sp and frame base in foo: get hexadecimal valueof "$sp"
>
> Use -wrap to make gdb_test_multiple consume the prompt.
>
> While at it, remove the bit that consumes the command name and do
> exp_continue, it's not really necessary.  And for consistency, do the
> same changes to the gdb_test_multiple that consumes the stack address,
> although that one was fine, it did consume the prompt explicitly.
>
> Change-Id: I2b7328c8844c7e98921ea494c4c05107162619fc
> ---
>   gdb/testsuite/gdb.base/unwind-on-each-insn.exp | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> index faa6a1a3f064..3b48805cff83 100644
> --- a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> @@ -44,11 +44,7 @@ proc get_sp_and_fba { testname } {
>
>   	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 $" {
> +	    -re -wrap ".*Stack level ${::decimal}, frame at ($::hex):.*" {
>   		set fba $expect_out(1,string)
>   	    }
>   	}
> @@ -62,11 +58,7 @@ proc get_sp_and_fba { testname } {
>   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\]+).*" {
> +	-re -wrap ".*frame-id for frame #${::decimal}: (.*)" {
>   	    set fid $expect_out(1,string)
>   	}
>       }

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

[Simon Marchi]

On 10/25/22 10:13, Bruno Larsen via Gdb-patches wrote:
> On 25/10/2022 15:59, Simon Marchi wrote:
>> On 10/25/22 09:44, Simon Marchi wrote:
>>> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>>>> 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.
>>>>
>>>> It also adds a test to confirm that the frame_id will be the same,
>>>> regardless of using the epilogue unwinder or not, thanks to Andrew
>>>> Burgess.
>>>>
>>>> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
>>>> ---
>>>>   gdb/amd64-tdep.c                              |  10 +-
>>>>   .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>>>>   gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>>>>   .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++/usr/lib/x86_64-linux-gnu/libasan.so.6
>>>>   4 files changed, 206 insertions(+), 5 deletions(-)
>>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp
>>> Hi Bruno,
>>>
>>> On Ubuntu 22.04, I can get this new test to fail quite reliably with:
>>>
>>>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>>
>>> Can you give it a try?
>>>
>>> Simon
>> Actually, I took the time to look into it, it turns out the problem is
>> simple.  Here's a patch below.
> Ah great! I didn't even have time to finish making an Ubuntu VM lol. I just noticed one typo:

99% of the time, when needing to test on a specific version of a
specific distro, it is sufficient to use Docker (or whatever)
containers.  It's much, much faster than installing a VM from scratch.

>>  From 7090bf701b2f1cca89985ea1b45b0a2e3859e19e Mon Sep 17 00:00:00 2001
>> From: Simon Marchi <simon.marchi@efficios.com>
>> Date: Tue, 25 Oct 2022 09:50:56 -0400
>> Subject: [PATCH] gdb/testsuite: make sure to consume the prompt in
>>   gdb.base/unwind-on-each-insn.exp
>>
>> This test fails quite reliably for me when ran as:
>>
>>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>
>> or more simply:
>>
>>      $ make check-read1 TESTS="gdb.base/unwind-on-each-insn.exp"
>>
>> The problem is that the that grabs the frame id from "maint print
> 
> Missing word here. "The problem is that the __proc__" ?

I think I meant "the gdb_test_multiple", thanks for pointing it out.

> With this fixed, LGTM.
> 
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Thanks, will push.

Simon

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

[Simon Marchi]

On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
> 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    | 19 ++++++-
>  gdb/testsuite/gdb.reverse/step-reverse.exp  | 55 +++++++++++++++++++--
>  4 files changed, 74 insertions(+), 8 deletions(-)

Hi Bruno,

I see these failures since this commit:

    $ make check TESTS="gdb.reverse/step-reverse.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
    FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again
    FAIL: gdb.reverse/step-reverse.exp: enter recursive function
    FAIL: gdb.reverse/step-reverse.exp: step over recursion inside the recursion

Simon

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

[Tom de Vries]

On 10/25/22 16:55, Simon Marchi via Gdb-patches wrote:
> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>> 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    | 19 ++++++-
>>   gdb/testsuite/gdb.reverse/step-reverse.exp  | 55 +++++++++++++++++++--
>>   4 files changed, 74 insertions(+), 8 deletions(-)
> 
> Hi Bruno,
> 
> I see these failures since this commit:
> 
>      $ make check TESTS="gdb.reverse/step-reverse.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>      FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again
>      FAIL: gdb.reverse/step-reverse.exp: enter recursive function
>      FAIL: gdb.reverse/step-reverse.exp: step over recursion inside the recursion
> 

On aarch64, I ran into another regression due to this commit (test-case 
gdb.reverse/solib-precsave.exp), filed as 
https://sourceware.org/bugzilla/show_bug.cgi?id=29721 .

Thanks,
- Tom

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

[Bruno Larsen]

On 25/10/2022 16:55, Simon Marchi wrote:
> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>> 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    | 19 ++++++-
>>   gdb/testsuite/gdb.reverse/step-reverse.exp  | 55 +++++++++++++++++++--
>>   4 files changed, 74 insertions(+), 8 deletions(-)
> Hi Bruno,
>
> I see these failures since this commit:
>
>      $ make check TESTS="gdb.reverse/step-reverse.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>      FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again
>      FAIL: gdb.reverse/step-reverse.exp: enter recursive function
>      FAIL: gdb.reverse/step-reverse.exp: step over recursion inside the recursion
Hi Simon,

Sorry about the delay in responding to this, took me a while to track it 
down.

This was actually a latent bug, not a regression. If you try to run 
gdb.reverse/step-reverse in the native-gdbserver setup manually, then 
run the following commands:

(gdb) tbreak main
(gdb) record
(gdb) until 61
(gdb) rn 2
(gdb) n

You'll see the same failures. After the final next we should be stopped 
at line 58, and we end up on line 61 instead.

This happens because when recording the instruction call, GDB doesn't 
seem to save the stack change, only registers, so when reconstructing 
the state we aren't aware that the return address changes. This makes it 
so when setting the breakpoint at the return address, we set it to the 
return of the second callee return, instead of the first callee in this 
case (or recursive_callee in the case you found).

I hope this explanation made sense...

Cheers,
Bruno

>
> Simon
>

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

[Simon Marchi]

On 11/2/22 13:03, Bruno Larsen via Gdb-patches wrote:
> On 25/10/2022 16:55, Simon Marchi wrote:
>> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>>> 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    | 19 ++++++-
>>>   gdb/testsuite/gdb.reverse/step-reverse.exp  | 55 +++++++++++++++++++--
>>>   4 files changed, 74 insertions(+), 8 deletions(-)
>> Hi Bruno,
>>
>> I see these failures since this commit:
>>
>>      $ make check TESTS="gdb.reverse/step-reverse.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>      FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again
>>      FAIL: gdb.reverse/step-reverse.exp: enter recursive function
>>      FAIL: gdb.reverse/step-reverse.exp: step over recursion inside the recursion
> Hi Simon,
> 
> Sorry about the delay in responding to this, took me a while to track it down.
> 
> This was actually a latent bug, not a regression. If you try to run gdb.reverse/step-reverse in the native-gdbserver setup manually, then run the following commands:
> 
> (gdb) tbreak main
> (gdb) record
> (gdb) until 61
> (gdb) rn 2
> (gdb) n
> 
> You'll see the same failures. After the final next we should be stopped at line 58, and we end up on line 61 instead.
> 
> This happens because when recording the instruction call, GDB doesn't seem to save the stack change, only registers, so when reconstructing the state we aren't aware that the return address changes. This makes it so when setting the breakpoint at the return address, we set it to the return of the second callee return, instead of the first callee in this case (or recursive_callee in the case you found).
> 
> I hope this explanation made sense...

I don't know the reverse stuff well, but the explanation makes sense.
Do you plan on tackling this bug?  If not, can you file a bug and add a
kfail?

Thanks,

Simon

[PATCH] gdb/testsuite: add KFAILs to gdb.reverse/step-reverse.exp

[Bruno Larsen]

> I don't know the reverse stuff well, but the explanation makes sense.
> Do you plan on tackling this bug?  If not, can you file a bug and add a
> kfail?

Sure, I do plan on tackling this at some point, but I don't know when
that will be, so I filed the bug, and this is the patch to add the
KFAILs, thoughts?

---

Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
PR record/29745, where we can't skip one funcion forward if we're using
native-gdbserver. This commit just adds kfails to the test.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745
---
 gdb/testsuite/gdb.reverse/step-reverse.exp | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index c28e1f6db4f..37e80a7d337 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -31,6 +31,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
 }
 
 runto_main
+set using_gdbserver [target_is_gdbserver]
 
 if [supports_process_record] {
     # Activate process record/replay
@@ -273,11 +274,25 @@ if { "$step_out" == 1 } {
 # 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"
+if {$using_gdbserver} {
+    # gdbserver doesn't record the change of return pointer, so we can't
+    # next forward over functions.
+    setup_kfail gdb/29745 *-*-*
+}
 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"
 
+if {$using_gdbserver} {
+    # Because of the above mentioned KFAIL, the inferior is now out of sync
+    setup_kfail gdb/29745 *-*-*
+}
 gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
 set seen_recursive_call 0
+if {$using_gdbserver} {
+    # Because of the above mentioned KFAIL, the inferior is now out of sync
+    # The fail state below will resync the inferior.
+    setup_kfail gdb/29745 *-*-*
+}
 gdb_test_multiple "next" "step over recursion inside the recursion" {
     -re -wrap ".*RECURSIVE CALL.*" {
 	incr seen_recursive_call
-- 
2.37.3

Re: [PATCH] gdb/testsuite: add KFAILs to gdb.reverse/step-reverse.exp

[Simon Marchi]

On 11/3/22 05:08, Bruno Larsen wrote:
>> I don't know the reverse stuff well, but the explanation makes sense.
>> Do you plan on tackling this bug?  If not, can you file a bug and add a
>> kfail?
> 
> Sure, I do plan on tackling this at some point, but I don't know when
> that will be, so I filed the bug, and this is the patch to add the
> KFAILs, thoughts?
> 
> ---
> 
> Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
> PR record/29745, where we can't skip one funcion forward if we're using
> native-gdbserver. This commit just adds kfails to the test.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745
> ---
>  gdb/testsuite/gdb.reverse/step-reverse.exp | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index c28e1f6db4f..37e80a7d337 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -31,6 +31,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>  }
>  
>  runto_main
> +set using_gdbserver [target_is_gdbserver]
>  
>  if [supports_process_record] {
>      # Activate process record/replay
> @@ -273,11 +274,25 @@ if { "$step_out" == 1 } {
>  # 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"
> +if {$using_gdbserver} {
> +    # gdbserver doesn't record the change of return pointer, so we can't
> +    # next forward over functions.
> +    setup_kfail gdb/29745 *-*-*

There's one thing bugging me in your explanation: as far as I know,
gdbserver does any recording, with the built-in GDB recorder (i.e. not
btrace).  So we probably shouldn't say "gdbserver doesn't record", as
it's not meant to record in the first place.  That would mean the
problem is within GDB, when using the remote target.  And the check for
the kfail should therefore use gdb_is_target_remote instead of
target_is_gdbserver.

Simon

[PATCHv2] gdb/testsuite: add KFAILs to gdb.reverse/step-reverse.exp

[Bruno Larsen]

On 03/11/2022 14:06, Simon Marchi wrote:
>
> On 11/3/22 05:08, Bruno Larsen wrote:
>>> I don't know the reverse stuff well, but the explanation makes sense.
>>> Do you plan on tackling this bug?  If not, can you file a bug and add a
>>> kfail?
>> Sure, I do plan on tackling this at some point, but I don't know when
>> that will be, so I filed the bug, and this is the patch to add the
>> KFAILs, thoughts?
>>
>> ---
>>
>> Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
>> PR record/29745, where we can't skip one funcion forward if we're using
>> native-gdbserver. This commit just adds kfails to the test.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745
>> ---
>>   gdb/testsuite/gdb.reverse/step-reverse.exp | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>> index c28e1f6db4f..37e80a7d337 100644
>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>> @@ -31,6 +31,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>>   }
>>   
>>   runto_main
>> +set using_gdbserver [target_is_gdbserver]
>>   
>>   if [supports_process_record] {
>>       # Activate process record/replay
>> @@ -273,11 +274,25 @@ if { "$step_out" == 1 } {
>>   # 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"
>> +if {$using_gdbserver} {
>> +    # gdbserver doesn't record the change of return pointer, so we can't
>> +    # next forward over functions.
>> +    setup_kfail gdb/29745 *-*-*
> There's one thing bugging me in your explanation: as far as I know,
> gdbserver does any recording, with the built-in GDB recorder (i.e. not
> btrace).  So we probably shouldn't say "gdbserver doesn't record", as
> it's not meant to record in the first place.  That would mean the
> problem is within GDB, when using the remote target.  And the check for
> the kfail should therefore use gdb_is_target_remote instead of
> target_is_gdbserver.

That makes sense. This is my first time working with gdbserver, so 
everything here is news to me.  Updated version:

---

     Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
     PR record/29745, where we can't skip one funcion forward if we're using
     native-gdbserver. This commit just adds kfails to the test.

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

diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp 
b/gdb/testsuite/gdb.reverse/step-reverse.exp
index c28e1f6db4f..d2975cffb5c 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -31,6 +31,7 @@ if { [prepare_for_testing "failed to prepare" 
$testfile $srcfile] } {
  }

  runto_main
+set target_remote [gdb_is_target_remote]

  if [supports_process_record] {
      # Activate process record/replay
@@ -273,11 +274,25 @@ if { "$step_out" == 1 } {
  # 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"
+if {$target_remote} {
+    # gdb doesn't record the change of return pointer for remote targets,
+    # so we can't next forward over functions.
+    setup_kfail gdb/29745 *-*-*
+}
  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"

+if {$target_remote} {
+    # Because of the above mentioned KFAIL, the inferior is now out of sync
+    setup_kfail gdb/29745 *-*-*
+}
  gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
  set seen_recursive_call 0
+if {$target_remote} {
+    # Because of the above mentioned KFAIL, the inferior is now out of sync
+    # The fail state below will resync the inferior.
+    setup_kfail gdb/29745 *-*-*
+}
  gdb_test_multiple "next" "step over recursion inside the recursion" {
      -re -wrap ".*RECURSIVE CALL.*" {
         incr seen_recursive_call

Re: [PATCHv2] gdb/testsuite: add KFAILs to gdb.reverse/step-reverse.exp

[Simon Marchi]

On 11/3/22 10:30, Bruno Larsen wrote:
> On 03/11/2022 14:06, Simon Marchi wrote:
>>
>> On 11/3/22 05:08, Bruno Larsen wrote:
>>>> I don't know the reverse stuff well, but the explanation makes sense.
>>>> Do you plan on tackling this bug?  If not, can you file a bug and add a
>>>> kfail?
>>> Sure, I do plan on tackling this at some point, but I don't know when
>>> that will be, so I filed the bug, and this is the patch to add the
>>> KFAILs, thoughts?
>>>
>>> ---
>>>
>>> Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
>>> PR record/29745, where we can't skip one funcion forward if we're using
>>> native-gdbserver. This commit just adds kfails to the test.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745
>>> ---
>>>   gdb/testsuite/gdb.reverse/step-reverse.exp | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>> index c28e1f6db4f..37e80a7d337 100644
>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>> @@ -31,6 +31,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>>>   }
>>>     runto_main
>>> +set using_gdbserver [target_is_gdbserver]
>>>     if [supports_process_record] {
>>>       # Activate process record/replay
>>> @@ -273,11 +274,25 @@ if { "$step_out" == 1 } {
>>>   # 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"
>>> +if {$using_gdbserver} {
>>> +    # gdbserver doesn't record the change of return pointer, so we can't
>>> +    # next forward over functions.
>>> +    setup_kfail gdb/29745 *-*-*
>> There's one thing bugging me in your explanation: as far as I know,
>> gdbserver does any recording, with the built-in GDB recorder (i.e. not
>> btrace).  So we probably shouldn't say "gdbserver doesn't record", as
>> it's not meant to record in the first place.  That would mean the
>> problem is within GDB, when using the remote target.  And the check for
>> the kfail should therefore use gdb_is_target_remote instead of
>> target_is_gdbserver.
> 
> That makes sense. This is my first time working with gdbserver, so everything here is news to me.  Updated version:
> 
> ---
> 
>     Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
>     PR record/29745, where we can't skip one funcion forward if we're using
>     native-gdbserver. This commit just adds kfails to the test.
> 
>     Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745

Thanks:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCHv2] gdb/testsuite: add KFAILs to gdb.reverse/step-reverse.exp

[Bruno Larsen]

On 03/11/2022 17:59, Simon Marchi wrote:
>
> On 11/3/22 10:30, Bruno Larsen wrote:
>> On 03/11/2022 14:06, Simon Marchi wrote:
>>> On 11/3/22 05:08, Bruno Larsen wrote:
>>>>> I don't know the reverse stuff well, but the explanation makes sense.
>>>>> Do you plan on tackling this bug?  If not, can you file a bug and add a
>>>>> kfail?
>>>> Sure, I do plan on tackling this at some point, but I don't know when
>>>> that will be, so I filed the bug, and this is the patch to add the
>>>> KFAILs, thoughts?
>>>>
>>>> ---
>>>>
>>>> Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
>>>> PR record/29745, where we can't skip one funcion forward if we're using
>>>> native-gdbserver. This commit just adds kfails to the test.
>>>>
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745
>>>> ---
>>>>    gdb/testsuite/gdb.reverse/step-reverse.exp | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>> index c28e1f6db4f..37e80a7d337 100644
>>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>> @@ -31,6 +31,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>>>>    }
>>>>      runto_main
>>>> +set using_gdbserver [target_is_gdbserver]
>>>>      if [supports_process_record] {
>>>>        # Activate process record/replay
>>>> @@ -273,11 +274,25 @@ if { "$step_out" == 1 } {
>>>>    # 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"
>>>> +if {$using_gdbserver} {
>>>> +    # gdbserver doesn't record the change of return pointer, so we can't
>>>> +    # next forward over functions.
>>>> +    setup_kfail gdb/29745 *-*-*
>>> There's one thing bugging me in your explanation: as far as I know,
>>> gdbserver does any recording, with the built-in GDB recorder (i.e. not
>>> btrace).  So we probably shouldn't say "gdbserver doesn't record", as
>>> it's not meant to record in the first place.  That would mean the
>>> problem is within GDB, when using the remote target.  And the check for
>>> the kfail should therefore use gdb_is_target_remote instead of
>>> target_is_gdbserver.
>> That makes sense. This is my first time working with gdbserver, so everything here is news to me.  Updated version:
>>
>> ---
>>
>>      Recent changes to gdb.reverse/step-reverse.exp revealed the latent bug
>>      PR record/29745, where we can't skip one funcion forward if we're using
>>      native-gdbserver. This commit just adds kfails to the test.
>>
>>      Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29745
> Thanks:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

thanks, pushed!

Cheers,
Bruno

>
> Simon
>