[PATCH v4 1/4] reggroups: Add test and docs for `info reg $reggroup` feature

Stafford Horne shorne@gmail.com
Wed Dec 27 14:34:00 GMT 2017


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  <shorne@gmail.com>
> > 
> > 	* infcmd.c (_initialize_infcmd): Add help for info reg $reggroup
> > 	and info all-registers $reggroup feature.
> > 
> > gdb/doc/ChangeLog:
> > 
> > yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* gdb.texinfo (Registers): Document info reg $reggroup feature.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* 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 <http://www.gnu.org/licenses/>.
> > +
> > +# 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 <reggroup>'.
> > +
> > +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 <reggroup> will return something like the following:
> > +    #
> > +    # r0             0x0      0^M
> > +    # r1             0x7fdffc 0x7fdffc^M
> > +    # r2             0x7fe000 0x7fe000^M
> > +    # npc            0x23a8   0x23a8 <main+12>^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 <repeats 16 times>}, ...
> 
> 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



More information about the Gdb-patches mailing list