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

[Stafford Horne <shorne@gmail.com>]

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