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 2921D3852769 for ; Thu, 9 Jun 2022 16:28:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2921D3852769 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-346-55fGI0IxOvqjcAzwwODwoA-1; Thu, 09 Jun 2022 12:28:01 -0400 X-MC-Unique: 55fGI0IxOvqjcAzwwODwoA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 12C8E804191; Thu, 9 Jun 2022 16:28:01 +0000 (UTC) Received: from [10.97.116.29] (ovpn-116-29.gru2.redhat.com [10.97.116.29]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27BC540466A4; Thu, 9 Jun 2022 16:27:59 +0000 (UTC) Message-ID: <23a15847-a4f5-d25b-3477-a03b5942eb21@redhat.com> Date: Thu, 9 Jun 2022 13:27:57 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH v3 02/14] Change gdb.base/skip-solib.exp deal with lack of epilogue information To: Pedro Alves , gdb-patches@sourceware.org References: <20220526151041.23223-1-blarsen@redhat.com> <20220526151041.23223-3-blarsen@redhat.com> From: Bruno Larsen In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.11.54.1 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=-13.4 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: Thu, 09 Jun 2022 16:28:04 -0000 On 6/8/22 12:37, Pedro Alves wrote: > On 2022-05-26 16:10, Bruno Larsen via Gdb-patches wrote: >> When running gdb.base/skip-solib.exp, the backtrace tests could fail if >> the compiler did not emit epilogue information for trivial epilogues, > > I found "emit epilogue information" very confusing FWIW. When I read that the first time, > I thought this was talking about unwind info for the prologue after the stack is destroyed, > and I wondered why not fix clang instead. > > I think the following would be more accurate: > > "fail with compilers that associate prologue insns with the function's last > statement line instead of the function's closing brace". Yeah, I think this is a better explanation, but s/prologue/epilogue, right? In the simplest terms, we need one more step to leave a function, because the pop and return instructions are not marked as belonging to the last brace. I tried to make this concept more specific by saying that clang doesn't emit epilogue information, Have I misunderstood or misnamed it? > >> despite the feature being fully functional. As an example, when testing >> skipping the function square, the testsuite would show >> > > >> --- >> 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 \\(\\)\\).*" > > FWIW, I'd remove the "regexp" from the function's name, just call it gdb_step_until. Alright, but this sounds more like a review of the first patch than this one. > > >> 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 > > "add epilogue information" here falls in the "very confusing" camp for me. > >> +# 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" >> + return >> +} > > That begs the question -- what if clang changes in the future? Nobody > will remember to update this. > > I think it would be better to add a procedure to lib/gdb.exp that detects this > automatically, if possible. I.e., with the gdb.base/skip.exp program, note: > > (gdb) list foo > 36 return 0; > 37 } > 38 > 39 int > 40 foo () > 41 { > 42 return 0; > 43 } > 44 > 45 static void > (gdb) > 46 test_skip (void) > 47 { > 48 } > 49 > 50 static void > 51 end_test_skip_file_and_function (void) > 52 { > 53 abort (); > 54 } > 55 > (gdb) > > when the program is compiled with clang, we get this: > > (gdb) info line 42 > Line 42 of "../../../src/gdb/testsuite/gdb.base/skip.c" starts at address 0x201804 and ends at 0x201810 . > (gdb) info line 43 > Line 43 of "../../../src/gdb/testsuite/gdb.base/skip.c" is at address 0x201830 but contains no code. > > while if the program is compiled with gcc, we get this: > > (gdb) info line 42 > Line 42 of "/home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/skip.c" starts at address 0x117d and ends at 0x1182 . > (gdb) info line 43 > Line 43 of "/home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/skip.c" starts at address 0x1182 and ends at 0x1184 . > > > Or even with a simple "just main" program, like so: > > (gdb) list > 1 int > 2 main () > 3 { > 4 return 0; > 5 } > > we get this with gcc: > > (gdb) info line 4 > Line 4 of "main.c" starts at address 0x1131 and ends at 0x1136 . > (gdb) info line 5 > Line 5 of "main.c" starts at address 0x1136 and ends at 0x1138. > (gdb) > > and this with clang: > > (gdb) info line 4 > Line 4 of "main.c" starts at address 0x40111d and ends at 0x40111f. > (gdb) info line 5 > Line number 5 is out of range for "main.c". > (gdb) > > I think that we can use that to write a caching proc like: > > # Return true if the compiler emits line information associating prologue insns with > # the function's closing brace. Return false if not, meaning the prologue > # associates prologue instructions with function's last line with a statement. > > gdb_caching_proc have_prologue_line_info { > use gdb_simple_compile the simple main program from above > > use "info line 5", and return false if we get "out of range", otherwise return true. > } > This sounds like a good idea, I would only change the default behavior to returning false, unless we saw specifically "start at address.*and ends at". This would make GDB changes more pronounced, as GCC would start having different results. Cheers! Bruno Larsen