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 F311A38A8150 for ; Mon, 12 Sep 2022 16:57:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F311A38A8150 Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-608-Y5BlQFxRMTaOmOkK58x_Sw-1; Mon, 12 Sep 2022 12:57:43 -0400 X-MC-Unique: Y5BlQFxRMTaOmOkK58x_Sw-1 Received: by mail-wm1-f70.google.com with SMTP id i129-20020a1c3b87000000b003b33e6160bdso6155221wma.7 for ; Mon, 12 Sep 2022 09:57:42 -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:to:from :x-gm-message-state:from:to:cc:subject:date; bh=R0pN8ShwRePaS9xCEscH8Zho4V7eBG4jYoD0OiajjS8=; b=2IjSJEksIZNypq0e0iA9i/QuRyGnzAWi2jQquII3r7lBhsJeSuvh7Zn+DibdmZ7yY1 mGWptkBuDz5A+lHgLeIwG7VX0eiEf1b3vx1SGa/4eEucQeWdEX3z7QN2js7hmVp6eDZI cL4sFr6n/IcNcp5YTWeqeHsbNbnho2D5pWj3DiNRahgP2OwfCEC370GcOZN4NtL9TRmW 9K4pktprEb7FLDiIwwUefDK5MTuRExBMHNM5dgvEASSZzceAQ1n5TPUklPNHvbPpiVBu AuhAXO+nrVwhp6n6lDlyQq64o0aBtIQikTAHXw5BkMsgTeXz7N89V0b0+SwUiL1rVhfa 5BYw== X-Gm-Message-State: ACgBeo2ZPehCgP0gYMVV8WcNk/PMeCBs9y4mYJYSVpdvdD/SJjZfCmqf opSPL79tOKG5QSDtiK3xT9t+/2bhifmsm7SASLKWb74yIHkVb8ze0tozjO5Hzgl8dFnAVARhCYJ 9qqqPSjgEwvis8jjlrZ4c6Q== X-Received: by 2002:adf:e585:0:b0:22a:47d8:352f with SMTP id l5-20020adfe585000000b0022a47d8352fmr7147765wrm.193.1663001861962; Mon, 12 Sep 2022 09:57:41 -0700 (PDT) X-Google-Smtp-Source: AA6agR6h+sOXV3cqCWHHMPN6qLTiMl4Pxjq+EA7T8HHMDmrWALKUFu4oOyKmqOkgZ7iBkuIAgL5qlw== X-Received: by 2002:adf:e585:0:b0:22a:47d8:352f with SMTP id l5-20020adfe585000000b0022a47d8352fmr7147749wrm.193.1663001861675; Mon, 12 Sep 2022 09:57:41 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id fc11-20020a05600c524b00b003b48dac344esm3437890wmb.43.2022.09.12.09.57.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Sep 2022 09:57:41 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH v4 14/15] gdb.base/skip.exp: Use finish to exit functions In-Reply-To: <20220720194441.168906-16-blarsen@redhat.com> References: <20220720194441.168906-1-blarsen@redhat.com> <20220720194441.168906-16-blarsen@redhat.com> Date: Mon, 12 Sep 2022 17:57:40 +0100 Message-ID: <87wna8jzsr.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, 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: Mon, 12 Sep 2022 16:57:46 -0000 Bruno Larsen via Gdb-patches writes: > gdb.base/skip.exp was making use of a fixed amount of step commands to I think s/amount/number/ - amount is for uncountable things, while the number of steps used can be counted. > exit some functions. This caused some problems when using clang to test > GDB, as GDB would need fewer steps to reach the desired spots. For > instance, when testing in the section "step after disabling 3", the log > looks like this: > > Breakpoint 4, main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32 > 32 x = baz ((bar (), foo ())); > (gdb) step > bar () at binutils-gdb/gdb/testsuite/gdb.base/skip1.c:21 > 21 return 1; > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step 1 > step > foo () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:42 > 42 return 0; > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step 2 > step > main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:34 > 34 test_skip_file_and_function (); > (gdb) step > test_skip_file_and_function () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:59 > 59 test_skip (); > (gdb) FAIL: gdb.base/skip.exp: step after disabling 3: step 3 > step > test_skip () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:48 > 48 } > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step 4 > step > test_skip_file_and_function () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:60 > 60 skip1_test_skip_file_and_function (); > (gdb) FAIL: gdb.base/skip.exp: step after disabling 3: step 5 > > This shows that the feature is working, but it is not easy to use steps > to test this feature without analyzing all possible outputs, such as > using gdb_step_until_regexp. Instead, skip.exp now uses finish to leave You mention gdb_step_until_regexp here, which I think is now named just gdb_step_until. But more importantly, you mention it, but then don't actually use this, and its not clear why (from this commit message). I think it's worth pointing out here that we can't use gdb_step_until because it would potentially step over the very thing we are trying to ensure that GDB doesn't do, i.e. call into the skipped function. > functions, synchronizing through compilers and compiler versions. Some > test names were also changed to be a bit more descriptive. The new log > looks like this, independently of compiler used: > > Breakpoint 4, main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32 > 32 x = baz ((bar (), foo ())); > (gdb) step > bar () at binutils-gdb/gdb/testsuite/gdb.base/skip1.c:21 > 21 return 1; > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step into bar > finish > Run till exit from #0 bar () at binutils-gdb/gdb/testsuite/gdb.base/skip1.c:21 > main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32 > 32 x = baz ((bar (), foo ())); > Value returned is $2 = 1 > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: return from bar > step > foo () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:42 > 42 return 0; > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step into foo > finish > Run till exit from #0 foo () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:42 > main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32 > 32 x = baz ((bar (), foo ())); > Value returned is $3 = 0 > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: Return from foo > step > 34 test_skip_file_and_function (); > (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step and skip baz > --- > gdb/testsuite/gdb.base/skip.exp | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp > index e6b660004d9..5b08cde93b7 100644 > --- a/gdb/testsuite/gdb.base/skip.exp > +++ b/gdb/testsuite/gdb.base/skip.exp > @@ -100,6 +100,12 @@ if ![runto_main] { > gdb_test "step" ".*" "step in the main" > gdb_test "bt" "\\s*\\#0\\s+main.*" "step after all ignored" > > +proc step_foo_skip_bar_baz {} { I think this proc should have a comment describing what it's doing. > + gdb_test "step" "foo \\(\\) at.*" "step and skip bar" > + gdb_test "finish" ".*" "return from bar" > + gdb_test "step" ".*test_skip_file_and_function.*" "step and skip baz" > +} > + > # Now remove skip.c from the skiplist. Our first step should take us > # into foo(), and our second step should take us to the next line in main(). > > @@ -117,21 +123,19 @@ with_test_prefix "step after deleting 1" { > return > } > > - gdb_test "step" "foo \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2" ; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 3" > + step_foo_skip_bar_baz > } > > # Test that we step into foo(), then into bar(), but not into baz(). > proc step_bar_foo_skip_baz {} { > - gdb_test "step" "bar \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from bar() > + gdb_test "step" "bar \\(\\) at.*" "step into bar" > + gdb_test "finish" ".*" "return from bar" > > # With at least gcc 6.5.0 and 9.2.0, we jump once back to main > # before entering foo here. If that happens try to step a second > # time. > set stepped_again 0 > - gdb_test_multiple "step" "step 3" { > + gdb_test_multiple "step" "step into foo" { > -re -wrap "foo \\(\\) at.*" { > pass $gdb_test_name > } > @@ -144,8 +148,8 @@ proc step_bar_foo_skip_baz {} { > } > } > > - gdb_test "step" ".*" "step 4"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 5" > + gdb_test "finish" ".*" "Return from foo" > + gdb_test "step" ".*test_skip_file_and_function.*" "step and skip baz" > } > > # Now disable the skiplist entry for skip1.c. We should now > @@ -178,9 +182,7 @@ with_test_prefix "step after enable 3" { > return > } > > - gdb_test "step" "foo \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 3" > + step_foo_skip_bar_baz > } > > # Admin tests (disable,enable,delete). > @@ -249,9 +251,7 @@ with_test_prefix "step using -fi" { > > gdb_test_no_output "skip disable" > gdb_test_no_output "skip enable 5" > - gdb_test "step" "foo \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 3" > + step_foo_skip_bar_baz > } > > with_test_prefix "step using -gfi" { > @@ -261,9 +261,7 @@ with_test_prefix "step using -gfi" { > > gdb_test_no_output "skip disable" > gdb_test_no_output "skip enable 6" > - gdb_test "step" "foo \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 3" > + step_foo_skip_bar_baz > } In an earlier version of this patch Pedro raised the concern that if we remove all of the "step out of a function" tests, then we may remove some important testing. With the patch #15 in this series (once that's merged), I think it will be OK to change this test as you propose. Thanks, Andrew > > with_test_prefix "step using -fu for baz" { > -- > 2.31.1