From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by sourceware.org (Postfix) with ESMTPS id 74A3838582B0 for ; Thu, 7 Jul 2022 18:48:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 74A3838582B0 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f48.google.com with SMTP id r14so21784106wrg.1 for ; Thu, 07 Jul 2022 11:48:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=T685/YJI32hkQoN88tLbyeDpFk4J8Fej5TnGJb55UH8=; b=4VMiilWrEsPVQlmgYGAX0qnXofPrPJuqQisOi5gF9Hj7cJeNtLvBKO4SVtWZPrsO+p udPDqQU/73hnbToFt49vht9iKE4MC0PxWVTrMxSOR/Qdf7LYvWhkB6hY59fwxivP6EUT /JdV1tJN48cuoUozGqE8/ilPpYn1FX5GgzGkrh6ZrhfmAzG0IUgQXDocIOUK5B/rhJKa wmuKNp2JJ4eHXSsQSWhcnRQL13yyQVeBJeETOv+Ou0tW6A6yrFu6fcRpG19MCKBnPEom 3ELCsPwotpFdA1eiBEybLXwGZifa8UWS6lmn9Ozghn8wJ1WwXvtNgY652s56DkRJGjCL RnCA== X-Gm-Message-State: AJIora+tFjmcrXnYmbNrVMqkz4Y0N4L2Htj9liK/GXiMqaOiVC05+FYO QK4zi2G0KA+Xt3U0fHaUDfPnlhrlF3g= X-Google-Smtp-Source: AGRyM1tRH3oBWGXiphlENjZDzuEZ28CS3iCt6Kh3NPGs/FWC7AlRCFkZjxSHzy0sUfu2TMcK4hr8Ig== X-Received: by 2002:a5d:64e4:0:b0:21b:940a:a002 with SMTP id g4-20020a5d64e4000000b0021b940aa002mr44111090wri.360.1657219702254; Thu, 07 Jul 2022 11:48:22 -0700 (PDT) Received: from ?IPv6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id d3-20020a05600c34c300b003a2c67aa6c0sm4832377wmq.23.2022.07.07.11.48.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 07 Jul 2022 11:48:21 -0700 (PDT) From: Pedro Alves Subject: Re: [PATCH] gdb/reverse: Fix stepping over recursive functions To: Bruno Larsen , gdb-patches@sourceware.org References: <20220525180247.29731-1-blarsen@redhat.com> Message-ID: <4883e160-bb4a-6578-1bfd-308646144aef@palves.net> Date: Thu, 7 Jul 2022 19:48:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220525180247.29731-1-blarsen@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, 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: Thu, 07 Jul 2022 18:48:27 -0000 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... . Pedro Alves