Message ID | f708d6f5e4f81f3f08c27916fb8d6a18d753cde8.1523286728.git.andrew.burgess@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 118901 invoked by alias); 9 Apr 2018 15:15:47 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 118790 invoked by uid 89); 9 Apr 2018 15:15:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=2122, internal-error, internalerror, Hx-spam-relays-external:209.85.128.194 X-HELO: mail-wr0-f194.google.com Received: from mail-wr0-f194.google.com (HELO mail-wr0-f194.google.com) (209.85.128.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Apr 2018 15:15:40 +0000 Received: by mail-wr0-f194.google.com with SMTP id 80so9971591wrb.2 for <gdb-patches@sourceware.org>; Mon, 09 Apr 2018 08:15:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:in-reply-to:references; bh=uiSOeXU4vPcDXUc1Iln1Qh/fiMc4RngF0Z0FylE30Tg=; b=iDXklTFpKCluTpQwJU8yZPBZ43zcyugGq+BmV18oHgjBhEjW0vkBQ5OE98tx/37OHV zw2S+y/hrPtgg/TcV+MRy2nX6XuDoIdSoHxI+Ehu8XTSdFp5V/rdaNdG5qYgXSqi7SH/ oR4ivheSJg65pITDnDpJyno9iiOnX6EUtbbD4MhPReYcw0B9JBemJA54BVKDlMCMWmIU VVyPv6H9VN6IwQ6Dnb2UGD+hPEWAJShEk9wHNHWFjWtmSUCglQSjo5Z+L+TK/cXtBB25 smkwzdfYcmD5wCNOekn9oXCSnnMp7sOKDzT5Qu8XK78XF6hal4Mt7fA/FdlrFLYZxVrO 7xuA== X-Gm-Message-State: ALQs6tCfI5PErSsSLb7lj2+0aGvwnbxY6ANAZZ0X1nkPnMN5iqdDzcut vC6IsGQj/OWdMr80+RA+cwCiQNbT X-Google-Smtp-Source: AIpwx48BlbBOePD0xjNoN6hvKnhmXUtEq4IBssP4kpzZL2PTpZcpLc5taZicGLIy+Eso1RaqeMxaIQ== X-Received: by 10.223.135.237 with SMTP id c42mr5012248wrc.146.1523286935958; Mon, 09 Apr 2018 08:15:35 -0700 (PDT) Received: from localhost (host81-148-252-121.range81-148.btcentralplus.com. [81.148.252.121]) by smtp.gmail.com with ESMTPSA id 39sm872726wry.89.2018.04.09.08.15.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Apr 2018 08:15:35 -0700 (PDT) From: Andrew Burgess <andrew.burgess@embecosm.com> To: gdb-patches@sourceware.org Cc: Andrew Burgess <andrew.burgess@embecosm.com> Subject: [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers Date: Mon, 9 Apr 2018 16:15:29 +0100 Message-Id: <f708d6f5e4f81f3f08c27916fb8d6a18d753cde8.1523286728.git.andrew.burgess@embecosm.com> In-Reply-To: <cover.1523286728.git.andrew.burgess@embecosm.com> References: <cover.1523286728.git.andrew.burgess@embecosm.com> In-Reply-To: <cover.1523286728.git.andrew.burgess@embecosm.com> References: <cover.1523286728.git.andrew.burgess@embecosm.com> X-IsSubscribed: yes |
Commit Message
Andrew Burgess
April 9, 2018, 3:15 p.m. UTC
In gdb.base/maint.exp a test calls 'maint print registers'. If the target has lots of registers this may overflow expect's buffers, causing the test to fail. After this commit we process the output line at a time until we get back to the GDB prompt, this should prevent buffer overrun while still testing that the command works as required. gdb/testsuite/ChangeLog: * gdb.base/maint.exp: Process output from 'maint print registers' line at a time. --- gdb/testsuite/ChangeLog | 5 +++++ gdb/testsuite/gdb.base/maint.exp | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-)
Comments
On Mon, 9 Apr 2018, Andrew Burgess wrote: > # Test for a regression where this command would internal-error if the > -# program wasn't running. > -gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*" > +# program wasn't running. If there's a lot of registers then this > +# might overflow expect's buffers, so process the output line at a > +# time. > +send_gdb "maint print registers\n" > +gdb_expect { > + -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" { > + exp_continue > + } I think this changes the meaning of the test; you want to preserve the heading match pattern at the very least. Also `gdb_test' handles various error cases gracefully (which matters for the avoidance of excessive timeouts with some test boards), whereas your simple matcher does not. Also how many is "a lot"? Perhaps you could take the path of least resistance instead and simply increase the size of the buffer, like with commit ff604a674771 ("gdb/testsuite: Bump up `match_max'"). This could be done temporarily for this test only, so as to avoid slowing down `expect' throughout the test suite. Maciej
On 04/13/2018 12:39 AM, Maciej W. Rozycki wrote: > On Mon, 9 Apr 2018, Andrew Burgess wrote: > >> # Test for a regression where this command would internal-error if the >> -# program wasn't running. >> -gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*" >> +# program wasn't running. If there's a lot of registers then this >> +# might overflow expect's buffers, so process the output line at a >> +# time. >> +send_gdb "maint print registers\n" >> +gdb_expect { >> + -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" { >> + exp_continue >> + } > > I think this changes the meaning of the test; you want to preserve the > heading match pattern at the very least. Also `gdb_test' handles various > error cases gracefully (which matters for the avoidance of excessive > timeouts with some test boards), whereas your simple matcher does not. Yeah, there's no good reason to use gdb_expect directly here, AFAICT. You can do the same thing with gdb_test_multiple, which handles the timeout already, as well as other error conditions, including the internal-error the comment the test above mentions. > > Also how many is "a lot"? Perhaps you could take the path of least > resistance instead and simply increase the size of the buffer, like with > commit ff604a674771 ("gdb/testsuite: Bump up `match_max'"). This could be > done temporarily for this test only, so as to avoid slowing down `expect' > throughout the test suite. I think the exp_continue trick is better for getting rid of the problem for good. We can still use it, with gdb_test_multiple. A few comments more: > + -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" { Nit: \n\r instead of \r\n kind of reads like a typo to me: $ grep -rn "\\\\r\\\\n\\\\]" | wc -l 1036 $ grep -rn "\\\\n\\\\r\\\\]" | wc -l 28 I'd suggest flipping it around to the more usual form just to avoid causing pause when people read the regexp. > + exp_continue > + } > + -re ".*$gdb_prompt $" { No need for leading .*, it's implied. > + pass "maint print registers" > + } > + timeout { > + fail "maint print registers (timeout)" > + } > +} > + So I'd suggest something like this: set saw_registers 0 set test "maint print registers" gdb_test_multiple $test $test { -re "^\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[\r\n\]+" { set saw_registers 1 exp_continue } -re "$gdb_prompt $" { gdb_assert $saw_registers $test } } The "saw_registers" bit ends up serving as replacement for seeing the heading, though you can also add a pattern to match the heading and check it in the gdb_assert instead if you'd like. Thanks, Pedro Alves
On Fri, 13 Apr 2018, Pedro Alves wrote: > So I'd suggest something like this: > > set saw_registers 0 > set test "maint print registers" > gdb_test_multiple $test $test { > -re "^\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[\r\n\]+" { > set saw_registers 1 > exp_continue > } > -re "$gdb_prompt $" { > gdb_assert $saw_registers $test > } > } > > The "saw_registers" bit ends up serving as replacement for > seeing the heading, though you can also add a pattern to > match the heading and check it in the gdb_assert instead if > you'd like. FWIW I think we should keep the heading check. Maciej
diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp index 93221085653..f73495faa5b 100644 --- a/gdb/testsuite/gdb.base/maint.exp +++ b/gdb/testsuite/gdb.base/maint.exp @@ -62,8 +62,22 @@ gdb_test_no_output "set height 0" gdb_file_cmd ${binfile} # Test for a regression where this command would internal-error if the -# program wasn't running. -gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*" +# program wasn't running. If there's a lot of registers then this +# might overflow expect's buffers, so process the output line at a +# time. +send_gdb "maint print registers\n" +gdb_expect { + -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" { + exp_continue + } + -re ".*$gdb_prompt $" { + pass "maint print registers" + } + timeout { + fail "maint print registers (timeout)" + } +} + # Test "mt expand-symtabs" here as it's easier to verify before we # run the program.