Re: [PATCH 5/5] Add new test internalvar.exp

[Pedro Alves <palves at redhat dot com>]

On 07/16/2015 07:51 PM, Simon Marchi wrote:
> I wrote this test While doing the work that lead to the previous patch,
> so I thought I would contribute it upstream.  From what I can see, there
> is no test currently to verify operations on internal variables (please
> point me to it if I'm wrong).

Pedantically, it'd be better to call these convenience vars.

Convenience var specifics are tested in gdbvars.exp.  Likely we have this
more covered on the C++ tests.  Something like what you're testing, but
for bitfields, is done in bitfields.exp.  structs.exp is for infcalls, which IIRC
your target doesn't do.  I guess one could hack a bug in the value field access
code somewhere in value.c/valops.c/valprint.c, and the find the test that
has more failures.  :-)

In any case, more tests can't hurt.  :-)

AFAICS, this is really more about getting value field offsets and contents
right than convenience vars per se, which end up just exercising the
routine value paths.

You could put these in gdb.base/struct3.c|.exp, which if you look
at struct3.c, it's structure is similar, with an inner and outer struct too.
Otherwise, I'd suggest renaming the test in the value
direction -- value-fields.exp, value-units.exp, value-offsets.exp or
some such.

I'd also suggest making the describing comment be something like:

# Test accessing different fields of structures, inner structures,
# arrays, etc., and make sure GDB prints the right contents.  Handy to
# exercise the value machinery code that handles host vs target bytes/memory
# units.

Anyway, the test code itself looks good to me, with:

> ---
>  gdb/testsuite/gdb.base/internalvar.c   | 45 ++++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/internalvar.exp | 42 +++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/internalvar.c
>  create mode 100644 gdb/testsuite/gdb.base/internalvar.exp
> 
> diff --git a/gdb/testsuite/gdb.base/internalvar.c b/gdb/testsuite/gdb.base/internalvar.c
> new file mode 100644
> index 0000000..2aadc11
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/internalvar.c
> @@ -0,0 +1,45 @@

Copyright header missing.

Thanks,
Pedro Alves