This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
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