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 22491385740D for ; Mon, 30 May 2022 14:06:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 22491385740D Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-248-VT8UPF4oN8G9gpgqdpoLUg-1; Mon, 30 May 2022 10:06:19 -0400 X-MC-Unique: VT8UPF4oN8G9gpgqdpoLUg-1 Received: by mail-qk1-f200.google.com with SMTP id bk38-20020a05620a1a2600b006a603146aa4so3872306qkb.13 for ; Mon, 30 May 2022 07:06:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=qd3eRu4wftqTKIt4KR6z3Wj+rCYrfrTiYaoa8LyXrRU=; b=ObemXx1uDK2olcP/9qxssa9CImWWRZrqPsacBOSgbE3Y3hyz8fql278mY5Culryecy fuvk57577VwU3Cf5L7H+EKyUNTyYv0vkFbPhnwlmHH0hrFeetv3/qMZZxFDaB+JgrPfU WsQbJcZlRSSMXT3LHFrsA3AsS615wOZCpgzY6CkXiUd5O1A/fzIlbx5SECFoN+NjOzOC w4TR092ALTAnFogtf7gDxC7DazDEHNiM6nHJ+Gg1Gs2bc4U7sHx/XbRX7QuqeRt/X5vM C4OWVprO1vr+K82WSvj3s8FVFw8gS5q4qNs8/cuO52svNa8WWZWbov7CqCAyyjNkNwtX t0SA== X-Gm-Message-State: AOAM533KDq9OWejyxQiDjyOyW1s74bIUGSyEabFXN4RRq4PQDj5vb3y4 Nzmb4JotiuD/5j4bUGIhuf+9b4QszKTJhp1vhMP3dleLFrtalKnxGiSYnf9MyAzdeMxj7uNz1PT KxKuc29YKr0mxhjK/8Z/LVg== X-Received: by 2002:ad4:5944:0:b0:462:310a:b54c with SMTP id eo4-20020ad45944000000b00462310ab54cmr34108170qvb.41.1653919579048; Mon, 30 May 2022 07:06:19 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxZOjzhDb8IdDHH6083/IVRzeiA0s+ijeLe/yNbu9FnspXUVsw/iUPsSLj10fbPybORU7XA/g== X-Received: by 2002:ad4:5944:0:b0:462:310a:b54c with SMTP id eo4-20020ad45944000000b00462310ab54cmr34108132qvb.41.1653919578636; Mon, 30 May 2022 07:06:18 -0700 (PDT) Received: from localhost (host109-152-215-36.range109-152.btcentralplus.com. [109.152.215.36]) by smtp.gmail.com with ESMTPSA id r14-20020ac87eee000000b002f938f5bb78sm7341499qtc.15.2022.05.30.07.06.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 May 2022 07:06:17 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH v3 01/14] gdb/testsuite: introduce gdb_step_until_regexp In-Reply-To: <704cc949-2cac-76e2-b8d2-1efdfc368d3a@redhat.com> References: <20220526151041.23223-1-blarsen@redhat.com> <20220526151041.23223-2-blarsen@redhat.com> <87ilprgd7j.fsf@redhat.com> <704cc949-2cac-76e2-b8d2-1efdfc368d3a@redhat.com> Date: Mon, 30 May 2022 15:06:16 +0100 Message-ID: <87ee0bdsiv.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=-10.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_BARRACUDACENTRAL, 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 14:06:22 -0000 Bruno Larsen via Gdb-patches writes: > 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. That's up to you, but not required from my side. Thanks, Andrew > >> >> 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