public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Fix reverse nexting over recursions
@ 2022-08-31 12:07 Bruno Larsen
  2022-08-31 12:07 ` [PATCH v3 1/2] Change calculation of frame_id by amd64 epilogue unwinder Bruno Larsen
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Bruno Larsen @ 2022-08-31 12:07 UTC (permalink / raw)
  To: gdb-patches; +Cc: pedro, aburgess, 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 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      |  18 +-
 gdb/testsuite/gdb.reverse/step-reverse.exp    |  58 ++++++-
 8 files changed, 282 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.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 1/2] Change calculation of frame_id by amd64 epilogue unwinder
  2022-08-31 12:07 [PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
@ 2022-08-31 12:07 ` Bruno Larsen
  2022-08-31 12:07 ` [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
  2022-09-14 13:16 ` [Ping][PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
  2 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2022-08-31 12:07 UTC (permalink / raw)
  To: gdb-patches; +Cc: pedro, aburgess, 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 d89e06d27cb..078cf3ce4c0 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.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions
  2022-08-31 12:07 [PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
  2022-08-31 12:07 ` [PATCH v3 1/2] Change calculation of frame_id by amd64 epilogue unwinder Bruno Larsen
@ 2022-08-31 12:07 ` Bruno Larsen
  2022-09-30 18:28   ` Pedro Alves
  2022-09-14 13:16 ` [Ping][PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
  2 siblings, 1 reply; 8+ messages in thread
From: Bruno Larsen @ 2022-08-31 12:07 UTC (permalink / raw)
  To: gdb-patches; +Cc: pedro, aburgess, 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    | 18 ++++++-
 gdb/testsuite/gdb.reverse/step-reverse.exp  | 58 +++++++++++++++++++--
 4 files changed, 76 insertions(+), 8 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..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..3d647b9b29d 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.c
+++ b/gdb/testsuite/gdb.reverse/step-reverse.c
@@ -26,6 +26,19 @@ 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 +64,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 +76,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..a540b1f88ce 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,59 @@ 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 "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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Ping][PATCH v3 0/2] Fix reverse nexting over recursions
  2022-08-31 12:07 [PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
  2022-08-31 12:07 ` [PATCH v3 1/2] Change calculation of frame_id by amd64 epilogue unwinder Bruno Larsen
  2022-08-31 12:07 ` [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
@ 2022-09-14 13:16 ` Bruno Larsen
  2022-09-22 14:13   ` [PINGv2][PATCH " Bruno Larsen
  2 siblings, 1 reply; 8+ messages in thread
From: Bruno Larsen @ 2022-09-14 13:16 UTC (permalink / raw)
  To: gdb-patches; +Cc: pedro, aburgess

ping!

Cheers,
Bruno

On 31/08/2022 14:07, 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 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      |  18 +-
>   gdb/testsuite/gdb.reverse/step-reverse.exp    |  58 ++++++-
>   8 files changed, 282 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
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PINGv2][PATCH v3 0/2] Fix reverse nexting over recursions
  2022-09-14 13:16 ` [Ping][PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
@ 2022-09-22 14:13   ` Bruno Larsen
  2022-09-29  7:02     ` [PINGv3][PATCH " Bruno Larsen
  0 siblings, 1 reply; 8+ messages in thread
From: Bruno Larsen @ 2022-09-22 14:13 UTC (permalink / raw)
  To: gdb-patches, Bruno Larsen

ping!

Cheers,
Bruno

On 14/09/2022 15:16, Bruno Larsen wrote:
> ping!
>
> Cheers,
> Bruno
>
> On 31/08/2022 14:07, 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 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      |  18 +-
>>   gdb/testsuite/gdb.reverse/step-reverse.exp    |  58 ++++++-
>>   8 files changed, 282 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
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PINGv3][PATCH v3 0/2] Fix reverse nexting over recursions
  2022-09-22 14:13   ` [PINGv2][PATCH " Bruno Larsen
@ 2022-09-29  7:02     ` Bruno Larsen
  0 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2022-09-29  7:02 UTC (permalink / raw)
  To: Bruno Larsen, Bruno Larsen via Gdb-patches

Ping

Cheers,
Bruno

On 22/09/2022 16:13, Bruno Larsen wrote:
> ping!
>
> Cheers,
> Bruno
>
> On 14/09/2022 15:16, Bruno Larsen wrote:
>> ping!
>>
>> Cheers,
>> Bruno
>>
>> On 31/08/2022 14:07, 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 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      |  18 +-
>>>   gdb/testsuite/gdb.reverse/step-reverse.exp    |  58 ++++++-
>>>   8 files changed, 282 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
>>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions
  2022-08-31 12:07 ` [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
@ 2022-09-30 18:28   ` Pedro Alves
  2022-10-04 16:40     ` Bruno Larsen
  0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2022-09-30 18:28 UTC (permalink / raw)
  To: Bruno Larsen, gdb-patches

[-- Attachment #1: Type: text/plain, Size: 9596 bytes --]

On 2022-08-31 1:07 p.m., Bruno Larsen 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    | 18 ++++++-
>  gdb/testsuite/gdb.reverse/step-reverse.exp  | 58 +++++++++++++++++++--
>  4 files changed, 76 insertions(+), 8 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..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..3d647b9b29d 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -26,6 +26,19 @@ 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 */

Could you make the function follow GNU formatting?  I know that the file
has other bits that don't follow the style, but we can just not add more cases.

> +
>  /* A structure which, we hope, will need to be passed using memcpy.  */
>  struct rhomboidal {
>    int rather_large[100];
> @@ -51,6 +64,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 +76,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..a540b1f88ce 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,59 @@ 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 "reverse-next\n"
> +	    exp_continue

The command passed to gdb_test_multiple was "next", but here you
do "reverse-next".  Seems best to be consistent.  After, this can
potentially infinite loop of the reverse-next never takes you to the
expected line, which seems a bit dangerous.

> +	}
> +    }
> +}
> +
> +# 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"
> +	}

gdb_assert

> +    }
> +    -re -wrap ".*" {
> +	send_gdb "next\n"

Ditto, re. infinite loop.

> +	exp_continue
> +    }
> +}
> +

Other than that, it looks fine to me.  

I still see this failing on Ubuntu 20.04, though.  I haven't investigated why.  I've attached
the gdb.log, in case something jumps out as obvious to you.

The before/after gdb.sum diff shows that some other tests FAIL on this machine, so maybe
it's just more of the same.

@@ -13,7 +13,7 @@ PASS: gdb.reverse/step-reverse.exp: next
 PASS: gdb.reverse/step-reverse.exp: step test 1
 PASS: gdb.reverse/step-reverse.exp: next test 2
 PASS: gdb.reverse/step-reverse.exp: step test 2
-PASS: gdb.reverse/step-reverse.exp: step up to call 
+PASS: gdb.reverse/step-reverse.exp: next over recursion 
 PASS: gdb.reverse/step-reverse.exp: next over call
 PASS: gdb.reverse/step-reverse.exp: step into call
 PASS: gdb.reverse/step-reverse.exp: finish out of fn call
@@ -27,14 +27,20 @@ PASS: gdb.reverse/step-reverse.exp: simp
 FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
 FAIL: gdb.reverse/step-reverse.exp: reverse step out of called fn
 FAIL: gdb.reverse/step-reverse.exp: reverse next over call
-FAIL: gdb.reverse/step-reverse.exp: reverse step test 1 
-FAIL: gdb.reverse/step-reverse.exp: reverse next test 1 
-FAIL: gdb.reverse/step-reverse.exp: reverse step test 2 
-FAIL: gdb.reverse/step-reverse.exp: reverse next test 2 
+FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion 
+PASS: gdb.reverse/step-reverse.exp: forward again to test recursion 
+FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again 
+PASS: gdb.reverse/step-reverse.exp: reverse again to test recursion 
+FAIL: gdb.reverse/step-reverse.exp: enter recursive function 
+PASS: gdb.reverse/step-reverse.exp: step over recursion inside the recursion 
+PASS: gdb.reverse/step-reverse.exp: reverse step test 1 
+PASS: gdb.reverse/step-reverse.exp: reverse next test 1 
+PASS: gdb.reverse/step-reverse.exp: reverse step test 2 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb.log --]
[-- Type: text/x-log; name="gdb.log", Size: 11016 bytes --]

Test run by pedro on Fri Sep 30 17:52:30 2022
Native configuration is x86_64-pc-linux-gnu

		=== gdb tests ===

Schedule of variations:
    unix

Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.exp ...
Executing on host: gcc   -fdiagnostics-color=never -c -o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/temp/1465468/ccopts1465468.o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/temp/1465468/ccopts1465468.c    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fdiagnostics-color=never -c -o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/temp/1465468/ccopts1465468.o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/temp/1465468/ccopts1465468.c
get_compiler_info: gcc-9-4-0
Executing on host: gcc  -fno-stack-protector  -fdiagnostics-color=never -c -g  -o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse0.o /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fno-stack-protector -fdiagnostics-color=never -c -g -o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse0.o /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c
Executing on host: gcc  -fno-stack-protector /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse0.o  -fdiagnostics-color=never -g  -lm   -o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fno-stack-protector /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse0.o -fdiagnostics-color=never -g -lm -o /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse
builtin_spawn /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../../gdb/gdb -nw -nx -iex set height 0 -iex set width 0 -data-directory /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../data-directory
GNU gdb (GDB) 13.0.50.20220930-git
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) set height 0
(gdb) set width 0
(gdb) dir
Reinitialize source path to empty? (y or n) y
Source directories searched: $cdir:$cwd
(gdb) dir /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse
Source directories searched: /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse:$cdir:$cwd
(gdb) kill
The program is not being run.
(gdb) file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse
Reading symbols from /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse...
(gdb) delete breakpoints
(gdb) info breakpoints
No breakpoints or watchpoints.
(gdb) break -qualified main
Breakpoint 1 at 0x11f1: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c, line 58.
(gdb) run 
Starting program: /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse 

Breakpoint 1, main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:58
58	   w = 0;	/* BREAK AT MAIN */
(gdb) record
(gdb) PASS: gdb.reverse/step-reverse.exp: turn on process record
next
59	   x = 1;	/* NEXT TEST 1 */
(gdb) PASS: gdb.reverse/step-reverse.exp: next test 1
step
60	   y = 2;	/* STEP TEST 1 */
(gdb) PASS: gdb.reverse/step-reverse.exp: step test 1
next 2
62	   w = w + 2;	/* NEXT TEST 2 */
(gdb) PASS: gdb.reverse/step-reverse.exp: next test 2
step 3
65	   z = z + 5;	/* STEP TEST 2 */
(gdb) PASS: gdb.reverse/step-reverse.exp: step test 2
next 2
71	   callee();	/* NEXT OVER THIS CALL */
(gdb) PASS: gdb.reverse/step-reverse.exp: next over recursion
next
74	   callee();	/* STEP INTO THIS CALL */
(gdb) PASS: gdb.reverse/step-reverse.exp: next over call
step
callee () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:26
26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) PASS: gdb.reverse/step-reverse.exp: step into call
finish
Run till exit from #0  callee () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:26
main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:77
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
Value returned is $1 = 1
(gdb) PASS: gdb.reverse/step-reverse.exp: finish out of fn call
stepi
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) stepi
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) stepi
0x0000555555555243	77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) stepi
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) stepi
78	   callee();	/* STEPI TEST */
(gdb) PASS: gdb.reverse/step-reverse.exp: simple stepi
stepi
0x000055555555524d	78	   callee();	/* STEPI TEST */
(gdb) stepi
callee () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:25
25	int callee() {		/* ENTER CALLEE */
(gdb) stepi
0x000055555555516d	25	int callee() {		/* ENTER CALLEE */
(gdb) stepi
0x000055555555516e	25	int callee() {		/* ENTER CALLEE */
(gdb) stepi
26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) PASS: gdb.reverse/step-reverse.exp: stepi into function call
stepi
0x0000555555555177	26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) stepi
0x000055555555517a	26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) stepi
27	}			/* RETURN FROM CALLEE */
(gdb) stepi
0x0000555555555181	27	}			/* RETURN FROM CALLEE */
(gdb) stepi
main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:81
81	   callee();	/* NEXTI TEST */
(gdb) PASS: gdb.reverse/step-reverse.exp: stepi back from function call
set exec-dir reverse
(gdb) PASS: gdb.reverse/step-reverse.exp: set reverse execution
stepi
0x0000555555555181 in callee () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:27
27	}			/* RETURN FROM CALLEE */
(gdb) stepi
27	}			/* RETURN FROM CALLEE */
(gdb) stepi
0x000055555555517a	26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) stepi
0x0000555555555177	26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) stepi
26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse stepi thru function return
stepi
0x000055555555516e	25	int callee() {		/* ENTER CALLEE */
(gdb) stepi
0x000055555555516d	25	int callee() {		/* ENTER CALLEE */
(gdb) stepi
25	int callee() {		/* ENTER CALLEE */
(gdb) stepi
0x000055555555524d in main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:78
78	   callee();	/* STEPI TEST */
(gdb) stepi
78	   callee();	/* STEPI TEST */
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse stepi from a function call
stepi
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) PASS: gdb.reverse/step-reverse.exp: simple reverse stepi
step
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
step
callee () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:27
27	}			/* RETURN FROM CALLEE */
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse step out of called fn
next
26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse next over call
next
main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:74
74	   callee();	/* STEP INTO THIS CALL */
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion
set exec-dir forward
(gdb) PASS: gdb.reverse/step-reverse.exp: forward again to test recursion
next
77	   a[5] = a[3] - a[4]; /* FINISH TEST */
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again
set exec-dir reverse
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse again to test recursion
step
callee () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:27
27	}			/* RETURN FROM CALLEE */
(gdb) FAIL: gdb.reverse/step-reverse.exp: enter recursive function
next
26	  return myglob++;	/* ARRIVED IN CALLEE */
(gdb) next
main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:74
74	   callee();	/* STEP INTO THIS CALL */
(gdb) next
71	   callee();	/* NEXT OVER THIS CALL */
(gdb) next
68	   recursive_callee (32); /* NEXT OVER THIS RECURSION */
(gdb) PASS: gdb.reverse/step-reverse.exp: step over recursion inside the recursion
step 3
63	   x = x + 3;	/* REVERSE STEP TEST 1 */
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse step test 1
next 2
61	   z = 3;	/* REVERSE NEXT TEST 1 */
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse next test 1
step
60	   y = 2;	/* STEP TEST 1 */
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse step test 2
next
59	   x = 1;	/* NEXT TEST 1 */
(gdb) PASS: gdb.reverse/step-reverse.exp: reverse next test 2
testcase /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/step-reverse.exp completed in 0 seconds

		=== gdb Summary ===

