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 DAD383858D35 for ; Fri, 25 Feb 2022 17:00:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DAD383858D35 Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-322-SHj8yTAHMimQKiDuN6uepg-1; Fri, 25 Feb 2022 12:00:41 -0500 X-MC-Unique: SHj8yTAHMimQKiDuN6uepg-1 Received: by mail-wm1-f72.google.com with SMTP id z15-20020a1c4c0f000000b00380d331325aso1577483wmf.6 for ; Fri, 25 Feb 2022 09:00:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=5m/r/fSfTmpc+yBkkKdrXVel1gu6vUe1iRDhLRhOOHk=; b=Vf9e/6LQBttsUQFGCTw97rKxzU4eW7fpd4UbdrJTADO/zoU+PA5pqRUWIWOxVJO/Y8 tlrxpKklpxJpMkvoIW2pEj6lqBTqcVc5Nt1PDNloAj2VsiynkbGFyCwrvJC84AnoLhq3 q5sHKae0M22qydP9tmu8Su66SH+3RXX15runrhPlLO1i6EnElxfSQAjbfVzXYECOvPuM j6/sDWbuTEapp5cX9YCMLa++mUrLuM3satjIz9BtanQNv933KI9nmC/4UbrfrVFloJi0 zMNavhzf2kXJFLL2nO2JUobPdSisZuWahDbKBeLshueCS/dD0LhwXKYLkBtpAhpEd0UQ XVKw== X-Gm-Message-State: AOAM533He2QhAeboXpYK121443lgIy1Lsu2Ykp3jiLYm/gEfc0qm+nXE GaZIlQl0Ik4KJNzLJmJHxqF4WSTGBsHBurdRARp9VKI0XrerTr51LbaYt8Cw1bxRqO05oOx4dbh qhCEjvYZqegLIPhmXSbTckg== X-Received: by 2002:a05:600c:3546:b0:37b:ecbb:f5a4 with SMTP id i6-20020a05600c354600b0037becbbf5a4mr3460722wmq.23.1645808439936; Fri, 25 Feb 2022 09:00:39 -0800 (PST) X-Google-Smtp-Source: ABdhPJzqiChaQbofcxGYMmP2v6W8U9kXMoLUh0wXrgYKohwlSpVPhKVvtEL78jLrhhg/+dCL7p3+GA== X-Received: by 2002:a05:600c:3546:b0:37b:ecbb:f5a4 with SMTP id i6-20020a05600c354600b0037becbbf5a4mr3460691wmq.23.1645808439520; Fri, 25 Feb 2022 09:00:39 -0800 (PST) Received: from localhost (host86-169-131-29.range86-169.btcentralplus.com. [86.169.131.29]) by smtp.gmail.com with ESMTPSA id s3-20020a5d4ec3000000b001ea95eba44dsm2593419wrv.109.2022.02.25.09.00.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Feb 2022 09:00:38 -0800 (PST) From: Andrew Burgess To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH 01/11] change gdb.base/skip.exp to use finish instead of step In-Reply-To: <20220126195053.69559-2-blarsen@redhat.com> References: <20220126195053.69559-1-blarsen@redhat.com> <20220126195053.69559-2-blarsen@redhat.com> Date: Fri, 25 Feb 2022 17:00:38 +0000 Message-ID: <87zgmeank9.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=-11.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 25 Feb 2022 17:00:44 -0000 Thanks for looking at improving clang compatibility. Bruno Larsen via Gdb-patches writes: > skip.exp was making use of a fixed amount of step commands to exit > some functions. This caused some problems when testing GDB with clang, > as it doesn't add epilogue information for functions. Since the step > commands weren't testing important features, they were changed to finish > commands. > --- > gdb/testsuite/gdb.base/skip.exp | 56 ++++++++++++++------------------- > 1 file changed, 23 insertions(+), 33 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp > index 7c71bb07a84..a961fbbdf41 100644 > --- a/gdb/testsuite/gdb.base/skip.exp > +++ b/gdb/testsuite/gdb.base/skip.exp > @@ -117,9 +117,8 @@ 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" > + gdb_test "step" "foo \\(\\) at.*" "step" > + gdb_test "finish" ".*" "finish" ; # Return from foo() The comment before this "with_test_prefix" block specifically talks about two skips, which is no longer the case. Can you ensure that is updated please. Additionally, the original third step did check that we ended up back in main, which your finish doesn't. I'd ideally like to see the same check in the finish so that the test isn't getting easier. Finally, there's really no reason to give this test a separate name if you're just going to use the command again, so gdb_test "finish" "main \\(\\) at .*" # Return from foo() should hopefully do the job (completely untested). I think this feedback applies throughout this patch, so I've not repeated myself below. > } > > # Now disable the skiplist entry for skip1.c. We should now > @@ -137,13 +136,11 @@ with_test_prefix "step after disabling 3" { > } > > gdb_test "step" "bar \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from bar() > - # With gcc 9.2.0 we jump once back to main before entering foo here. > - # If that happens try to step a second time. > - gdb_test "step" "foo \\(\\) at.*" "step 3" \ > - "main \\(\\) at .*\r\n$gdb_prompt " "step" > - gdb_test "step" ".*" "step 4"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 5" > + # guarantee that we jump once back to main before entering foo here. Start comments with a capital letter. > + gdb_test "finish" ".*" "finish 1"; # Return to main() > + gdb_test "step" "foo \\(\\) at.*" "step 2" \ > + "main \\(\\) at .*\r\n$gdb_prompt " "step" # enter foo() This line with the possibility of sending an extra 'step' used to have a comment attached, which has been removed. I do wonder if the problem was actually with the preceeding 'step', which you've now switched for a finish command? Handily, the old comment had a GCC version number atteched, so it might be possible to try and recreate the old problem. I think that if the extra 'step' is still needed then we should be keeping the comment about GCC 9.2.0 in some form, or, ideally, we'd show that switching to finish removes the need for that extra 'step', and remove it. > + gdb_test "finish" ".*" "finish 2"; # Return from foo() Again, would be nicer to have a better pattern than ".*" here. I think for the rest of the patch it's just the same feedback repeated. Thank, Andrew > } > > # Enable skiplist entry 3 and make sure we step over it like before. > @@ -159,9 +156,8 @@ 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" > + gdb_test "step" "foo \\(\\) at.*" "step" > + gdb_test "finish" ".*" "finish"; # Return from foo() > } > > # Admin tests (disable,enable,delete). > @@ -230,9 +226,8 @@ 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" > + gdb_test "step" "foo \\(\\) at.*" "step" > + gdb_test "finish" ".*" "finish"; # Return from foo() > } > > with_test_prefix "step using -gfi" { > @@ -242,9 +237,8 @@ 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" > + gdb_test "step" "foo \\(\\) at.*" "step" > + gdb_test "finish" ".*" "finish"; # Return from foo() > } > > with_test_prefix "step using -fu for baz" { > @@ -255,13 +249,11 @@ with_test_prefix "step using -fu for baz" { > gdb_test_no_output "skip disable" > gdb_test_no_output "skip enable 7" > gdb_test "step" "bar \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from bar() > - # With gcc 9.2.0 we jump once back to main before entering foo here. > - # If that happens try to step a second time. > - gdb_test "step" "foo \\(\\) at.*" "step 3" \ > - "main \\(\\) at .*\r\n$gdb_prompt " "step" > - gdb_test "step" ".*" "step 4"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 5" > + # guarantee that we jump once back to main before entering foo here. > + gdb_test "finish" ".*" "finish 1"; # Return to main() > + gdb_test "step" "foo \\(\\) at.*" "step 2" \ > + "main \\(\\) at .*\r\n$gdb_prompt " "step" # enter foo() > + gdb_test "finish" ".*" "finish 2"; # Return from foo() > } > > with_test_prefix "step using -rfu for baz" { > @@ -272,13 +264,11 @@ with_test_prefix "step using -rfu for baz" { > gdb_test_no_output "skip disable" > gdb_test_no_output "skip enable 8" > gdb_test "step" "bar \\(\\) at.*" "step 1" > - gdb_test "step" ".*" "step 2"; # Return from bar() > - # With gcc 9.2.0 we jump once back to main before entering foo here. > - # If that happens try to step a second time. > - gdb_test "step" "foo \\(\\) at.*" "step 3" \ > - "main \\(\\) at .*\r\n$gdb_prompt " "step" > - gdb_test "step" ".*" "step 4"; # Return from foo() > - gdb_test "step" "main \\(\\) at.*" "step 5" > + # guarantee that we jump once back to main before entering foo here. > + gdb_test "finish" ".*" "finish 1"; # Return to main() > + gdb_test "step" "foo \\(\\) at.*" "step 2" \ > + "main \\(\\) at .*\r\n$gdb_prompt " "step" # enter foo() > + gdb_test "finish" ".*" "finish 2"; # Return from foo() > } > > # Test -fi + -fu. > -- > 2.31.1