Re: [RFA/DWARF] Add array DW_AT_bit_stride and DW_AT_byte_stride support

[Joel Brobecker <brobecker at adacore dot com>]

> On Wed, 2014-01-29 at 18:30 +0400, Joel Brobecker wrote:
> > gdb/ChangeLog:
> > 
> >         * gdbtypes.h (create_array_type_with_stride): Add declaration.
> >         * gdbtypes.c (create_array_type_with_stride): New function,
> >         renaming create_array_type, but with an added parameter
> >         called "bit_stride".
> >         (create_array_type): Re-implement using
> >         create_array_type_with_stride.
> >         * dwarf2read.c (read_array_type): Add support for DW_AT_byte_stride
> >         and DW_AT_bit_stride attributes.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> >         * gdb.dwarf2/arr-stride.c: New file.
> >         * gdb.dwarf2/arr-stride.exp: New file.
> 
> I know little of the GDB code base, just looking at it because it using
> some interesting DWARF, but this patch does look fine to me. Just a few
> small nitpicks below.

Thanks :)

> > +++ b/gdb/gdbtypes.c
> > @@ -951,14 +951,18 @@ get_array_bounds (struct type *type, LONGEST *low_bound, LONGEST *high_bound)
> >     Elements will be of type ELEMENT_TYPE, the indices will be of type
> >     RANGE_TYPE.
> >  
> > +   If BIT_STRIDE is non-null, build a packed array type whose element
> > +   size is BIT_STRIDE.  Otherwise, ignore this parameter.
> 
> s/non-null/not zero/ ?

Correct, and ...

> 
> > +/* Same as create_array_type_with_stride but with no bit_stride
> > +   (BIT_STRIDE = -1), thus building an unpacked array.  */
> 
> Shouldn't that be BIT_STRIDE = 0?

... correct! I will fix those.

> > diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> > +extern struct type *create_array_type_with_stride
> > +  (struct type *, struct type *, struct type *, unsigned int);
> > +
> >  extern struct type *create_array_type (struct type *, struct type *,
> >  				       struct type *);
> 
> Personally I would have just added the new argument and fixed up all
> callers, just so I had an overview of all the callers that might have a
> bit_stride concept that could be added.

I briefely considered this idea, but since most uses seem to be
creating arrays where the context suggested that bit-strides was
not relevant, I went for the dual approach; it also reduces the patch
size with noise from updating all calls (~30 of them). That being said,
if people prefer your suggestion, it can certainly be done easily.

-- 
Joel