# of expected passes		23
# of unexpected failures	6
Executing on host: /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../../gdb/gdb -nw -nx -iex "set height 0" -iex "set width 0" -data-directory /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../data-directory --version    (timeout = 300)
builtin_spawn -ignore SIGHUP /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../../gdb/gdb -nw -nx -iex set height 0 -iex set width 0 -data-directory /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../data-directory --version
GNU gdb (GDB) 13.0.50.20220930-git
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
/home/pedro/gdb/binutils-gdb/build/gdb/gdb version  13.0.50.20220930-git -nw -nx -iex "set height 0" -iex "set width 0" -data-directory /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../data-directory 

runtest completed at Fri Sep 30 17:52:30 2022

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions
  2022-09-30 18:28   ` Pedro Alves
@ 2022-10-04 16:40     ` Bruno Larsen
  0 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2022-10-04 16:40 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches

On 30/09/2022 20:28, Pedro Alves wrote:
> On 2022-08-31 1:07 p.m., Bruno Larsen 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    | 18 ++++++-
>>   gdb/testsuite/gdb.reverse/step-reverse.exp  | 58 +++++++++++++++++++--
>>   4 files changed, 76 insertions(+), 8 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..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..3d647b9b29d 100644
>> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
>> @@ -26,6 +26,19 @@ 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 */
> Could you make the function follow GNU formatting?  I know that the file
> has other bits that don't follow the style, but we can just not add more cases.
Oops, sorry, old habits die hard.
>
>> +
>>   /* A structure which, we hope, will need to be passed using memcpy.  */
>>   struct rhomboidal {
>>     int rather_large[100];
>> @@ -51,6 +64,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 +76,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..a540b1f88ce 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,59 @@ 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 "reverse-next\n"
>> +	    exp_continue
> The command passed to gdb_test_multiple was "next", but here you
> do "reverse-next".  Seems best to be consistent.  After, this can
> potentially infinite loop of the reverse-next never takes you to the
> expected line, which seems a bit dangerous.

Thanks for catching that, it is a problem, but for a different reason.

On line 147 on this file we set the execution direction as reverse, so 
all "next" commands are "reverse-next". The problem is that using an 
explicit reverse-next makes GDB throw an error and doesn't step, which 
will give us an infinite loop for sure.

>
>> +	}
>> +    }
>> +}
>> +
>> +# 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"
I just noticed this test name is incorrect.
>> +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"
>> +	}
> gdb_assert
>
>> +    }
>> +    -re -wrap ".*" {
>> +	send_gdb "next\n"
> Ditto, re. infinite loop.
This, on the other hand, is not a problem. The execution is set as 
reverse, so "next" here is equivalent to "reverse-next" implicitly.
>
>> +	exp_continue
>> +    }
>> +}
>> +
> Other than that, it looks fine to me.
>
> I still see this failing on Ubuntu 20.04, though.  I haven't investigated why.  I've attached
> the gdb.log, in case something jumps out as obvious to you.
>
> The before/after gdb.sum diff shows that some other tests FAIL on this machine, so maybe
> it's just more of the same.

