Re: [PATCH v5 03/15] type: add c99 variable length array support

[Joel Brobecker <brobecker at adacore dot com>]

> I addressed all issues (see below) except for these two:
> 
> > > +  if (!attr_to_dynamic_prop (attr, die, cu, &high))
> > >      {
> > >        attr = dwarf2_attr (die, DW_AT_count, cu);
> > >        if (attr)
> > 
> > I know that in the testcase you are trying to support, the bounds
> > are necessarily starting at zero and therefore constant/non-dynamic.
> > But can you modify the function to also handle the DW_AT_lower_bound
> > the same way? Other languages such as Ada will need that also, and
> > that seems like the logical time to be doing it.
> >
> The motivation behind the c99 patch series is to introduce the core concept
> of dynamic properties and thus we like to keep it small. Based on that work
> the Fortran patch series will fill the gap.

OK with me if you prefer to handle this case as a follow up patch.

> > > +int
> > > +is_dynamic_type (const struct type *type)
> > > +{
> > > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > > +      && TYPE_NFIELDS (type) == 1)
> > > +    {
> > > +      const struct type *range_type = TYPE_INDEX_TYPE (type);
> > > +
> > > +      if (!has_static_range (TYPE_RANGE_DATA (range_type)))
> > > +	return 1;
> > > +    }
> > > +
> > > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > > +      || TYPE_CODE (type) == TYPE_CODE_PTR
> > > +      || TYPE_CODE (type) == TYPE_CODE_REF
> > > +      || TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
> > > +    return is_dynamic_type (check_typedef (TYPE_TARGET_TYPE (type)));
> > As discussed on IRC, I think that TYPE_NFIELDS should always
> > be 1 for TYPE_CODE_ARRAY. So let's transform that into an assert,
> > if you don't mind, and see what happens?
> > Can you explain why you included TYPE_CODE_PTR in the list?
> >
> > This is not clear to me based on the function's description.
> > The risk I see with this is trying to print the value of a pointer
> > to a dynamic array - the printing really only needs to display
> > the address, and not resolve the underlying array. Also, from
> > a logical standpoint, pointers are not dynamic types.
> >
> I may remove the pointer chasing from the patch series, but this means
> we won't support the following feature:
> 
>   1| int foo(int vla[n][m])
> 
>   (gdb) ptype vla
>   type = int (*)[] 
>   (gdb) print vla[0]
>   Cannot perform pointer math on incomplete types, try casting to a known type, or void *.
> 
> So we included TYPE_CODE_PTR to support such use case. 

OK. Do we have a test for this in the patches you propose?
I think it's important to have one. That way, if anyone like me
just tries to remove it, it'll show up in the testsuite.

> > Can you explain in this comment what the referenced type is?
> > If it helps, giving an example could be used.
> > 
> Done, see my inlined my changes:

Thank you. Small comment (see below):

> -- >8 --
> diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
> index 644c546..bcc02694 100644
> --- a/gdb/dwarf2loc.h
> +++ b/gdb/dwarf2loc.h
> @@ -144,13 +144,14 @@ struct dwarf2_loclist_baton
>  };
> 
>  /* A dynamic property is either expressed as a single location expression
> -   or a location list. If the property is a reference store its targeted
> -   type in TYPE.  */
> +   or a location list. If the property is an indirection, pointing to

                        ^^ missing second space after period.

Ok - I think we're ready for v6 of the patch series. Let's hope that'll
be it! :)

Thanks,
-- 
Joel