From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id MnshLXEqBmOKlSwAWB0awg (envelope-from ) for ; Wed, 24 Aug 2022 09:41:05 -0400 Received: by simark.ca (Postfix, from userid 112) id AE9081E4A7; Wed, 24 Aug 2022 09:41:05 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=C9UlT0MK; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id AB0CB1E13B for ; Wed, 24 Aug 2022 09:41:01 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2D7AB385C32E for ; Wed, 24 Aug 2022 13:41:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2D7AB385C32E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1661348460; bh=b5i1AdsYB83GYrnSmcvps0EN+7nO7H+nxC6siI4i+hQ=; h=To:Subject:In-Reply-To:References:Date:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=C9UlT0MKUDZOr+j0Zta2sV0k9rBXZZ06PSyfdvxXxF8kBwUMrQ6UeI1v0Y6PlhDt7 ZZRh/rFTkg0YL16KZKlCxTGAMBMeKA/w9LolUKR5t2zLXU0gioPrGEKOKohhZrboGw FbLCn1bRcWpb2VEGxkT6j4KKmN5Y25mvSHLscxSE= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 504B2385843D for ; Wed, 24 Aug 2022 13:40:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 504B2385843D Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-21-70AYNrt-MBaXNWj1cdnjCg-1; Wed, 24 Aug 2022 09:40:36 -0400 X-MC-Unique: 70AYNrt-MBaXNWj1cdnjCg-1 Received: by mail-wr1-f69.google.com with SMTP id v19-20020adf8b53000000b00225728573e6so301591wra.21 for ; Wed, 24 Aug 2022 06:40:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc; bh=b5i1AdsYB83GYrnSmcvps0EN+7nO7H+nxC6siI4i+hQ=; b=b1X4Cnai/RDN09sUhryPuLetlfj/DEBcsW3lNLINdZ4NpzXqvP/vGo+negNNw3jSEa ZDQsXzxgTG1h/BOB54dFRy0yuipNXhlAl7hJUZstwjGviBTnXeI4qOluIPPpTcdPqjwx tVVc0YYv8qSUGPgaIouqulo+9hm7j3JFg8/CqCW1gewIvio0KEBQcZZbDnWcr/A/qMms Jh2IU/5F28PobsF3QHLrp8xBewlSvIHkPdleSJs6x1xBJ3apiw6Ed+B93XHve9agQefS j2CGySF5XqwJknZf/Jhedq6vxG9GTnTfNgNGf5vByrD4p64RbXyYJQP1zGtKSRrB4V5+ tukg== X-Gm-Message-State: ACgBeo0TNzmdOwe9njnwKzviiUmiLWVL/lb4Zqn+Cgn+PpMUcXkWbQ2z EnzlrhMdx9/GEh5K2t2wjt+L5eRvozHP+33ImMqWelCvkGFgBV8TjrlXhYKprlPuCaSwVP5nk+E AIweBI0wf/0uu6azSFRSr/g== X-Received: by 2002:a05:600c:582:b0:3a5:4f7b:3146 with SMTP id o2-20020a05600c058200b003a54f7b3146mr5368241wmd.152.1661348435569; Wed, 24 Aug 2022 06:40:35 -0700 (PDT) X-Google-Smtp-Source: AA6agR65wbo+vVsFNwP3YW1/Kk8wW6DUaKU0UL3To9TgOSK3EqVL72wq/uhnfywl0CQ0hk69H5EPyg== X-Received: by 2002:a05:600c:582:b0:3a5:4f7b:3146 with SMTP id o2-20020a05600c058200b003a54f7b3146mr5368227wmd.152.1661348435295; Wed, 24 Aug 2022 06:40:35 -0700 (PDT) Received: from localhost ([31.111.84.229]) by smtp.gmail.com with ESMTPSA id l16-20020a05600c1d1000b003a61306d79dsm2866181wms.41.2022.08.24.06.40.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Aug 2022 06:40:34 -0700 (PDT) To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH v2 2/2] gdb/reverse: Fix stepping over recursive functions In-Reply-To: <20220823142204.31659-3-blarsen@redhat.com> References: <20220823142204.31659-1-blarsen@redhat.com> <20220823142204.31659-3-blarsen@redhat.com> Date: Wed, 24 Aug 2022 14:40:33 +0100 Message-ID: <87h721ep4e.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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: , From: Andrew Burgess via Gdb-patches Reply-To: Andrew Burgess Cc: pedro@palves.net Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Bruno Larsen via Gdb-patches writes: > Currently, when using GDB to do reverse debugging, if we try to use the > command "reverse next" to skip a recursive function, instead of skipping > all of the recursive calls and stopping in the previous line, we stop at > the second to last recursive call, and need to manually step backwards > until we leave the first call. This is well documented in PR gdb/16678. > > This bug happens because when GDB notices that a reverse step has > entered into a function, GDB will add a step_resume_breakpoint at the > start of the function, then single step out of the prologue once that > breakpoint is hit. The problem was happening because GDB wouldn't give > that step_resume_breakpoint a frame-id, so the first time the breakpoint > was hit, the inferior would be stopped. This is fixed by giving the > current frame-id to the breakpoint. > > This commit also changes gdb.reverse/step-reverse.c to contain a > recursive function and attempt to both, skip it altogether, and to skip > the second call from inside the first call, as this setup broke a > previous version of the patch. Thanks for this. The GDB fix looks fine. I had a couple of really minor style nits relating to the test, see inline below. > --- > gdb/infrun.c | 2 +- > gdb/testsuite/gdb.reverse/step-precsave.exp | 6 ++- > gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++- > gdb/testsuite/gdb.reverse/step-reverse.exp | 59 +++++++++++++++++++-- > 4 files changed, 76 insertions(+), 7 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 033699bc3f7..679a0c83ece 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7133,7 +7133,7 @@ process_event_stop_test (struct execution_control_state *ecs) > sr_sal.pc = ecs->stop_func_start; > sr_sal.pspace = get_frame_program_space (frame); > insert_step_resume_breakpoint_at_sal (gdbarch, > - sr_sal, null_frame_id); > + sr_sal, get_stack_frame_id (frame)); > } > } > else > diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp > index 0836ed2629f..54df9209c96 100644 > --- a/gdb/testsuite/gdb.reverse/step-precsave.exp > +++ b/gdb/testsuite/gdb.reverse/step-precsave.exp > @@ -86,7 +86,8 @@ gdb_test "step 3" ".*STEP TEST 2.*" "step test 2" > > # step over call > > -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call" > +gdb_test "step" ".*NEXT OVER THIS RECURSION.*" "step up to call" > +gdb_test "next" ".*NEXT OVER THIS CALL.*" "skip recursive call" > gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call" > > # step into call > @@ -280,9 +281,10 @@ gdb_test_multiple "step" "$test_message" { > } > } > > -# next backward over call > +# next backward over calls > > gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call" > +gdb_test "next" ".*NEXT OVER THIS RECURSION.*" "reverse next over recursive call" > > # step/next backward with count > > diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c > index aea2a98541d..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) Unless required by the test, this code should follow GNU coding standard, so int recursive_callee (int val) { and if (val > 1) If the non-standard layout is required, then the comment above the function should explain why. > + 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 Comments should be proper sentences, so capital 'N', and end with '.'. > +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 Missing '.'. > + > +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" You can replace the strings used with pass/fail with the magic variable $gdb_test_name. This variable is magically set within the code blocks of a gdb_test_multiple to the name of the test. Thanks, Andrew > + } > + } > + -re -wrap ".*" { > + send_gdb "next\n" > + exp_continue > + } > +} > + > # step/next backward with count > > gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1" > -- > 2.37.2