I think you attached the before log, not the after, not sure if that is 
what you meant to do...

Regardless, what it looks to me is that this version of gcc in ubuntu 
20.04 is making it so we need more than one step to go through the line 
`a[5] = a[3] - a[4];`, so the inferior's actual state gets de-synced 
from the test's expected state. As supporting evidence, after my patch , 
we get PASSes after my re-syncing code, which were FAILs before.

You can probably easily fix this with a gcc check and an extra step at 
the right spot.

Cheers,
Bruno

>
> @@ -13,7 +13,7 @@ PASS: gdb.reverse/step-reverse.exp: next
>   PASS: gdb.reverse/step-reverse.exp: step test 1
>   PASS: gdb.reverse/step-reverse.exp: next test 2
>   PASS: gdb.reverse/step-reverse.exp: step test 2
> -PASS: gdb.reverse/step-reverse.exp: step up to call
> +PASS: gdb.reverse/step-reverse.exp: next over recursion
>   PASS: gdb.reverse/step-reverse.exp: next over call
>   PASS: gdb.reverse/step-reverse.exp: step into call
>   PASS: gdb.reverse/step-reverse.exp: finish out of fn call
> @@ -27,14 +27,20 @@ PASS: gdb.reverse/step-reverse.exp: simp
>   FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
>   FAIL: gdb.reverse/step-reverse.exp: reverse step out of called fn
>   FAIL: gdb.reverse/step-reverse.exp: reverse next over call
> -FAIL: gdb.reverse/step-reverse.exp: reverse step test 1
> -FAIL: gdb.reverse/step-reverse.exp: reverse next test 1
> -FAIL: gdb.reverse/step-reverse.exp: reverse step test 2
> -FAIL: gdb.reverse/step-reverse.exp: reverse next test 2
> +FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion
> +PASS: gdb.reverse/step-reverse.exp: forward again to test recursion
> +FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again
> +PASS: gdb.reverse/step-reverse.exp: reverse again to test recursion
> +FAIL: gdb.reverse/step-reverse.exp: enter recursive function
> +PASS: gdb.reverse/step-reverse.exp: step over recursion inside the recursion
> +PASS: gdb.reverse/step-reverse.exp: reverse step test 1
> +PASS: gdb.reverse/step-reverse.exp: reverse next test 1
> +PASS: gdb.reverse/step-reverse.exp: reverse step test 2
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-10-04 16:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-31 12:07 [PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
2022-08-31 12:07 ` [PATCH v3 1/2] Change calculation of frame_id by amd64 epilogue unwinder Bruno Larsen
2022-08-31 12:07 ` [PATCH v3 2/2] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
2022-09-30 18:28   ` Pedro Alves
2022-10-04 16:40     ` Bruno Larsen
2022-09-14 13:16 ` [Ping][PATCH v3 0/2] Fix reverse nexting over recursions Bruno Larsen
2022-09-22 14:13   ` [PINGv2][PATCH " Bruno Larsen
2022-09-29  7:02     ` [PINGv3][PATCH " Bruno Larsen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).