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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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.

> +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.

> +	    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.

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.

> +
> +    # 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.

> +	    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

> +	}
> +	-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.

> +
> +# 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 .*"

Simon