* [PATCH] gdb/reverse: Fix stepping over recursive functions
@ 2022-05-25 18:02 Bruno Larsen
2022-06-08 11:17 ` [PING] " Bruno Larsen
2022-07-07 18:48 ` Pedro Alves
0 siblings, 2 replies; 9+ messages in thread
From: Bruno Larsen @ 2022-05-25 18:02 UTC (permalink / raw)
To: gdb-patches
Currently, when using GDB to do reverse debugging, if we try to use the
command "reverse next" to skip a recursive function, instead of skipping
all of the recursive calls and stopping in the previous line, we stop at
the second to last recursive call, and need to manually step backwards
until we leave the first call. This is well documented in PR gdb/16678.
This bug happens because when GDB notices that a reverse step has
entered into a function, GDB will add a step_resume_breakpoint at the
start of the function, then single step out of the prologue once that
breakpoint is hit. Recursion poses a problem because that breakpoint will
be hit many times before GDB should actually stop the inferior. To fix
this issue, when the step_resume_breakpoint is hit (and GDB is executing
backwards), we analyze if the caller of the frame is the original frame
where we started the "reverse next", and if it is, GDB will stop the
inferior, otherwise GDB will just ignore the breakpoint.
This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
3 files changed, 95 insertions(+), 4 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 02c98b50c8c..95fb3227aa3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
case BPSTAT_WHAT_STEP_RESUME:
infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
+ /* If we are executing backwards, we are doing a "next" and the
+ current frame is not the same as where we started, the
+ step_resume_breakpoint we have just hit has been added to the start
+ of a function so we can skip the whle function. However, if we are
+ skipping a recursive call, we only want to act as if we hit the
+ breakpoint only when the caller of the current frame is the original
+ frame we were single stepping from. */
+ if (execution_direction == EXEC_REVERSE
+ && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+ && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
+ ecs->event_thread->control.step_stack_frame_id))
+ {
+ infrun_debug_printf ("We are not done with recursing yet");
+ /* If the caller's ID is not the same as the starting frame, we
+ can ignore this breakpoint. */
+ if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
+ ecs->event_thread->control.step_stack_frame_id))
+ {
+ keep_going (ecs);
+ return;
+ }
+ } else
+ infrun_debug_printf ("are we not recursing?");
+
delete_step_resume_breakpoint (ecs->event_thread);
if (ecs->event_thread->control.proceed_to_finish
&& execution_direction == EXEC_REVERSE)
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
index aea2a98541d..a390ac2580c 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.c
+++ b/gdb/testsuite/gdb.reverse/step-reverse.c
@@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
return myglob++; /* ARRIVED IN CALLEE */
} /* RETURN FROM CALLEE */
+/* We need to make this function take more than a single instruction
+ to run, otherwise it could hide PR gdb/16678, as reverse execution can
+ step over a single-instruction function. */
+int recursive_callee (int val){
+ if (val == 0) return 0;
+ val /= 2;
+ if (val>1)
+ val++;
+ return recursive_callee (val); /* RECURSIVE CALL */
+} /* EXIT RECURSIVE FUNCTION */
+
/* A structure which, we hope, will need to be passed using memcpy. */
struct rhomboidal {
int rather_large[100];
@@ -51,6 +62,9 @@ int main () {
y = y + 4;
z = z + 5; /* STEP TEST 2 */
+ /* Test that next goes over recursive calls too */
+ recursive_callee (32); /* NEXT OVER THIS RECURSION */
+
/* Test that "next" goes over a call */
callee(); /* NEXT OVER THIS CALL */
@@ -60,7 +74,7 @@ int main () {
/* Test "stepi" */
a[5] = a[3] - a[4]; /* FINISH TEST */
callee(); /* STEPI TEST */
-
+
/* Test "nexti" */
callee(); /* NEXTI TEST */
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index 997b62604d5..4f56b4785ca 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
+# next through a recursive function call
+gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
+
# step over call
-gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
# step into call
@@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
set test_message "stepi back from function call"
gdb_test_multiple "stepi" "$test_message" {
- -re "NEXTI TEST.*$gdb_prompt $" {
+ -re -wrap "NEXTI TEST.*" {
pass "$test_message"
}
-re "ARRIVED IN CALLEE.*$gdb_prompt $" {
@@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
###
# Set reverse execution direction
-
gdb_test_no_output "set exec-dir reverse" "set reverse execution"
# stepi backward thru return and into a function
@@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
+# Dont reverse the execution direction yet, as we will need another
+# forward step after this test
+
+set step_out 0
+gdb_test_multiple "next" "reverse next over recursion" {
+ -re -wrap ".*NEXT OVER THIS RECURSION.*" {
+ pass "reverse next over recursion"
+ }
+ -re -wrap ".*RECURSIVE CALL.*" {
+ fail "reverse next over recursion"
+ set step_out 1
+ }
+}
+if { "$step_out" == 1 } {
+ gdb_test_multiple "next" "stepping out of recursion" {
+ -re -wrap "NEXT OVER THIS RECURSION.*" {
+ set step_out 0
+ }
+ -re -wrap ".*" {
+ send_gdb "reverse-next\n"
+ exp_continue
+ }
+ }
+}
+
+# Step forward over recursion again so we can test stepping over calls
+# inside the recursion itself.
+gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
+gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
+gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
+
+gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
+set step_pass 1
+gdb_test_multiple "next" "step over recursion inside the recursion" {
+ -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
+ set step_pass 0
+ send_gdb "next\n"
+ exp_continue
+ }
+ -re -wrap ".*NEXT OVER THIS RECURSION.*" {
+ if {$step_pass} {
+ pass "step over recursion inside the recursion"
+ } else {
+ fail "step over recursion inside the recursion"
+ }
+ }
+ -re -wrap ".*" {
+ send_gdb "next\n"
+ exp_continue
+ }
+}
+
# step/next backward with count
gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PING] [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-05-25 18:02 [PATCH] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
@ 2022-06-08 11:17 ` Bruno Larsen
2022-06-15 19:21 ` [PINGv2] " Bruno Larsen
2022-07-07 18:48 ` Pedro Alves
1 sibling, 1 reply; 9+ messages in thread
From: Bruno Larsen @ 2022-06-08 11:17 UTC (permalink / raw)
To: gdb-patches
Ping!
Cheers!
Bruno Larsen
On 5/25/22 15:02, 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. Recursion poses a problem because that breakpoint will
> be hit many times before GDB should actually stop the inferior. To fix
> this issue, when the step_resume_breakpoint is hit (and GDB is executing
> backwards), we analyze if the caller of the frame is the original frame
> where we started the "reverse next", and if it is, GDB will stop the
> inferior, otherwise GDB will just ignore the breakpoint.
>
> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
> 3 files changed, 95 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 02c98b50c8c..95fb3227aa3 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
> case BPSTAT_WHAT_STEP_RESUME:
> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>
> + /* If we are executing backwards, we are doing a "next" and the
> + current frame is not the same as where we started, the
> + step_resume_breakpoint we have just hit has been added to the start
> + of a function so we can skip the whle function. However, if we are
> + skipping a recursive call, we only want to act as if we hit the
> + breakpoint only when the caller of the current frame is the original
> + frame we were single stepping from. */
> + if (execution_direction == EXEC_REVERSE
> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
> + ecs->event_thread->control.step_stack_frame_id))
> + {
> + infrun_debug_printf ("We are not done with recursing yet");
> + /* If the caller's ID is not the same as the starting frame, we
> + can ignore this breakpoint. */
> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
> + ecs->event_thread->control.step_stack_frame_id))
> + {
> + keep_going (ecs);
> + return;
> + }
> + } else
> + infrun_debug_printf ("are we not recursing?");
> +
> delete_step_resume_breakpoint (ecs->event_thread);
> if (ecs->event_thread->control.proceed_to_finish
> && execution_direction == EXEC_REVERSE)
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
> index aea2a98541d..a390ac2580c 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
> return myglob++; /* ARRIVED IN CALLEE */
> } /* RETURN FROM CALLEE */
>
> +/* We need to make this function take more than a single instruction
> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
> + step over a single-instruction function. */
> +int recursive_callee (int val){
> + if (val == 0) return 0;
> + val /= 2;
> + if (val>1)
> + val++;
> + return recursive_callee (val); /* RECURSIVE CALL */
> +} /* EXIT RECURSIVE FUNCTION */
> +
> /* A structure which, we hope, will need to be passed using memcpy. */
> struct rhomboidal {
> int rather_large[100];
> @@ -51,6 +62,9 @@ int main () {
> y = y + 4;
> z = z + 5; /* STEP TEST 2 */
>
> + /* Test that next goes over recursive calls too */
> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
> +
> /* Test that "next" goes over a call */
> callee(); /* NEXT OVER THIS CALL */
>
> @@ -60,7 +74,7 @@ int main () {
> /* Test "stepi" */
> a[5] = a[3] - a[4]; /* FINISH TEST */
> callee(); /* STEPI TEST */
> -
> +
> /* Test "nexti" */
> callee(); /* NEXTI TEST */
>
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index 997b62604d5..4f56b4785ca 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>
> +# next through a recursive function call
> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
> +
> # step over call
>
> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>
> # step into call
> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>
> set test_message "stepi back from function call"
> gdb_test_multiple "stepi" "$test_message" {
> - -re "NEXTI TEST.*$gdb_prompt $" {
> + -re -wrap "NEXTI TEST.*" {
> pass "$test_message"
> }
> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
> ###
>
> # Set reverse execution direction
> -
> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>
> # stepi backward thru return and into a function
> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>
> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>
> +# Dont reverse the execution direction yet, as we will need another
> +# forward step after this test
> +
> +set step_out 0
> +gdb_test_multiple "next" "reverse next over recursion" {
> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> + pass "reverse next over recursion"
> + }
> + -re -wrap ".*RECURSIVE CALL.*" {
> + fail "reverse next over recursion"
> + set step_out 1
> + }
> +}
> +if { "$step_out" == 1 } {
> + gdb_test_multiple "next" "stepping out of recursion" {
> + -re -wrap "NEXT OVER THIS RECURSION.*" {
> + set step_out 0
> + }
> + -re -wrap ".*" {
> + send_gdb "reverse-next\n"
> + exp_continue
> + }
> + }
> +}
> +
> +# Step forward over recursion again so we can test stepping over calls
> +# inside the recursion itself.
> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
> +
> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
> +set step_pass 1
> +gdb_test_multiple "next" "step over recursion inside the recursion" {
> + -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
> + set step_pass 0
> + send_gdb "next\n"
> + exp_continue
> + }
> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> + if {$step_pass} {
> + pass "step over recursion inside the recursion"
> + } else {
> + fail "step over recursion inside the recursion"
> + }
> + }
> + -re -wrap ".*" {
> + send_gdb "next\n"
> + exp_continue
> + }
> +}
> +
> # step/next backward with count
>
> gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PINGv2] [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-06-08 11:17 ` [PING] " Bruno Larsen
@ 2022-06-15 19:21 ` Bruno Larsen
2022-06-22 18:49 ` [PINGv3] " Bruno Larsen
0 siblings, 1 reply; 9+ messages in thread
From: Bruno Larsen @ 2022-06-15 19:21 UTC (permalink / raw)
To: gdb-patches
ping!
Cheers!
Bruno Larsen
On 6/8/22 08:17, Bruno Larsen wrote:
> Ping!
>
> Cheers!
> Bruno Larsen
>
> On 5/25/22 15:02, 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. Recursion poses a problem because that breakpoint will
>> be hit many times before GDB should actually stop the inferior. To fix
>> this issue, when the step_resume_breakpoint is hit (and GDB is executing
>> backwards), we analyze if the caller of the frame is the original frame
>> where we started the "reverse next", and if it is, GDB will stop the
>> inferior, otherwise GDB will just ignore the breakpoint.
>>
>> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
>> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
>> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
>> 3 files changed, 95 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 02c98b50c8c..95fb3227aa3 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
>> case BPSTAT_WHAT_STEP_RESUME:
>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>> + /* If we are executing backwards, we are doing a "next" and the
>> + current frame is not the same as where we started, the
>> + step_resume_breakpoint we have just hit has been added to the start
>> + of a function so we can skip the whle function. However, if we are
>> + skipping a recursive call, we only want to act as if we hit the
>> + breakpoint only when the caller of the current frame is the original
>> + frame we were single stepping from. */
>> + if (execution_direction == EXEC_REVERSE
>> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
>> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
>> + ecs->event_thread->control.step_stack_frame_id))
>> + {
>> + infrun_debug_printf ("We are not done with recursing yet");
>> + /* If the caller's ID is not the same as the starting frame, we
>> + can ignore this breakpoint. */
>> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
>> + ecs->event_thread->control.step_stack_frame_id))
>> + {
>> + keep_going (ecs);
>> + return;
>> + }
>> + } else
>> + infrun_debug_printf ("are we not recursing?");
>> +
>> delete_step_resume_breakpoint (ecs->event_thread);
>> if (ecs->event_thread->control.proceed_to_finish
>> && execution_direction == EXEC_REVERSE)
>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
>> index aea2a98541d..a390ac2580c 100644
>> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
>> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
>> return myglob++; /* ARRIVED IN CALLEE */
>> } /* RETURN FROM CALLEE */
>> +/* We need to make this function take more than a single instruction
>> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
>> + step over a single-instruction function. */
>> +int recursive_callee (int val){
>> + if (val == 0) return 0;
>> + val /= 2;
>> + if (val>1)
>> + val++;
>> + return recursive_callee (val); /* RECURSIVE CALL */
>> +} /* EXIT RECURSIVE FUNCTION */
>> +
>> /* A structure which, we hope, will need to be passed using memcpy. */
>> struct rhomboidal {
>> int rather_large[100];
>> @@ -51,6 +62,9 @@ int main () {
>> y = y + 4;
>> z = z + 5; /* STEP TEST 2 */
>> + /* Test that next goes over recursive calls too */
>> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
>> +
>> /* Test that "next" goes over a call */
>> callee(); /* NEXT OVER THIS CALL */
>> @@ -60,7 +74,7 @@ int main () {
>> /* Test "stepi" */
>> a[5] = a[3] - a[4]; /* FINISH TEST */
>> callee(); /* STEPI TEST */
>> -
>> +
>> /* Test "nexti" */
>> callee(); /* NEXTI TEST */
>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>> index 997b62604d5..4f56b4785ca 100644
>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>> +# next through a recursive function call
>> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
>> +
>> # step over call
>> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>> # step into call
>> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>> set test_message "stepi back from function call"
>> gdb_test_multiple "stepi" "$test_message" {
>> - -re "NEXTI TEST.*$gdb_prompt $" {
>> + -re -wrap "NEXTI TEST.*" {
>> pass "$test_message"
>> }
>> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
>> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>> ###
>> # Set reverse execution direction
>> -
>> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>> # stepi backward thru return and into a function
>> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>> +# Dont reverse the execution direction yet, as we will need another
>> +# forward step after this test
>> +
>> +set step_out 0
>> +gdb_test_multiple "next" "reverse next over recursion" {
>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>> + pass "reverse next over recursion"
>> + }
>> + -re -wrap ".*RECURSIVE CALL.*" {
>> + fail "reverse next over recursion"
>> + set step_out 1
>> + }
>> +}
>> +if { "$step_out" == 1 } {
>> + gdb_test_multiple "next" "stepping out of recursion" {
>> + -re -wrap "NEXT OVER THIS RECURSION.*" {
>> + set step_out 0
>> + }
>> + -re -wrap ".*" {
>> + send_gdb "reverse-next\n"
>> + exp_continue
>> + }
>> + }
>> +}
>> +
>> +# Step forward over recursion again so we can test stepping over calls
>> +# inside the recursion itself.
>> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
>> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
>> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
>> +
>> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
>> +set step_pass 1
>> +gdb_test_multiple "next" "step over recursion inside the recursion" {
>> + -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
>> + set step_pass 0
>> + send_gdb "next\n"
>> + exp_continue
>> + }
>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>> + if {$step_pass} {
>> + pass "step over recursion inside the recursion"
>> + } else {
>> + fail "step over recursion inside the recursion"
>> + }
>> + }
>> + -re -wrap ".*" {
>> + send_gdb "next\n"
>> + exp_continue
>> + }
>> +}
>> +
>> # step/next backward with count
>> gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PINGv3] [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-06-15 19:21 ` [PINGv2] " Bruno Larsen
@ 2022-06-22 18:49 ` Bruno Larsen
2022-06-29 18:46 ` [PINGv4] " Bruno Larsen
0 siblings, 1 reply; 9+ messages in thread
From: Bruno Larsen @ 2022-06-22 18:49 UTC (permalink / raw)
To: gdb-patches
ping!
Cheers!
Bruno Larsen
On 6/15/22 16:21, Bruno Larsen wrote:
> ping!
>
> Cheers!
> Bruno Larsen
>
> On 6/8/22 08:17, Bruno Larsen wrote:
>> Ping!
>>
>> Cheers!
>> Bruno Larsen
>>
>> On 5/25/22 15:02, 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. Recursion poses a problem because that breakpoint will
>>> be hit many times before GDB should actually stop the inferior. To fix
>>> this issue, when the step_resume_breakpoint is hit (and GDB is executing
>>> backwards), we analyze if the caller of the frame is the original frame
>>> where we started the "reverse next", and if it is, GDB will stop the
>>> inferior, otherwise GDB will just ignore the breakpoint.
>>>
>>> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
>>> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
>>> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
>>> 3 files changed, 95 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>> index 02c98b50c8c..95fb3227aa3 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
>>> case BPSTAT_WHAT_STEP_RESUME:
>>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>>> + /* If we are executing backwards, we are doing a "next" and the
>>> + current frame is not the same as where we started, the
>>> + step_resume_breakpoint we have just hit has been added to the start
>>> + of a function so we can skip the whle function. However, if we are
>>> + skipping a recursive call, we only want to act as if we hit the
>>> + breakpoint only when the caller of the current frame is the original
>>> + frame we were single stepping from. */
>>> + if (execution_direction == EXEC_REVERSE
>>> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
>>> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
>>> + ecs->event_thread->control.step_stack_frame_id))
>>> + {
>>> + infrun_debug_printf ("We are not done with recursing yet");
>>> + /* If the caller's ID is not the same as the starting frame, we
>>> + can ignore this breakpoint. */
>>> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
>>> + ecs->event_thread->control.step_stack_frame_id))
>>> + {
>>> + keep_going (ecs);
>>> + return;
>>> + }
>>> + } else
>>> + infrun_debug_printf ("are we not recursing?");
>>> +
>>> delete_step_resume_breakpoint (ecs->event_thread);
>>> if (ecs->event_thread->control.proceed_to_finish
>>> && execution_direction == EXEC_REVERSE)
>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
>>> index aea2a98541d..a390ac2580c 100644
>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
>>> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
>>> return myglob++; /* ARRIVED IN CALLEE */
>>> } /* RETURN FROM CALLEE */
>>> +/* We need to make this function take more than a single instruction
>>> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
>>> + step over a single-instruction function. */
>>> +int recursive_callee (int val){
>>> + if (val == 0) return 0;
>>> + val /= 2;
>>> + if (val>1)
>>> + val++;
>>> + return recursive_callee (val); /* RECURSIVE CALL */
>>> +} /* EXIT RECURSIVE FUNCTION */
>>> +
>>> /* A structure which, we hope, will need to be passed using memcpy. */
>>> struct rhomboidal {
>>> int rather_large[100];
>>> @@ -51,6 +62,9 @@ int main () {
>>> y = y + 4;
>>> z = z + 5; /* STEP TEST 2 */
>>> + /* Test that next goes over recursive calls too */
>>> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
>>> +
>>> /* Test that "next" goes over a call */
>>> callee(); /* NEXT OVER THIS CALL */
>>> @@ -60,7 +74,7 @@ int main () {
>>> /* Test "stepi" */
>>> a[5] = a[3] - a[4]; /* FINISH TEST */
>>> callee(); /* STEPI TEST */
>>> -
>>> +
>>> /* Test "nexti" */
>>> callee(); /* NEXTI TEST */
>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>> index 997b62604d5..4f56b4785ca 100644
>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>>> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>>> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>>> +# next through a recursive function call
>>> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
>>> +
>>> # step over call
>>> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>>> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>>> # step into call
>>> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>>> set test_message "stepi back from function call"
>>> gdb_test_multiple "stepi" "$test_message" {
>>> - -re "NEXTI TEST.*$gdb_prompt $" {
>>> + -re -wrap "NEXTI TEST.*" {
>>> pass "$test_message"
>>> }
>>> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
>>> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>>> ###
>>> # Set reverse execution direction
>>> -
>>> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>>> # stepi backward thru return and into a function
>>> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>>> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>>> +# Dont reverse the execution direction yet, as we will need another
>>> +# forward step after this test
>>> +
>>> +set step_out 0
>>> +gdb_test_multiple "next" "reverse next over recursion" {
>>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>>> + pass "reverse next over recursion"
>>> + }
>>> + -re -wrap ".*RECURSIVE CALL.*" {
>>> + fail "reverse next over recursion"
>>> + set step_out 1
>>> + }
>>> +}
>>> +if { "$step_out" == 1 } {
>>> + gdb_test_multiple "next" "stepping out of recursion" {
>>> + -re -wrap "NEXT OVER THIS RECURSION.*" {
>>> + set step_out 0
>>> + }
>>> + -re -wrap ".*" {
>>> + send_gdb "reverse-next\n"
>>> + exp_continue
>>> + }
>>> + }
>>> +}
>>> +
>>> +# Step forward over recursion again so we can test stepping over calls
>>> +# inside the recursion itself.
>>> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
>>> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
>>> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
>>> +
>>> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
>>> +set step_pass 1
>>> +gdb_test_multiple "next" "step over recursion inside the recursion" {
>>> + -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
>>> + set step_pass 0
>>> + send_gdb "next\n"
>>> + exp_continue
>>> + }
>>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>>> + if {$step_pass} {
>>> + pass "step over recursion inside the recursion"
>>> + } else {
>>> + fail "step over recursion inside the recursion"
>>> + }
>>> + }
>>> + -re -wrap ".*" {
>>> + send_gdb "next\n"
>>> + exp_continue
>>> + }
>>> +}
>>> +
>>> # step/next backward with count
>>> gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PINGv4] [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-06-22 18:49 ` [PINGv3] " Bruno Larsen
@ 2022-06-29 18:46 ` Bruno Larsen
2022-07-06 11:47 ` [PINGv5] " Bruno Larsen
0 siblings, 1 reply; 9+ messages in thread
From: Bruno Larsen @ 2022-06-29 18:46 UTC (permalink / raw)
To: gdb-patches
ping!
Cheers!
Bruno Larsen
On 6/22/22 15:49, Bruno Larsen wrote:
> ping!
>
> Cheers!
> Bruno Larsen
>
> On 6/15/22 16:21, Bruno Larsen wrote:
>> ping!
>>
>> Cheers!
>> Bruno Larsen
>>
>> On 6/8/22 08:17, Bruno Larsen wrote:
>>> Ping!
>>>
>>> Cheers!
>>> Bruno Larsen
>>>
>>> On 5/25/22 15:02, 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. Recursion poses a problem because that breakpoint will
>>>> be hit many times before GDB should actually stop the inferior. To fix
>>>> this issue, when the step_resume_breakpoint is hit (and GDB is executing
>>>> backwards), we analyze if the caller of the frame is the original frame
>>>> where we started the "reverse next", and if it is, GDB will stop the
>>>> inferior, otherwise GDB will just ignore the breakpoint.
>>>>
>>>> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
>>>> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
>>>> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
>>>> 3 files changed, 95 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>>> index 02c98b50c8c..95fb3227aa3 100644
>>>> --- a/gdb/infrun.c
>>>> +++ b/gdb/infrun.c
>>>> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
>>>> case BPSTAT_WHAT_STEP_RESUME:
>>>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>>>> + /* If we are executing backwards, we are doing a "next" and the
>>>> + current frame is not the same as where we started, the
>>>> + step_resume_breakpoint we have just hit has been added to the start
>>>> + of a function so we can skip the whle function. However, if we are
>>>> + skipping a recursive call, we only want to act as if we hit the
>>>> + breakpoint only when the caller of the current frame is the original
>>>> + frame we were single stepping from. */
>>>> + if (execution_direction == EXEC_REVERSE
>>>> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
>>>> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
>>>> + ecs->event_thread->control.step_stack_frame_id))
>>>> + {
>>>> + infrun_debug_printf ("We are not done with recursing yet");
>>>> + /* If the caller's ID is not the same as the starting frame, we
>>>> + can ignore this breakpoint. */
>>>> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
>>>> + ecs->event_thread->control.step_stack_frame_id))
>>>> + {
>>>> + keep_going (ecs);
>>>> + return;
>>>> + }
>>>> + } else
>>>> + infrun_debug_printf ("are we not recursing?");
>>>> +
>>>> delete_step_resume_breakpoint (ecs->event_thread);
>>>> if (ecs->event_thread->control.proceed_to_finish
>>>> && execution_direction == EXEC_REVERSE)
>>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
>>>> index aea2a98541d..a390ac2580c 100644
>>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
>>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
>>>> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
>>>> return myglob++; /* ARRIVED IN CALLEE */
>>>> } /* RETURN FROM CALLEE */
>>>> +/* We need to make this function take more than a single instruction
>>>> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
>>>> + step over a single-instruction function. */
>>>> +int recursive_callee (int val){
>>>> + if (val == 0) return 0;
>>>> + val /= 2;
>>>> + if (val>1)
>>>> + val++;
>>>> + return recursive_callee (val); /* RECURSIVE CALL */
>>>> +} /* EXIT RECURSIVE FUNCTION */
>>>> +
>>>> /* A structure which, we hope, will need to be passed using memcpy. */
>>>> struct rhomboidal {
>>>> int rather_large[100];
>>>> @@ -51,6 +62,9 @@ int main () {
>>>> y = y + 4;
>>>> z = z + 5; /* STEP TEST 2 */
>>>> + /* Test that next goes over recursive calls too */
>>>> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
>>>> +
>>>> /* Test that "next" goes over a call */
>>>> callee(); /* NEXT OVER THIS CALL */
>>>> @@ -60,7 +74,7 @@ int main () {
>>>> /* Test "stepi" */
>>>> a[5] = a[3] - a[4]; /* FINISH TEST */
>>>> callee(); /* STEPI TEST */
>>>> -
>>>> +
>>>> /* Test "nexti" */
>>>> callee(); /* NEXTI TEST */
>>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>> index 997b62604d5..4f56b4785ca 100644
>>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>>>> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>>>> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>>>> +# next through a recursive function call
>>>> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
>>>> +
>>>> # step over call
>>>> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>>>> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>>>> # step into call
>>>> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>>>> set test_message "stepi back from function call"
>>>> gdb_test_multiple "stepi" "$test_message" {
>>>> - -re "NEXTI TEST.*$gdb_prompt $" {
>>>> + -re -wrap "NEXTI TEST.*" {
>>>> pass "$test_message"
>>>> }
>>>> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
>>>> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>>>> ###
>>>> # Set reverse execution direction
>>>> -
>>>> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>>>> # stepi backward thru return and into a function
>>>> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>>>> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>>>> +# Dont reverse the execution direction yet, as we will need another
>>>> +# forward step after this test
>>>> +
>>>> +set step_out 0
>>>> +gdb_test_multiple "next" "reverse next over recursion" {
>>>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>>>> + pass "reverse next over recursion"
>>>> + }
>>>> + -re -wrap ".*RECURSIVE CALL.*" {
>>>> + fail "reverse next over recursion"
>>>> + set step_out 1
>>>> + }
>>>> +}
>>>> +if { "$step_out" == 1 } {
>>>> + gdb_test_multiple "next" "stepping out of recursion" {
>>>> + -re -wrap "NEXT OVER THIS RECURSION.*" {
>>>> + set step_out 0
>>>> + }
>>>> + -re -wrap ".*" {
>>>> + send_gdb "reverse-next\n"
>>>> + exp_continue
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +# Step forward over recursion again so we can test stepping over calls
>>>> +# inside the recursion itself.
>>>> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
>>>> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
>>>> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
>>>> +
>>>> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
>>>> +set step_pass 1
>>>> +gdb_test_multiple "next" "step over recursion inside the recursion" {
>>>> + -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
>>>> + set step_pass 0
>>>> + send_gdb "next\n"
>>>> + exp_continue
>>>> + }
>>>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>>>> + if {$step_pass} {
>>>> + pass "step over recursion inside the recursion"
>>>> + } else {
>>>> + fail "step over recursion inside the recursion"
>>>> + }
>>>> + }
>>>> + -re -wrap ".*" {
>>>> + send_gdb "next\n"
>>>> + exp_continue
>>>> + }
>>>> +}
>>>> +
>>>> # step/next backward with count
>>>> gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PINGv5] [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-06-29 18:46 ` [PINGv4] " Bruno Larsen
@ 2022-07-06 11:47 ` Bruno Larsen
0 siblings, 0 replies; 9+ messages in thread
From: Bruno Larsen @ 2022-07-06 11:47 UTC (permalink / raw)
To: gdb-patches
ping!
Cheers!
Bruno Larsen
On 6/29/22 15:46, Bruno Larsen wrote:
> ping!
>
> Cheers!
> Bruno Larsen
>
> On 6/22/22 15:49, Bruno Larsen wrote:
>> ping!
>>
>> Cheers!
>> Bruno Larsen
>>
>> On 6/15/22 16:21, Bruno Larsen wrote:
>>> ping!
>>>
>>> Cheers!
>>> Bruno Larsen
>>>
>>> On 6/8/22 08:17, Bruno Larsen wrote:
>>>> Ping!
>>>>
>>>> Cheers!
>>>> Bruno Larsen
>>>>
>>>> On 5/25/22 15:02, 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. Recursion poses a problem because that breakpoint will
>>>>> be hit many times before GDB should actually stop the inferior. To fix
>>>>> this issue, when the step_resume_breakpoint is hit (and GDB is executing
>>>>> backwards), we analyze if the caller of the frame is the original frame
>>>>> where we started the "reverse next", and if it is, GDB will stop the
>>>>> inferior, otherwise GDB will just ignore the breakpoint.
>>>>>
>>>>> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
>>>>> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
>>>>> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
>>>>> 3 files changed, 95 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>>>> index 02c98b50c8c..95fb3227aa3 100644
>>>>> --- a/gdb/infrun.c
>>>>> +++ b/gdb/infrun.c
>>>>> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
>>>>> case BPSTAT_WHAT_STEP_RESUME:
>>>>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>>>>> + /* If we are executing backwards, we are doing a "next" and the
>>>>> + current frame is not the same as where we started, the
>>>>> + step_resume_breakpoint we have just hit has been added to the start
>>>>> + of a function so we can skip the whle function. However, if we are
>>>>> + skipping a recursive call, we only want to act as if we hit the
>>>>> + breakpoint only when the caller of the current frame is the original
>>>>> + frame we were single stepping from. */
>>>>> + if (execution_direction == EXEC_REVERSE
>>>>> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
>>>>> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
>>>>> + ecs->event_thread->control.step_stack_frame_id))
>>>>> + {
>>>>> + infrun_debug_printf ("We are not done with recursing yet");
>>>>> + /* If the caller's ID is not the same as the starting frame, we
>>>>> + can ignore this breakpoint. */
>>>>> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
>>>>> + ecs->event_thread->control.step_stack_frame_id))
>>>>> + {
>>>>> + keep_going (ecs);
>>>>> + return;
>>>>> + }
>>>>> + } else
>>>>> + infrun_debug_printf ("are we not recursing?");
>>>>> +
>>>>> delete_step_resume_breakpoint (ecs->event_thread);
>>>>> if (ecs->event_thread->control.proceed_to_finish
>>>>> && execution_direction == EXEC_REVERSE)
>>>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
>>>>> index aea2a98541d..a390ac2580c 100644
>>>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
>>>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
>>>>> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
>>>>> return myglob++; /* ARRIVED IN CALLEE */
>>>>> } /* RETURN FROM CALLEE */
>>>>> +/* We need to make this function take more than a single instruction
>>>>> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
>>>>> + step over a single-instruction function. */
>>>>> +int recursive_callee (int val){
>>>>> + if (val == 0) return 0;
>>>>> + val /= 2;
>>>>> + if (val>1)
>>>>> + val++;
>>>>> + return recursive_callee (val); /* RECURSIVE CALL */
>>>>> +} /* EXIT RECURSIVE FUNCTION */
>>>>> +
>>>>> /* A structure which, we hope, will need to be passed using memcpy. */
>>>>> struct rhomboidal {
>>>>> int rather_large[100];
>>>>> @@ -51,6 +62,9 @@ int main () {
>>>>> y = y + 4;
>>>>> z = z + 5; /* STEP TEST 2 */
>>>>> + /* Test that next goes over recursive calls too */
>>>>> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
>>>>> +
>>>>> /* Test that "next" goes over a call */
>>>>> callee(); /* NEXT OVER THIS CALL */
>>>>> @@ -60,7 +74,7 @@ int main () {
>>>>> /* Test "stepi" */
>>>>> a[5] = a[3] - a[4]; /* FINISH TEST */
>>>>> callee(); /* STEPI TEST */
>>>>> -
>>>>> +
>>>>> /* Test "nexti" */
>>>>> callee(); /* NEXTI TEST */
>>>>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>>> index 997b62604d5..4f56b4785ca 100644
>>>>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>>>>> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>>>>> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>>>>> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>>>>> +# next through a recursive function call
>>>>> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
>>>>> +
>>>>> # step over call
>>>>> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>>>>> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>>>>> # step into call
>>>>> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>>>>> set test_message "stepi back from function call"
>>>>> gdb_test_multiple "stepi" "$test_message" {
>>>>> - -re "NEXTI TEST.*$gdb_prompt $" {
>>>>> + -re -wrap "NEXTI TEST.*" {
>>>>> pass "$test_message"
>>>>> }
>>>>> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
>>>>> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>>>>> ###
>>>>> # Set reverse execution direction
>>>>> -
>>>>> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>>>>> # stepi backward thru return and into a function
>>>>> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>>>>> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>>>>> +# Dont reverse the execution direction yet, as we will need another
>>>>> +# forward step after this test
>>>>> +
>>>>> +set step_out 0
>>>>> +gdb_test_multiple "next" "reverse next over recursion" {
>>>>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>>>>> + pass "reverse next over recursion"
>>>>> + }
>>>>> + -re -wrap ".*RECURSIVE CALL.*" {
>>>>> + fail "reverse next over recursion"
>>>>> + set step_out 1
>>>>> + }
>>>>> +}
>>>>> +if { "$step_out" == 1 } {
>>>>> + gdb_test_multiple "next" "stepping out of recursion" {
>>>>> + -re -wrap "NEXT OVER THIS RECURSION.*" {
>>>>> + set step_out 0
>>>>> + }
>>>>> + -re -wrap ".*" {
>>>>> + send_gdb "reverse-next\n"
>>>>> + exp_continue
>>>>> + }
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +# Step forward over recursion again so we can test stepping over calls
>>>>> +# inside the recursion itself.
>>>>> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
>>>>> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"
>>>>> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
>>>>> +
>>>>> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
>>>>> +set step_pass 1
>>>>> +gdb_test_multiple "next" "step over recursion inside the recursion" {
>>>>> + -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
>>>>> + set step_pass 0
>>>>> + send_gdb "next\n"
>>>>> + exp_continue
>>>>> + }
>>>>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>>>>> + if {$step_pass} {
>>>>> + pass "step over recursion inside the recursion"
>>>>> + } else {
>>>>> + fail "step over recursion inside the recursion"
>>>>> + }
>>>>> + }
>>>>> + -re -wrap ".*" {
>>>>> + send_gdb "next\n"
>>>>> + exp_continue
>>>>> + }
>>>>> +}
>>>>> +
>>>>> # step/next backward with count
>>>>> gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-05-25 18:02 [PATCH] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
2022-06-08 11:17 ` [PING] " Bruno Larsen
@ 2022-07-07 18:48 ` Pedro Alves
2022-07-08 17:16 ` Bruno Larsen
2022-07-22 15:31 ` Bruno Larsen
1 sibling, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2022-07-07 18:48 UTC (permalink / raw)
To: Bruno Larsen, gdb-patches
On 2022-05-25 19:02, 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. Recursion poses a problem because that breakpoint will
> be hit many times before GDB should actually stop the inferior. To fix
> this issue, when the step_resume_breakpoint is hit (and GDB is executing
> backwards), we analyze if the caller of the frame is the original frame
> where we started the "reverse next", and if it is, GDB will stop the
> inferior, otherwise GDB will just ignore the breakpoint.
>
> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
> 3 files changed, 95 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 02c98b50c8c..95fb3227aa3 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
> case BPSTAT_WHAT_STEP_RESUME:
> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>
> + /* If we are executing backwards, we are doing a "next" and the
> + current frame is not the same as where we started, the
> + step_resume_breakpoint we have just hit has been added to the start
> + of a function so we can skip the whle function. However, if we are
whle -> whole
> + skipping a recursive call, we only want to act as if we hit the
> + breakpoint only when the caller of the current frame is the original
> + frame we were single stepping from. */
This wording confused me, took me a while to understand what is meant. I suggest:
/* If we are executing a reverse next, and going backwards entered
a different frame, we've set a step_resume_breakpoint at the start
of the function so we can skip the whole function. However, if we are
skipping a recursive call, we want to ignore the breakpoint hit until
the caller of the current frame is the original frame we were nexting. */
However, see further below.
> + if (execution_direction == EXEC_REVERSE
> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
> + ecs->event_thread->control.step_stack_frame_id))
> + {
> + infrun_debug_printf ("We are not done with recursing yet");
> + /* If the caller's ID is not the same as the starting frame, we
> + can ignore this breakpoint. */
> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
> + ecs->event_thread->control.step_stack_frame_id))
> + {
> + keep_going (ecs);
> + return;
> + }
> + } else
> + infrun_debug_printf ("are we not recursing?");
This last else is odd -- we will print it if execution_direction != EXEC_REVERSE, for
example.
I also don't understand why do we need all these different frame checks as above. Breakpoints
can have a frame associated, so that when a breakpoint hits, the breakpoint.c stuff checks
whether the hit happened in the right frame, and if not, the breakpoint is ignored / set to
be auto-stepped-over, and BPSTAT_WHAT_STEP_RESUME isn't reached.
The actual problem seems to me that we create the step-resume breakpoint in
question without giving it a frame. If I do that instead of your fix, like so:
diff --git c/gdb/infrun.c w/gdb/infrun.c
index 02c98b50c8c..1360b8da70e 100644
--- c/gdb/infrun.c
+++ w/gdb/infrun.c
@@ -7118,8 +7118,8 @@ process_event_stop_test (struct execution_control_state *ecs)
symtab_and_line sr_sal;
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);
+ insert_step_resume_breakpoint_at_sal
+ (gdbarch, sr_sal, get_stack_frame_id (frame));
}
}
else
and then run your updated testcase, it passes. I also see no regressions in gdb.reverse/ tests.
WDYT? Are you seeing something that would break with this approach?
> +
> delete_step_resume_breakpoint (ecs->event_thread);
> if (ecs->event_thread->control.proceed_to_finish
> && execution_direction == EXEC_REVERSE)
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
> index aea2a98541d..a390ac2580c 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
> return myglob++; /* ARRIVED IN CALLEE */
> } /* RETURN FROM CALLEE */
>
> +/* We need to make this function take more than a single instruction
> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
> + step over a single-instruction function. */
> +int recursive_callee (int val){
> + if (val == 0) return 0;
> + val /= 2;
> + if (val>1)
> + val++;
> + return recursive_callee (val); /* RECURSIVE CALL */
> +} /* EXIT RECURSIVE FUNCTION */
> +
> /* A structure which, we hope, will need to be passed using memcpy. */
> struct rhomboidal {
> int rather_large[100];
> @@ -51,6 +62,9 @@ int main () {
> y = y + 4;
> z = z + 5; /* STEP TEST 2 */
>
> + /* Test that next goes over recursive calls too */
> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
> +
> /* Test that "next" goes over a call */
> callee(); /* NEXT OVER THIS CALL */
>
> @@ -60,7 +74,7 @@ int main () {
> /* Test "stepi" */
> a[5] = a[3] - a[4]; /* FINISH TEST */
> callee(); /* STEPI TEST */
> -
> +
> /* Test "nexti" */
> callee(); /* NEXTI TEST */
>
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index 997b62604d5..4f56b4785ca 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>
> +# next through a recursive function call
> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
> +
> # step over call
>
> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>
> # step into call
> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>
> set test_message "stepi back from function call"
> gdb_test_multiple "stepi" "$test_message" {
> - -re "NEXTI TEST.*$gdb_prompt $" {
> + -re -wrap "NEXTI TEST.*" {
> pass "$test_message"
> }
> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
> ###
>
> # Set reverse execution direction
> -
> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>
> # stepi backward thru return and into a function
> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>
> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>
> +# Dont reverse the execution direction yet, as we will need another
> +# forward step after this test
- Dont -> Don't
- Add missing period.
> +
> +set step_out 0
> +gdb_test_multiple "next" "reverse next over recursion" {
> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> + pass "reverse next over recursion"
> + }
> + -re -wrap ".*RECURSIVE CALL.*" {
> + fail "reverse next over recursion"
> + set step_out 1
> + }
> +}
Please use $gdb_test_name.
> +if { "$step_out" == 1 } {
> + gdb_test_multiple "next" "stepping out of recursion" {
"stepping out" -> "step out"
> + -re -wrap "NEXT OVER THIS RECURSION.*" {
> + set step_out 0
> + }
> + -re -wrap ".*" {
> + send_gdb "reverse-next\n"
> + exp_continue
> + }
> + }
> +}
No pass call? I guess I see -- this whole $step_out thing is to kind of
handle the previous test's failure? Why do we have this at all?
> +
> +# 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"
This "next" wasn't a reverse next.
> +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 $step_pass $gdb_test_name
> + }
> + -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"
I actually have to leave now, and haven't had a chance to finish looking at the testcase
in much detail. But figured it's best to press send anyhow. So... <send>.
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-07-07 18:48 ` Pedro Alves
@ 2022-07-08 17:16 ` Bruno Larsen
2022-07-22 15:31 ` Bruno Larsen
1 sibling, 0 replies; 9+ messages in thread
From: Bruno Larsen @ 2022-07-08 17:16 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 7/7/22 15:48, Pedro Alves wrote:
> On 2022-05-25 19:02, 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. Recursion poses a problem because that breakpoint will
>> be hit many times before GDB should actually stop the inferior. To fix
>> this issue, when the step_resume_breakpoint is hit (and GDB is executing
>> backwards), we analyze if the caller of the frame is the original frame
>> where we started the "reverse next", and if it is, GDB will stop the
>> inferior, otherwise GDB will just ignore the breakpoint.
>>
>> This commit also changes gdb.reverse/step-reverse.exp 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 | 24 +++++++++
>> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
>> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
>> 3 files changed, 95 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 02c98b50c8c..95fb3227aa3 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
>> case BPSTAT_WHAT_STEP_RESUME:
>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>>
>> + /* If we are executing backwards, we are doing a "next" and the
>> + current frame is not the same as where we started, the
>> + step_resume_breakpoint we have just hit has been added to the start
>> + of a function so we can skip the whle function. However, if we are
>
> whle -> whole
>
>> + skipping a recursive call, we only want to act as if we hit the
>> + breakpoint only when the caller of the current frame is the original
>> + frame we were single stepping from. */
>
> This wording confused me, took me a while to understand what is meant. I suggest:
>
> /* If we are executing a reverse next, and going backwards entered
> a different frame, we've set a step_resume_breakpoint at the start
> of the function so we can skip the whole function. However, if we are
> skipping a recursive call, we want to ignore the breakpoint hit until
> the caller of the current frame is the original frame we were nexting. */
>
> However, see further below.
>
>> + if (execution_direction == EXEC_REVERSE
>> + && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
>> + && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
>> + ecs->event_thread->control.step_stack_frame_id))
>> + {
>> + infrun_debug_printf ("We are not done with recursing yet");
>> + /* If the caller's ID is not the same as the starting frame, we
>> + can ignore this breakpoint. */
>> + if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
>> + ecs->event_thread->control.step_stack_frame_id))
>> + {
>> + keep_going (ecs);
>> + return;
>> + }
>> + } else
>> + infrun_debug_printf ("are we not recursing?");
>
> This last else is odd -- we will print it if execution_direction != EXEC_REVERSE, for
> example.
Oops, this was for my own debugging, wasn't meant to be sent
>
> I also don't understand why do we need all these different frame checks as above. Breakpoints
> can have a frame associated, so that when a breakpoint hits, the breakpoint.c stuff checks
> whether the hit happened in the right frame, and if not, the breakpoint is ignored / set to
> be auto-stepped-over, and BPSTAT_WHAT_STEP_RESUME isn't reached.
>
> The actual problem seems to me that we create the step-resume breakpoint in
> question without giving it a frame. If I do that instead of your fix, like so:
>
> diff --git c/gdb/infrun.c w/gdb/infrun.c
> index 02c98b50c8c..1360b8da70e 100644
> --- c/gdb/infrun.c
> +++ w/gdb/infrun.c
> @@ -7118,8 +7118,8 @@ process_event_stop_test (struct execution_control_state *ecs)
> symtab_and_line sr_sal;
> 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);
> + insert_step_resume_breakpoint_at_sal
> + (gdbarch, sr_sal, get_stack_frame_id (frame));
> }
> }
> else
>
> and then run your updated testcase, it passes. I also see no regressions in gdb.reverse/ tests.
Wait... GDB can _do_ that? Well, this makes the solution much easier! Sorry for making you
re-do my solution. This looks like a much better solution
>
> WDYT? Are you seeing something that would break with this approach?
>
>> +
>> delete_step_resume_breakpoint (ecs->event_thread);
>> if (ecs->event_thread->control.proceed_to_finish
>> && execution_direction == EXEC_REVERSE)
>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
>> index aea2a98541d..a390ac2580c 100644
>> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
>> @@ -26,6 +26,17 @@ int callee() { /* ENTER CALLEE */
>> return myglob++; /* ARRIVED IN CALLEE */
>> } /* RETURN FROM CALLEE */
>>
>> +/* We need to make this function take more than a single instruction
>> + to run, otherwise it could hide PR gdb/16678, as reverse execution can
>> + step over a single-instruction function. */
>> +int recursive_callee (int val){
>> + if (val == 0) return 0;
>> + val /= 2;
>> + if (val>1)
>> + val++;
>> + return recursive_callee (val); /* RECURSIVE CALL */
>> +} /* EXIT RECURSIVE FUNCTION */
>> +
>> /* A structure which, we hope, will need to be passed using memcpy. */
>> struct rhomboidal {
>> int rather_large[100];
>> @@ -51,6 +62,9 @@ int main () {
>> y = y + 4;
>> z = z + 5; /* STEP TEST 2 */
>>
>> + /* Test that next goes over recursive calls too */
>> + recursive_callee (32); /* NEXT OVER THIS RECURSION */
>> +
>> /* Test that "next" goes over a call */
>> callee(); /* NEXT OVER THIS CALL */
>>
>> @@ -60,7 +74,7 @@ int main () {
>> /* Test "stepi" */
>> a[5] = a[3] - a[4]; /* FINISH TEST */
>> callee(); /* STEPI TEST */
>> -
>> +
>> /* Test "nexti" */
>> callee(); /* NEXTI TEST */
>>
>> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
>> index 997b62604d5..4f56b4785ca 100644
>> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
>> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
>> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>> gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>> gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>>
>> +# next through a recursive function call
>> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
>> +
>> # step over call
>>
>> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>> gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>>
>> # step into call
>> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>>
>> set test_message "stepi back from function call"
>> gdb_test_multiple "stepi" "$test_message" {
>> - -re "NEXTI TEST.*$gdb_prompt $" {
>> + -re -wrap "NEXTI TEST.*" {
>> pass "$test_message"
>> }
>> -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
>> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>> ###
>>
>> # Set reverse execution direction
>> -
>> gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>>
>> # stepi backward thru return and into a function
>> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>>
>> gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>>
>> +# Dont reverse the execution direction yet, as we will need another
>> +# forward step after this test
>
> - Dont -> Don't
>
> - Add missing period.
>
>
>> +
>> +set step_out 0
>> +gdb_test_multiple "next" "reverse next over recursion" {
>> + -re -wrap ".*NEXT OVER THIS RECURSION.*" {
>> + pass "reverse next over recursion"
>> + }
>> + -re -wrap ".*RECURSIVE CALL.*" {
>> + fail "reverse next over recursion"
>> + set step_out 1
>> + }
>> +}
>
> Please use $gdb_test_name.
>
>> +if { "$step_out" == 1 } {
>> + gdb_test_multiple "next" "stepping out of recursion" {
>
> "stepping out" -> "step out"
>
>> + -re -wrap "NEXT OVER THIS RECURSION.*" {
>> + set step_out 0
>> + }
>> + -re -wrap ".*" {
>> + send_gdb "reverse-next\n"
>> + exp_continue
>> + }
>> + }
>> +}
>
> No pass call? I guess I see -- this whole $step_out thing is to kind of
> handle the previous test's failure? Why do we have this at all?
This is used to re-sync GDB, in case the previous test failed. I didn't want
to add a pass because I wanted the expected passes to decrease if we got the
failure.
I can add the pass if you think it is important, but I will definitely document
why this is here.
>
>> +
>> +# 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"
>
> This "next" wasn't a reverse next.
>
>> +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 $step_pass $gdb_test_name
>
>> + }
>> + -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"
>
> I actually have to leave now, and haven't had a chance to finish looking at the testcase
> in much detail. But figured it's best to press send anyhow. So... <send>.
It really was! I'll take a second look over my changes and send a v2 soon, fixing everything
I haven't responded to.
Cheers!
Bruno Larsen
>
> Pedro Alves
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb/reverse: Fix stepping over recursive functions
2022-07-07 18:48 ` Pedro Alves
2022-07-08 17:16 ` Bruno Larsen
@ 2022-07-22 15:31 ` Bruno Larsen
1 sibling, 0 replies; 9+ messages in thread
From: Bruno Larsen @ 2022-07-22 15:31 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 7/7/22 15:48, Pedro Alves wrote:
> I also don't understand why do we need all these different frame checks as above. Breakpoints
> can have a frame associated, so that when a breakpoint hits, the breakpoint.c stuff checks
> whether the hit happened in the right frame, and if not, the breakpoint is ignored / set to
> be auto-stepped-over, and BPSTAT_WHAT_STEP_RESUME isn't reached.
>
> The actual problem seems to me that we create the step-resume breakpoint in
> question without giving it a frame. If I do that instead of your fix, like so:
>
> diff --git c/gdb/infrun.c w/gdb/infrun.c
> index 02c98b50c8c..1360b8da70e 100644
> --- c/gdb/infrun.c
> +++ w/gdb/infrun.c
> @@ -7118,8 +7118,8 @@ process_event_stop_test (struct execution_control_state *ecs)
> symtab_and_line sr_sal;
> 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);
> + insert_step_resume_breakpoint_at_sal
> + (gdbarch, sr_sal, get_stack_frame_id (frame));
> }
> }
> else
>
> and then run your updated testcase, it passes. I also see no regressions in gdb.reverse/ tests.
>
> WDYT? Are you seeing something that would break with this approach?
Hi Pedro
I have spent a while attempting to use your suggestion, but I did find something that breaks with this patch.
gdb.reverse/solib-precsave.exp shows 10 new failures. Here's a snippet before your change:
record restore /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/outputs/gdb.reverse/solib-precsave/solib.precsave
Warning: 'record restore', an alias for the command 'record full restore', is deprecated.
Use 'record full restore'.
[New LWP 22073]
Core was generated by `/home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/outputs/gdb.rever'.
Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0 main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:27
27 char* cptr = "String 1";
Restored records from core file /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/outputs/gdb.reverse/solib-precsave/solib.precsave.
#0 main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:27
27 char* cptr = "String 1";
(gdb) PASS: gdb.reverse/solib-precsave.exp: reload core file
until 46
main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:46
46 return 0; /* end part one */
(gdb) PASS: gdb.reverse/solib-precsave.exp: run until end part one
reverse-step
44 shr1 ("message 3\n"); /* shr1 three */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step third shr1
reverse-step
43 shr1 ("message 2\n"); /* shr1 two */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step second shr1
reverse-step
42 shr1 ("message 1\n"); /* shr1 one */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step first shr1
reverse-step
40 b[0] = 6; b[1] = 9; /* generic statement, end part two */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step generic
until 46
main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:46
46 return 0; /* end part one */
(gdb) PASS: gdb.reverse/solib-precsave.exp: forward to end part one
reverse-next
44 shr1 ("message 3\n"); /* shr1 three */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-next third shr1
and after your change:
record restore /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/outputs/gdb.reverse/solib-precsave/solib.precsave
Warning: 'record restore', an alias for the command 'record full restore', is deprecated.
Use 'record full restore'.
[New LWP 21721]
Core was generated by `/home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/outputs/gdb.rever'.
Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0 main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:27
27 char* cptr = "String 1";
Restored records from core file /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/outputs/gdb.reverse/solib-precsave/solib.precsave.
#0 main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:27
27 char* cptr = "String 1";
(gdb) PASS: gdb.reverse/solib-precsave.exp: reload core file
until 46
main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:46
46 return 0; /* end part one */
(gdb) PASS: gdb.reverse/solib-precsave.exp: run until end part one
reverse-step
44 shr1 ("message 3\n"); /* shr1 three */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step third shr1
reverse-step
43 shr1 ("message 2\n"); /* shr1 two */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step second shr1
reverse-step
42 shr1 ("message 1\n"); /* shr1 one */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step first shr1
reverse-step
40 b[0] = 6; b[1] = 9; /* generic statement, end part two */
(gdb) PASS: gdb.reverse/solib-precsave.exp: reverse-step generic
until 46
main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:46
46 return 0; /* end part one */
(gdb) PASS: gdb.reverse/solib-precsave.exp: forward to end part one
reverse-next
No more reverse-execution history.
main () at /home/blarsen/git-pool/git-repos/gdb-local-build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c:27
27 char* cptr = "String 1";
(gdb) FAIL: gdb.reverse/solib-precsave.exp: reverse-next third shr1
And a few more reverse-next fails after this. I haven't made much progress in debugging this issue because it seems that the whole record restore
has somewhat bit-rotted, as trying to restore a recording from another GDB session results in internal errors, so I wonder if this test is still
relevant, or if we can add these failures as "FIXME" since the whole feature is having issues. DO you have any thoughts on this?
Cheers!
Bruno Larsen
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-22 15:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-25 18:02 [PATCH] gdb/reverse: Fix stepping over recursive functions Bruno Larsen
2022-06-08 11:17 ` [PING] " Bruno Larsen
2022-06-15 19:21 ` [PINGv2] " Bruno Larsen
2022-06-22 18:49 ` [PINGv3] " Bruno Larsen
2022-06-29 18:46 ` [PINGv4] " Bruno Larsen
2022-07-06 11:47 ` [PINGv5] " Bruno Larsen
2022-07-07 18:48 ` Pedro Alves
2022-07-08 17:16 ` Bruno Larsen
2022-07-22 15:31 ` 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).