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.129.124]) by sourceware.org (Postfix) with ESMTPS id 262DA385E03D for ; Mon, 30 May 2022 12:44:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 262DA385E03D Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-425-XZz7rDWCPhuv-qtQk_oCsw-1; Mon, 30 May 2022 08:44:49 -0400 X-MC-Unique: XZz7rDWCPhuv-qtQk_oCsw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 51EE9299E757 for ; Mon, 30 May 2022 12:44:49 +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 85C3D2026D07; Mon, 30 May 2022 12:44:48 +0000 (UTC) Message-ID: <704cc949-2cac-76e2-b8d2-1efdfc368d3a@redhat.com> Date: Mon, 30 May 2022 09:44:46 -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 01/14] gdb/testsuite: introduce gdb_step_until_regexp To: Andrew Burgess , gdb-patches@sourceware.org References: <20220526151041.23223-1-blarsen@redhat.com> <20220526151041.23223-2-blarsen@redhat.com> <87ilprgd7j.fsf@redhat.com> From: Bruno Larsen In-Reply-To: <87ilprgd7j.fsf@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 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.0 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 12:44:52 -0000 Hi Andrew, thanks for the review! On 5/27/22 13:19, Andrew Burgess wrote: > Bruno Larsen via Gdb-patches writes: > >> Currently, GDB's testsuite uses a set amount of step commands to exit >> functions. This is a problem if a compiler emits different epilogue >> information from gcc, or emits no epilogue information at all. It was >> most noticeable if Clang was used to test GDB. >> >> To fix this unreliability, this commit introduces a new proc that will >> single step the inferior until it is stopped at a line that matches the >> given regexp, or until it steps too many times - defined as an optional >> argument. If the line is found, it shows up as a single PASS in the >> test, and if the line is not found, a single FAIL is emitted. >> >> This patch only introduces this proc, but does not add it to any >> existing tests, these will be introduced in the following commit. >> --- >> >> No change in v3 >> >> New patch in v2 >> >> --- >> gdb/testsuite/lib/gdb.exp | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp >> index b04fbb89e4e..c0ca1d04cc2 100644 >> --- a/gdb/testsuite/lib/gdb.exp >> +++ b/gdb/testsuite/lib/gdb.exp >> @@ -8648,5 +8648,35 @@ proc get_set_option_choices {set_cmd} { >> return $values >> } >> >> +# This proc is used mainly to exit function in a compiler agnostic way >> +# It makes gdb single step and evaluate the output at every step, to see >> +# if the regexp is present. >> +# >> +# The proc takes 2 optional arguments, the first being the name of the >> +# test and the second the maximum amount of iterations until we expect to >> +# see the function. The default is 10 steps, since this is meant as the >> +# last step by default, and we don't expect any compiler generated epilogue >> +# longer than 10 steps. > > I feel like you are being overly prescriptive in how this function > should be used. > > I would rewrite this to just describe what the function does, and let > folk use it as they see fit. Sure, initially it will only be used as > you imagine - that's why you're adding it. But once it's there, who > knows what uses it might be put too. > > I'd go with something like: > > # Single step until the pattern REGEXP is found. Step at most > # MAX_STEPS times, but stop stepping once REGEXP is found. > # > # If REGEXP is found then a single pass is emitted, otherwise, after > # MAX_STEPS steps, a single fail is emitted. > # > # TEST_NAME is the name used in the pass/fail calls. > Good point. I've used your version of the comment. >> + >> +proc gdb_step_until_regexp { regexp {test_name "single stepping until regexp"} {max_steps 10} } { > > You should keep this line under 80 characters. You can wrap the > arguments I believe, like: > > proc gdb_step_until_regexp { regexp > {test_name "single stepping until regexp"} > {max_steps 10} } { > > However, I'd be tempted to take a different approach, like this: > > proc gdb_step_until_regexp { regexp {test_name ""} {max_steps 10} } { > > if { $test_name == "" } { > set test_name "single stepping until regexp" > } > > The benefit I see in this approach is that if a user wants to adjust > max_steps, but doesn't care about the test name, they can do this: > > gdb_step_until_regexp "SOME_PATTERN" "" 50 > > And still get the default test name. That's a good reason. I didn't have any for my choice other than it's what I was comfortable with. Using your version again > >> + global gdb_prompt > > I think this is OK, there's certainly lots of precedent for this > approach, but I think more often these days, we just refer to the global > directly as: > > $::gdb_prompt > > As this removes the need for the 'global gdb_prompt' line. > > But I don't think this is a hard requirement if you prefer what you > have. I do like this version more too, more clear that it is a global variable for new contributors. At this point, I wonder if I should add a co-authored tag to the patch. > > Thanks, > Andrew > >> + >> + set count 0 >> + gdb_test_multiple "step" "$test_name" { >> + -re "$regexp\r\n$gdb_prompt $" { >> + pass $test_name >> + } >> + -re ".*$gdb_prompt $" { >> + if {$count < $max_steps} { >> + incr count >> + send_gdb "step\n" >> + exp_continue >> + } else { >> + fail $test_name >> + } >> + } >> + } >> +} >> + >> # Always load compatibility stuff. >> load_lib future.exp >> -- >> 2.31.1 > Cheers! Bruno Larsen