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

Stafford Horne
2017-06-08T23:27:55Z

On Thu, Jun 08, 2017 at 10:31:56PM +0200, Simon Marchi wrote:
> On 2017-06-08 00:15, Stafford Horne wrote:
> > Until now this feature has existed but was not documented. Adding docs
> > and tests.
>
> Hi Stafford,
>
> Thanks for doing this! It would be cool if you could improve the
> documentation printed by "help info registers", while at it.
>
> > gdb/ChangeLog:
> >
> > 2017-06-06 Stafford Horne <shorne@gmail.com>
> >
> > * doc/gdb.texinfo: Document info reg $reggroup feature.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 2017-06-06 Stafford Horne <shorne@gmail.com>
> >
> > * gdb.base/reggroups.c: New file.
> > * gdb.base/reggroups.exp: New file.
> > ---
> > gdb/doc/gdb.texinfo | 5 +++
> > gdb/testsuite/gdb.base/reggroups.c | 4 ++
> > gdb/testsuite/gdb.base/reggroups.exp | 71
> > ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 80 insertions(+)
> > 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 9fb70f6..a11db0c 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -10878,6 +10878,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 restiers in each of the specified
>
> Do you mean "registers"?

Yes. will fix

> > +@var{reggroup}. The @var{reggoup} can be any of those returned by
> > +@code{maint print reggroups}.
> > +
> > @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/testsuite/gdb.base/reggroups.c
> > b/gdb/testsuite/gdb.base/reggroups.c
> > new file mode 100644
> > index 0000000..f8b643a
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/reggroups.c
> > @@ -0,0 +1,4 @@
> > +int main()
>
> We try to use the GDB formatting style for the test code as well, so here it
> would be:
>
> int
> main ()
> {
> return 0;
> }

OK

> > +{
> > + return 0;
> > +}
> > diff --git a/gdb/testsuite/gdb.base/reggroups.exp
> > b/gdb/testsuite/gdb.base/reggroups.exp
> > new file mode 100644
> > index 0000000..fd51c50
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/reggroups.exp
> > @@ -0,0 +1,71 @@
> > +# This testcase is part of GDB, the GNU debugger.
> > +
> > +# Copyright 2014-2017 Free Software Foundation, Inc.
>
> Should it be 2017 only?

Yes. will fix

> > +
> > +# 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
> > +}
> > +
> > +proc fetch_reggroups {test} {
> > + global gdb_prompt
> > + global expect_out
> > +
> > + set reggroups {}
> > + set bad -1
> > + 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" {
> > + lappend reggroups $expect_out(1,string)
> > + exp_continue
> > + }
> > + -re ".*$gdb_prompt $" {
> > + if { [llength $reggroups] != 0 } {
> > + incr bad
> > + } else {
> > + set bad 1
> > + }
>
> I don't quite understand this. Why not simply "set bad 1"?
>
> Another option would be to call error to throw an exception, which will end
> the test abruptly:
>
> error "maint print reggroups: unexpected output"

Alright, I was copying this pattern from other test, I did think it was a
bit strange but it makese a bit of sense if you want to capture the failure
condition but continue with other tests.

Ill change it as you suggest.

> You could also assert that there is at least one reggroup with something
> like:
>
> gdb_assert { [llength $reggroups] > 0 } "at least one reggroup"

OK, ill look at that.

> > + }
> > + }
> > +
> > + if {$bad} {
> > + fail $test
> > + return {}
> > + }
> >
> > +
> > + pass $test
> > + return $reggroups
> > +}
> > +
> > +set reggroups [fetch_reggroups "fetch reggroups"]
> > +set reggroup ""
>
> You don't need to declare reggroup.

OK

> > +
> > +foreach reggroup [lrange $reggroups 0 end] {
>
> And this can be simply:
>
> foreach reggroup $reggroups

OK

> > + gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
> > +}

Thanks for reviewing, I dont write much expect code, this is helpful.

-Stafford