From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68004 invoked by alias); 27 Dec 2017 14:34:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 67988 invoked by uid 89); 27 Dec 2017 14:34:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=cid X-HELO: mail-pl0-f41.google.com Received: from mail-pl0-f41.google.com (HELO mail-pl0-f41.google.com) (209.85.160.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 27 Dec 2017 14:34:51 +0000 Received: by mail-pl0-f41.google.com with SMTP id s10so19507481plj.5 for ; Wed, 27 Dec 2017 06:34:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=zS7Hx9LrUmJ9R4K/fEclZCIDy0S+r4HPw+4XTm1qpzw=; b=kEuq2/22wNSGwmZmkxS1QRugBrwRDB7uxgDasIfe5afBMriMzegQ7nxmHSWh1HsuHo ONuG3COxHC+D1JwOXzozbXXmH+UsyjyylzbIFFQ5toCHgL/mccLZKmUy4Vb+RC993EYE Hu4IcmRSPjvo9Z+imVP5tkBnCs5aHj10Evjp91fVj2aOWOVgN/M/HRXICj/l1BM65rqM 8xJQyLod906OflZzztbEMdFgX5p4ephIbXgoXuMUILejBi4eqg7vL0+DNirhc7kmSnvo l7LWcegZ3i9g1SzhXz3xw4SRVQB4DMzuu7xsq26sDqz2V3VfCh76vSUzj7Jqzp4M6eLm CArw== X-Gm-Message-State: AKGB3mJS9DK8ShtMG527mVVFHXwQsHd+MjTBndGuHjebsfMlIya+JquV s1+KELStx7Z6Hz3lPGKBMz0= X-Google-Smtp-Source: ACJfBouqHXsrMnJfw9fSBd9vh1yryIB6BzJidal2B3/WIsP79BvjlBseypeP7p2j5rMqZtpYq8yIug== X-Received: by 10.84.234.193 with SMTP id i1mr28494357plt.146.1514385289530; Wed, 27 Dec 2017 06:34:49 -0800 (PST) Received: from localhost (g121.222-224-191.ppp.wakwak.ne.jp. [222.224.191.121]) by smtp.gmail.com with ESMTPSA id n24sm66508953pfi.33.2017.12.27.06.34.48 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 27 Dec 2017 06:34:48 -0800 (PST) Date: Wed, 27 Dec 2017 14:34:00 -0000 From: Stafford Horne To: Simon Marchi Cc: GDB patches , Eli Zaretskii Subject: Re: [PATCH v4 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Message-ID: <20171227143445.GG32243@lianli.shorne-pla.net> References: <20171226134832.23497-1-shorne@gmail.com> <20171226134832.23497-2-shorne@gmail.com> <4345a5d3-3ad2-7e20-1eb3-7f776737f2d0@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4345a5d3-3ad2-7e20-1eb3-7f776737f2d0@polymtl.ca> User-Agent: Mutt/1.9.1 (2017-09-22) X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00510.txt.bz2 On Tue, Dec 26, 2017 at 07:20:09PM -0500, Simon Marchi wrote: > Hi Stafford, > > I just pointed out some nits, it's fine with me to push with those fixed. > > On 2017-12-26 08:48 AM, Stafford Horne wrote: > > Until now this feature has existed but was not documented. Adding docs > > and tests. > > > > gdb/ChangeLog: > > > > yyyy-mm-dd Stafford Horne > > > > * infcmd.c (_initialize_infcmd): Add help for info reg $reggroup > > and info all-registers $reggroup feature. > > > > gdb/doc/ChangeLog: > > > > yyyy-mm-dd Stafford Horne > > > > * gdb.texinfo (Registers): Document info reg $reggroup feature. > > > > gdb/testsuite/ChangeLog: > > > > yyyy-mm-dd Stafford Horne > > > > * gdb.base/reggroups.c: New file. > > * gdb.base/reggroups.exp: New file. > > --- > > > > Since v3 > > - Fixed grammar issue in gdb.texinfo pointed out by Eli. > > - Added xref in gdb.texinfo pointed out by Eli. > > - Documented multi-reggroup support in infcmd.c suggested by Eli. > > - Enhanced testing to actual test info reg results suggested by Simon. > > > > gdb/doc/gdb.texinfo | 5 ++ > > gdb/infcmd.c | 8 ++- > > gdb/testsuite/gdb.base/reggroups.c | 5 ++ > > gdb/testsuite/gdb.base/reggroups.exp | 112 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 128 insertions(+), 2 deletions(-) > > create mode 100644 gdb/testsuite/gdb.base/reggroups.c > > create mode 100644 gdb/testsuite/gdb.base/reggroups.exp > > > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > > index 60ed80c363..a16e79bc2a 100644 > > --- a/gdb/doc/gdb.texinfo > > +++ b/gdb/doc/gdb.texinfo > > @@ -11023,6 +11023,11 @@ and vector registers (in the selected stack frame). > > Print the names and values of all registers, including floating-point > > and vector registers (in the selected stack frame). > > > > +@item info registers @var{reggroup} @dots{} > > +Print the name and value of the registers in each of the specified > > +@var{reggroup}s. The @var{reggoup} can be any of those returned by > > +@code{maint print reggroups} (@pxref{Maintenance Commands}). > > + > > @item info registers @var{regname} @dots{} > > Print the @dfn{relativized} value of each specified register @var{regname}. > > As discussed in detail below, register values are normally relative to > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > > index 8bde28eab6..1b63f9b730 100644 > > --- a/gdb/infcmd.c > > +++ b/gdb/infcmd.c > > @@ -3460,13 +3460,17 @@ interrupt all running threads in non-stop mode, use the -a option.")); > > > > c = add_info ("registers", info_registers_command, _("\ > > List of integer registers and their contents, for selected stack frame.\n\ > > -Register name as argument means describe only that register.")); > > +One or more register names as argument means describe the given registers.\n\ > > +One or more register group names as argument means describe the registers\n\ > > +in the named register groups.")); > > add_info_alias ("r", "registers", 1); > > set_cmd_completer (c, reg_or_group_completer); > > > > c = add_info ("all-registers", info_all_registers_command, _("\ > > List of all registers and their contents, for selected stack frame.\n\ > > -Register name as argument means describe only that register.")); > > +One or more register names as argument means describe the given registers.\n\ > > +One or more register group names as argument means describe the registers\n\ > > +in the named register groups.")); > > set_cmd_completer (c, reg_or_group_completer); > > > > add_info ("program", info_program_command, > > diff --git a/gdb/testsuite/gdb.base/reggroups.c b/gdb/testsuite/gdb.base/reggroups.c > > new file mode 100644 > > index 0000000000..8e8f518aae > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/reggroups.c > > @@ -0,0 +1,5 @@ > > I think we need a license head for this file, even if it's trivial. At least > that's what we do for other tests with trivial source files. OK. > > +int > > +main() > > +{ > > + return 0; > > +} > > diff --git a/gdb/testsuite/gdb.base/reggroups.exp b/gdb/testsuite/gdb.base/reggroups.exp > > new file mode 100644 > > index 0000000000..4fc2c9992a > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/reggroups.exp > > @@ -0,0 +1,112 @@ > > +# This testcase is part of GDB, the GNU debugger. > > + > > +# Copyright 2017 Free Software Foundation, Inc. > > + > > +# This program is free software; you can redistribute it and/or modify > > +# it under the terms of the GNU General Public License as published by > > +# the Free Software Foundation; either version 3 of the License, or > > +# (at your option) any later version. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +# You should have received a copy of the GNU General Public License > > +# along with this program. If not, see . > > + > > +# Test listing reggroups and the registers in each group. > > + > > +standard_testfile > > + > > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { > > + return -1 > > +} > > + > > +if ![runto_main] then { > > + fail "can't run to main" > > + return 0 > > +} > > + > > +# Fetch all reggroups from 'maint print reggroups'. > > + > > +proc fetch_reggroups {test} { > > + global gdb_prompt > > + global expect_out > > + > > + set reggroups {} > > + gdb_test_multiple "maint print reggroups" "get reggroups" { > > + -re "maint print reggroups\r\n" { > > + exp_continue > > + } > > + -re "^ Group\[ \t\]+Type\[ \t\]+\r\n" { > > + exp_continue > > + } > > + -re "^ (\[0-9a-zA-Z\-\]+)\[ \t\]+(user|internal)\[ \t\]+\r\n" { > > You don't need to escape the - in the regex. I assume you put it to avoid > it meaning a range in the character set? When it's first or last in the set, > it doesn't need to be escaped. But anyway, here, the backslash is removed > by TCL when it interprets the string (even though the dash has no special > meaning for tcl, unlike the square brackets for example), so it's not there > anymore when the regex gets evaluated. You would need two backslashes for > that. But since it's not needed in this case, I suggest to remove it. Thanks, I admit I am pretty goood with regular expressions, but I really don't remember all of the escaping details. Will fix here and the other spot. > > + lappend reggroups $expect_out(1,string) > > + exp_continue > > + } > > + -re "$gdb_prompt $" { > > + if { [llength $reggroups] != 0 } { > > + pass $test > > + } else { > > + fail "$test - didn't fetch any reggroups" > > + } > > + } > > In cases like this, it's good to use the same test name for the pass and the fail. > If this test starts failing, tools that analyze regressions (for example in the buildbot) > will show it as a FAIL -> PASS, rather than a new FAIL, so it's a bit clearer. OK. Didn't know that. > And in this case I would suggest using gdb_assert: > > gdb_assert "[llength $reggroups] != 0" $test > > Finally, you should use the same test name here than what you pass to gdb_test_multiple, > so that same name will be used regardless of the outcome. For example, if an internal > error happens when sending the command, the gdb_test_multiple code will catch it and > cause a FAIL with that test name. And for the reason stated above, it's good if the > test name is consistent. So to summarize, please change: > > gdb_test_multiple "maint print reggroups" "get reggroups" > > to > > gdb_test_multiple "maint print reggroups" $test > > > + } > > + > > + return $reggroups > > +} > > + > > +# Fetch all registers for a reggroup from 'info reg '. > > + > > +proc fetch_reggroup_regs {reggroup test} { > > + global gdb_prompt > > + global expext_out > > Typo here? It probably means that it's not required? If it's not required here, > it's probably not required in the other proc either. Right, gdb_prompt is needed, but not expect_out. > > + > > + # The command info reg will return something like the following: > > + # > > + # r0 0x0 0^M > > + # r1 0x7fdffc 0x7fdffc^M > > + # r2 0x7fe000 0x7fe000^M > > + # npc 0x23a8 0x23a8 ^M > > + # sr 0x8401 [ SM CY FO CID=0 ]^M > > + # > > + # We parse out and return the reg names, this is done by detecting > > + # that for each line we have a register name followed by a $hex number. > > + set regs {} > > + gdb_test_multiple "info reg $reggroup" "info reg $reggroup" { > > Use $test as the test name. > > > + -re "info reg $reggroup\r\n" { > > + exp_continue > > + } > > + -re "^(\[0-9a-zA-Z\-\]+)\[ \t\]+(0x\[0-9a-f\]+)\[ \t\]+(\[^\n\r\]+)\r\n" { > > Same thing about escaping the dash. > > There are registers whose value is not a simple number, like vector registers: > > xmm0 {v4_float = {0x0, 0x0, 0x0, 0x0}, v2_double = {0x0, 0x0}, v16_int8 = {0x0 }, ... > > Those won't be matched and returned by this regex (not sure where that output goes, > since it's never matched). I think it doesn't matter for this test, but I just > want to be sure you've considered it. My thought is that the test is trying to ensure we capture some interger registers and no Invalid register result accross the system. I think limiting it to this is ok. Let add a comment. > > + lappend regs $expect_out(1,string) > > + exp_continue > > + } > > + -re "Invalid register .*\r\n" { > > + fail "$test - unexpected invalid register response" > > Again, we should use the same test name. But if you want to put more > info about the failure, as you do here, put it in parentheses. The buildbot > strips trailing text in parentheses from test names when analyzing regressions, > so "$test" and "$test (unexpected invalid register response)" will be > considered as the same test name. > > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages OK, good to know. > > + } > > + -re "$gdb_prompt $" { > > + pass $test > > + } > > + } > > + return $regs > > +} > > + > > +set reggroups [fetch_reggroups "fetch reggroups"] > > +set regcount 0 > > +foreach reggroup $reggroups { > > + set regs [fetch_reggroup_regs $reggroup "fetch reggroup regs $reggroup"] > > + set regcount [expr $regcount + [llength $regs]] > > +} > > +if { $regcount != 0 } { > > + pass "system has reggroup regs $regcount" > > +} else { > > + fail "system has no reggroup regs at all" > > +} > > I suggest using gdb_assert. OK. > > + > > +# If this fails it means that probably someone changed the error text returned > > +# for an invalid register argument. If that happens we should fix the pattern > > +# here and in the fetch_reggroup_regs procedure above. > > +gdb_test "info reg invalid-reggroup" "Invalid register .*" \ > > + "info reg invalid-reggroup should report 'Invalid register'" > > I would suggest putting the regex in a variable > > set invalid_register_re "Invalid register .*" OK. > Simon