From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 6821C38582BA for ; Fri, 8 Jul 2022 17:16:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6821C38582BA Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-660-RGfOw1IlPCWiHpuzRkYIpA-1; Fri, 08 Jul 2022 13:16:07 -0400 X-MC-Unique: RGfOw1IlPCWiHpuzRkYIpA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9BD8929AA386; Fri, 8 Jul 2022 17:16:07 +0000 (UTC) Received: from [10.97.116.37] (ovpn-116-37.gru2.redhat.com [10.97.116.37]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C666F2026D64; Fri, 8 Jul 2022 17:16:06 +0000 (UTC) Message-ID: <8e0e76a0-4bc2-fd4e-25d9-f8ed71e67522@redhat.com> Date: Fri, 8 Jul 2022 14:16:05 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] gdb/reverse: Fix stepping over recursive functions To: Pedro Alves , gdb-patches@sourceware.org References: <20220525180247.29731-1-blarsen@redhat.com> <4883e160-bb4a-6578-1bfd-308646144aef@palves.net> From: Bruno Larsen In-Reply-To: <4883e160-bb4a-6578-1bfd-308646144aef@palves.net> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Jul 2022 17:16:11 -0000 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... . 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 >