From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id OpvkCFi8lGIKOAkAWB0awg (envelope-from ) for ; Mon, 30 May 2022 08:45:12 -0400 Received: by simark.ca (Postfix, from userid 112) id 1472B1E221; Mon, 30 May 2022 08:45:12 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=DJJXLyRE; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 623AD1E143 for ; Mon, 30 May 2022 08:45:11 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D89D638515D9 for ; Mon, 30 May 2022 12:45:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D89D638515D9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1653914710; bh=YKxQbZSKljdGQD7KEcp+6ssLfJqxXDNT/RH+P3oMIFg=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=DJJXLyREtVDd7uKrZTfHuGIlf8bUs7iYB0E3aYjLkAdX2lvFsIqOIFrzcoeA2vOeI 1i3zX+8/2J0uF9P2N8lLWWKVwB6B+EZsfVXoIxuPnpnSu+ONsNBia6kY0Dzs3LIyTm Pfz3abxCiwOMuSjtYxLF3dkPeGxHFGv7apqjUyUA= 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> 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-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: , From: Bruno Larsen via Gdb-patches Reply-To: Bruno Larsen Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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