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 10BE03858CDB for ; Sat, 10 Sep 2022 09:53:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 10BE03858CDB Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-112-cRJqBId1M-GpSCTDRRHDRw-1; Sat, 10 Sep 2022 05:53:47 -0400 X-MC-Unique: cRJqBId1M-GpSCTDRRHDRw-1 Received: by mail-wr1-f70.google.com with SMTP id k17-20020adfb351000000b00228853e5d71so793101wrd.17 for ; Sat, 10 Sep 2022 02:53:47 -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=PHjzSXYbWPcuYiCGs079eunfMquttuDUwAC7TLm/cdg=; b=FxuaItC8yGQojt058oJD65KIGDg6gdowBCcyl0VwlDCf/qnDyQ20JLDf9yapBy17vX aoOj5z8Krsb492JngcOQ89keeFp987TAB2OaPpnmYnaVf7gbFpwWrSykXxSlv5oCaORP L11w4KuNGB/aQIjzTc7/YfQ0Uw9juluZhAy1MAppXEeI4XfEol02Qx6CdrSui/+cdM2b lms2K2F/Id5fkj+AWQ8e6cqWhVAgyhcMxUz3MNyxG6zLlB72Zl3hTF1Ur7NXsVJCsSy4 o4jwUAbjJbtrQK1lyoZgsDTY+MIKnDsnlUEZe40r6mzEtj304JqxI0KX3R+R8GNWhHMk VCsw== X-Gm-Message-State: ACgBeo36NlHISJglIPZ1NDes5eD4BUmf9FFxWdDNae3/5wisB182lgcK GZuWFR/RWQVSeVH/pYdAfXXlb4rPjWUO1v/dw3jW/ZCVpGkD/WaKexVpcw6m0w0RKOhIKOl0RGC Wl9KNeZo4MpjXZA1zl15SUg== X-Received: by 2002:a05:600c:1c8d:b0:3a6:8ef:f6c0 with SMTP id k13-20020a05600c1c8d00b003a608eff6c0mr7801763wms.23.1662803626255; Sat, 10 Sep 2022 02:53:46 -0700 (PDT) X-Google-Smtp-Source: AA6agR7QW4X7dUow9Rb0O7lPQozpOALtaHpdwAK5ahrl1c9gehhiz/zElCD9dGCC0VV+dWjuhVeqyA== X-Received: by 2002:a05:600c:1c8d:b0:3a6:8ef:f6c0 with SMTP id k13-20020a05600c1c8d00b003a608eff6c0mr7801756wms.23.1662803625951; Sat, 10 Sep 2022 02:53:45 -0700 (PDT) Received: from localhost (92.40.179.160.threembb.co.uk. [92.40.179.160]) by smtp.gmail.com with ESMTPSA id n17-20020a5d4c51000000b0021eed2414c9sm2480594wrt.40.2022.09.10.02.53.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 10 Sep 2022 02:53:45 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH v4 03/15] Change gdb.base/skip-solib.exp deal with lack of epilogue information In-Reply-To: <20220720194441.168906-5-blarsen@redhat.com> References: <20220720194441.168906-1-blarsen@redhat.com> <20220720194441.168906-5-blarsen@redhat.com> Date: Sat, 10 Sep 2022 10:53:43 +0100 Message-ID: <87y1urlfmg.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=-12.6 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, 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: Sat, 10 Sep 2022 09:53:51 -0000 Bruno Larsen via Gdb-patches writes: > When running gdb.base/skip-solib.exp, the backtrace tests could fail with > compilers that associated epilogue instructions with the last statement > line of the function, instead of associating it with the closing brace, > despite the feature being fully functional. As an example, when testing > skipping the function square, the testsuite would show > > Breakpoint 1, main () at (...)/binutils-gdb/gdb/testsuite/gdb.base/skip-solib-main.c:5 > 5 return square(0); > (gdb) step > 0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6 > (gdb) PASS: gdb.base/skip-solib.exp: ignoring solib file: step > bt > #0 0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6 > #1 0x00007ffff7cef60c in __libc_start_main_impl () from /lib64/libc.so.6 > #2 0x0000000000401065 in _start () > (gdb) FAIL: gdb.base/skip-solib.exp: ignoring solib file: bt > > Which means that the feature is working, the testsuite is just > mis-identifying it. To avoid this problem, the skipped function calls > have been sent to a line before `return`, so epilogues won't factor in. > > This commit has also changed a few hardcoded steps to leave functions to > the newly introduced gdb_step_until to leave those functions. I think I would like to see the skip-inline.exp change moved into a separate commit given it's a completely different type of fix. > --- > gdb/testsuite/gdb.base/skip-inline.exp | 23 ++++++++++++++++------- > gdb/testsuite/gdb.base/skip-solib-lib.c | 3 ++- > gdb/testsuite/gdb.base/skip-solib-main.c | 3 ++- > gdb/testsuite/gdb.base/skip-solib.exp | 12 ++++++++++-- > 4 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp > index f6e9926b66c..3fbaa5469dd 100644 > --- a/gdb/testsuite/gdb.base/skip-inline.exp > +++ b/gdb/testsuite/gdb.base/skip-inline.exp > @@ -15,6 +15,11 @@ > > standard_testfile > > +set epilogue 1 > +if {![have_epilogue_line_info]} { > + set epilogue 0 > +} I think 'set epilogue [have_epilogue_line_info]' would be better. > + > if { [prepare_for_testing "failed to prepare" "skip-inline" \ > {skip-inline.c skip1.c } \ > {debug nowarnings}] } { > @@ -35,16 +40,20 @@ gdb_test "skip function foo" "Function foo will be skipped when stepping\." > gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main" > gdb_test "step" ".*" "step into baz, since foo will be skipped" > gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped" > -gdb_test "step" ".*" "step in the baz" > -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz" > -gdb_test "step" ".*" "step back to main" > +gdb_step_until ".*x = 0; x = baz \\(foo \\(\\)\\).*" > gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main" > gdb_test "step" ".*" "step again into baz, since foo will be skipped" > gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz" > -gdb_test "step" ".*" "step in the baz, again" > -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again" > -gdb_test "step" ".*" "step back to main, again" > -gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main" > +gdb_step_until "main \\(\\) at .*" "step back to main, again" > +gdb_test "bt" "\\s*\\#0.*main.*" "again back to main" > + > +# Because clang doesn't add epilogue information, having a set number of > +# steps puts clang more and more out of sync with gcc. It is unlikely that > +# the effort of keeping both outputs will be useful. > +if {$epilogue == 0} { Just 'if { !$epilogue } {' would be better. > + untested "Multiple steps tests are not supported with this compiler" > + return > +} I notice that there's actually another test at the end of this file that doesn't rely on multiple steps, which we now end up skipping due to this early return. I wonder if this test file would be better structured something like: proc_with_prefix single_step { } { # The first block of tests that just does 'step'. } proc_with_prefix double_step { } { # The second block of tests that do 'step 2'. } proc_with_prefix triple_step { } { # The third block of tests that do 'step 3'. } proc_with_prefix skip_current_frame { } { # The final bit of test that sets up a skip of foo. } single_step if { $epilogue } { double_step triple_step } skip_current_frame > > if ![runto_main] { > return > diff --git a/gdb/testsuite/gdb.base/skip-solib-lib.c b/gdb/testsuite/gdb.base/skip-solib-lib.c > index b2c4d86d703..341f1440a3b 100644 > --- a/gdb/testsuite/gdb.base/skip-solib-lib.c > +++ b/gdb/testsuite/gdb.base/skip-solib-lib.c > @@ -7,5 +7,6 @@ int multiply(int a, int b) > > int square(int num) > { > - return multiply(num, num); > + int res = multiply(num, num); > + return res; > } > diff --git a/gdb/testsuite/gdb.base/skip-solib-main.c b/gdb/testsuite/gdb.base/skip-solib-main.c > index 746bb5f36bb..a3b6d417935 100644 > --- a/gdb/testsuite/gdb.base/skip-solib-main.c > +++ b/gdb/testsuite/gdb.base/skip-solib-main.c > @@ -2,5 +2,6 @@ int square(int num); > > int main() > { > - return square(0); > + int s = square(0); > + return s; > } > diff --git a/gdb/testsuite/gdb.base/skip-solib.exp b/gdb/testsuite/gdb.base/skip-solib.exp > index 0f2ce7e1ad8..8e61725ad1b 100644 > --- a/gdb/testsuite/gdb.base/skip-solib.exp > +++ b/gdb/testsuite/gdb.base/skip-solib.exp > @@ -82,7 +82,7 @@ with_test_prefix "ignoring solib file" { > # We shouldn't step into square(), since we skipped skip-solib-lib.c. > # > gdb_test "step" "" > - gdb_test "bt" "#0\\s+main.*" > + gdb_test "bt 1" "#0\\s+main.*" > } > > # > @@ -114,5 +114,13 @@ with_test_prefix "ignoring solib function" { > # the last line of square. > # > gdb_test "step" "" > - gdb_test "bt" "#0\\s+square.*" > + gdb_test "bt 1" "#0\\s+square.*" "skipped multiply" > +# gdb_test_multiple "bt 1" "skipped multiply" { > +# -re "#0\\s+square.*" { > +# pass "skipped multiply" > +# } > +# -re "#0.*main.*" { > +# pass "skipped multiply" > +# } > +# } This commented out code should be removed. In fact, I wonder if any of the changes in skip-solib.exp are actually needed. Sure, 'bt 1' is maybe a little more specific than 'bt', but given the pattern we check for doesn't change, I don't think this change should make any difference. If this is just a preference/cleanup then this should probably move into a sperate patch to avoid any confusion. Or just drop this part? Thanks, Andrew > } > -- > 2.31.1