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 2BC7B384B433 for ; Mon, 30 May 2022 20:31:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2BC7B384B433 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-502-Nce7FUhcN0u8wr1KWo68CQ-1; Mon, 30 May 2022 16:31:27 -0400 X-MC-Unique: Nce7FUhcN0u8wr1KWo68CQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2301B802814 for ; Mon, 30 May 2022 20:31:27 +0000 (UTC) Received: from [10.97.116.24] (ovpn-116-24.gru2.redhat.com [10.97.116.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6784C40EC002; Mon, 30 May 2022 20:31:26 +0000 (UTC) Message-ID: Date: Mon, 30 May 2022 17:31:19 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH v3 02/14] Change gdb.base/skip-solib.exp deal with lack of epilogue information To: Andrew Burgess , gdb-patches@sourceware.org References: <20220526151041.23223-1-blarsen@redhat.com> <20220526151041.23223-3-blarsen@redhat.com> <87h757dslm.fsf@redhat.com> From: Bruno Larsen In-Reply-To: <87h757dslm.fsf@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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, 30 May 2022 20:31:31 -0000 Cheers! Bruno Larsen On 5/30/22 11:04, Andrew Burgess wrote: > Bruno Larsen via Gdb-patches writes: > >> When running gdb.base/skip-solib.exp, the backtrace tests could fail if >> the compiler did not emit epilogue information for trivial epilogues, >> 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_regexp to leave those functions. >> --- >> >> No change in v3 >> >> v2: chagned to use step_until_regexp instead of finish >> >> --- >> gdb/testsuite/gdb.base/skip-inline.exp | 18 +++++++++++------- >> 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, 25 insertions(+), 11 deletions(-) >> >> diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp >> index f6e9926b66c..327ea676140 100644 >> --- a/gdb/testsuite/gdb.base/skip-inline.exp >> +++ b/gdb/testsuite/gdb.base/skip-inline.exp >> @@ -35,16 +35,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_regexp ".*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_regexp "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 > > Capital letter - 'Because'. Fixed. > >> +# 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 {[test_compiler_info "clang-*"]} { >> + untested "Multiple steps are not supported with clang" > > I'd reword this as "Multiple step tests are not supported with clang" > just to avoid confusion. Multiple steps are fine, it's the tests > themselves we're not proposing to support with clang. good catch! Fixed. > >> + return >> +} >> >> 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.*" > > I don't object to this change, but it's not necessary. > >> } >> >> # >> @@ -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" >> +# } >> +# } > > So, I was worried while looking at this change that we might end up > removing all the places where we try to step through code like: > > int square(int num) > { > return multiply(num, num); > } > > Which I think would be a shame. However, I notice you (accidentally) > left in some commented out code - and I think we might be able to use > that. > > What if we reverted the change to the `square` function, and then, for > this last test did this: > > gdb_test_multiple "bt 1" "skipped multiply" { > -re "#0\\s+square.*" { > pass $gdb_test_name > } > -re "#0.*main.*" { > # Clang doesn't add sufficient epilogue information to > # allow GDB to stop within square after the call to > # multiply. We accept ending up back in main, but only > # for clang. > if [test_compiler_info clang*] { > pass $gdb_test_name > } > } > } > > For any compiler that, like gcc, emits some line info for the function > epilogue, the test passes. But we let clang pass even though it appears > to unwind the extra frame. > > Thoughts? As we spoke off-list, I am of the opinion that tests should only be used to test what they are trying to test. So the ideal solution would be having a test that tries to step through epilogues in different configurations, and we can safely assume that it works for the rest of the tests. I am working in one such test, which I'll send soon. In the meantime, I'm not opposed to the specific change you proposed, as it is readable and small, so I can also do this if you think it is worth doing. > > Thanks, > Andrew >