From: Bruno Larsen <blarsen@redhat.com>
To: Pedro Alves <pedro@palves.net>, gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb/reverse: Fix stepping over recursive functions
Date: Fri, 8 Jul 2022 14:16:05 -0300 [thread overview]
Message-ID: <8e0e76a0-4bc2-fd4e-25d9-f8ed71e67522@redhat.com> (raw)
In-Reply-To: <4883e160-bb4a-6578-1bfd-308646144aef@palves.net>
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
>
next prev parent reply other threads:[~2022-07-08 17:16 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-25 18:02 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 [this message]
2022-07-22 15:31 ` Bruno Larsen
-- strict thread matches above, loose matches on Subject: below --
2022-05-23 13:46 Bruno Larsen
2022-05-23 20:03 ` Bruno Larsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e0e76a0-4bc2-fd4e-25d9-f8ed71e67522@redhat.com \
--to=blarsen@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).