[2/2] handle VLA in a struct or union

  It is valid in C to have a VLA in a struct or union type, but gdb did
not handle this.

This patch adds support for these cases in the obvious way.

Built and regtested on x86-64 Fedora 20.
New tests included.

However, before this goes in, I'd like to understand the oddity
pointed out by a change in is_dynamic_type.  That is, this test:

	      /* This can happen with Ada for reasons unknown.  */
	      && TYPE_FIELD_TYPE (type, i) != NULL

is needed to avoid a crash with the Ada "iwide.exp" test.  This type:

    (top-gdb) p real_type.main_type.name
    $15 = 0x7ffff0354c6d "ada__tags__type_specific_data___XVE"

... has a seemingly invalid field:

    (top-gdb) p real_type.main_type.nfields
    $9 = 13
[...]
    (top-gdb) p real_type.main_type.flds_bnds.fields[12]
    $12 = {
      loc = {
	bitpos = 576,
	enumval = 576,
	physaddr = 576,
	physname = 0x240 <Address 0x240 out of bounds>,
	dwarf_block = 0x240
      },
      artificial = 0,
      loc_kind = FIELD_LOC_KIND_BITPOS,
      bitsize = 0,
      type = 0x0,
      name = 0x0
    }

Joel, can you comment?  Thanks.

2014-05-08  Tom Tromey  <tromey@redhat.com>

	* gdbtypes.c (is_dynamic_type, resolve_dynamic_type)
	<TYPE_CODE_STRUCT, TYPE_CODE_UNION>: New cases.

2014-05-08  Tom Tromey  <tromey@redhat.com>

	* gdb.base/vla-datatypes.exp: Add tests for VLA-in-structure and
	VLA-in-union.
	* gdb.base/vla-datatypes.c (vla_factory): Add vla_struct,
	vla_union types.  Initialize objects of those types and compute
	their sizes.
---
 gdb/ChangeLog                            |  5 ++
 gdb/gdbtypes.c                           | 94 ++++++++++++++++++++++++++++++++
 gdb/testsuite/ChangeLog                  |  8 +++
 gdb/testsuite/gdb.base/vla-datatypes.c   | 16 ++++++
 gdb/testsuite/gdb.base/vla-datatypes.exp | 14 +++++
 5 files changed, 137 insertions(+)
  

> On May 8, 2014, at 11:46 AM, Tom Tromey <tromey@redhat.com> wrote:
> 
> It is valid in C to have a VLA in a struct or union type, but gdb did
> not handle this.

In GNU C only. It is extension.

Thanks,
Andrew

> 
> This patch adds support for these cases in the obvious way.
> 
> Built and regtested on x86-64 Fedora 20.
> New tests included.
> 
> However, before this goes in, I'd like to understand the oddity
> pointed out by a change in is_dynamic_type.  That is, this test:
> 
>          /* This can happen with Ada for reasons unknown.  */
>          && TYPE_FIELD_TYPE (type, i) != NULL
> 
> is needed to avoid a crash with the Ada "iwide.exp" test.  This type:
> 
>    (top-gdb) p real_type.main_type.name
>    $15 = 0x7ffff0354c6d "ada__tags__type_specific_data___XVE"
> 
> ... has a seemingly invalid field:
> 
>    (top-gdb) p real_type.main_type.nfields
>    $9 = 13
> [...]
>    (top-gdb) p real_type.main_type.flds_bnds.fields[12]
>    $12 = {
>      loc = {
>    bitpos = 576,
>    enumval = 576,
>    physaddr = 576,
>    physname = 0x240 <Address 0x240 out of bounds>,
>    dwarf_block = 0x240
>      },
>      artificial = 0,
>      loc_kind = FIELD_LOC_KIND_BITPOS,
>      bitsize = 0,
>      type = 0x0,
>      name = 0x0
>    }
> 
> Joel, can you comment?  Thanks.
> 
> 2014-05-08  Tom Tromey  <tromey@redhat.com>
> 
>    * gdbtypes.c (is_dynamic_type, resolve_dynamic_type)
>    <TYPE_CODE_STRUCT, TYPE_CODE_UNION>: New cases.
> 
> 2014-05-08  Tom Tromey  <tromey@redhat.com>
> 
>    * gdb.base/vla-datatypes.exp: Add tests for VLA-in-structure and
>    VLA-in-union.
>    * gdb.base/vla-datatypes.c (vla_factory): Add vla_struct,
>    vla_union types.  Initialize objects of those types and compute
>    their sizes.
> ---
> gdb/ChangeLog                            |  5 ++
> gdb/gdbtypes.c                           | 94 ++++++++++++++++++++++++++++++++
> gdb/testsuite/ChangeLog                  |  8 +++
> gdb/testsuite/gdb.base/vla-datatypes.c   | 16 ++++++
> gdb/testsuite/gdb.base/vla-datatypes.exp | 14 +++++
> 5 files changed, 137 insertions(+)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 95b861e..59f354a 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1636,6 +1636,20 @@ is_dynamic_type (struct type *type)
>      return 1;
>    return is_dynamic_type (TYPE_TARGET_TYPE (type));
>       }
> +
> +    case TYPE_CODE_STRUCT:
> +    case TYPE_CODE_UNION:
> +      {
> +    int i;
> +
> +    for (i = 0; i < TYPE_NFIELDS (type); ++i)
> +      if (!field_is_static (&TYPE_FIELD (type, i))
> +          /* This can happen with Ada for reasons unknown.  */
> +          && TYPE_FIELD_TYPE (type, i) != NULL
> +          && is_dynamic_type (TYPE_FIELD_TYPE (type, i)))
> +        return 1;
> +      }
> +      break;
>     }
> 
>   return 0;
> @@ -1753,6 +1767,86 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
>       case TYPE_CODE_RANGE:
>    resolved_type = resolve_dynamic_range (type);
>    break;
> +
> +    case TYPE_CODE_UNION:
> +      {
> +    int i;
> +    unsigned int max_len;
> +
> +    resolved_type = copy_type (type);
> +    TYPE_FIELDS (resolved_type)
> +      = TYPE_ALLOC (resolved_type,
> +            TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> +    memcpy (TYPE_FIELDS (resolved_type),
> +        TYPE_FIELDS (type),
> +        TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> +    for (i = 0; i < TYPE_NFIELDS (resolved_type); ++i)
> +      {
> +        struct type *t;
> +
> +        if (field_is_static (&TYPE_FIELD (type, i)))
> +          continue;
> +
> +        t = resolve_dynamic_type (TYPE_FIELD_TYPE (resolved_type, i),
> +                      addr);
> +
> +        TYPE_FIELD_TYPE (resolved_type, i) = t;
> +        if (TYPE_LENGTH (t) > max_len)
> +          max_len = TYPE_LENGTH (t);
> +      }
> +
> +    TYPE_LENGTH (resolved_type) = max_len;
> +      }
> +      break;
> +
> +    case TYPE_CODE_STRUCT:
> +      {
> +    int i;
> +    int vla_field = TYPE_NFIELDS (type) - 1;
> +
> +    resolved_type = copy_type (type);
> +    TYPE_FIELDS (resolved_type)
> +      = TYPE_ALLOC (resolved_type,
> +            TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> +    memcpy (TYPE_FIELDS (resolved_type),
> +        TYPE_FIELDS (type),
> +        TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> +    for (i = 0; i < TYPE_NFIELDS (resolved_type); ++i)
> +      {
> +        struct type *t;
> +
> +        if (field_is_static (&TYPE_FIELD (type, i)))
> +          continue;
> +
> +        t = resolve_dynamic_type (TYPE_FIELD_TYPE (resolved_type, i),
> +                      addr);
> +
> +        /* This is a bit odd.  We do not support a VLA in any
> +           position of a struct except for the last.  GCC does
> +           have an extension that allows a VLA in the middle of a
> +           structure, but the DWARF it emits is relatively useless
> +           to us, so we can't represent such a type properly --
> +           and even if we could, we do not have enough information
> +           to redo structure layout anyway.  Nevertheless, we
> +           check all the fields in case something odd slips
> +           through, since it's better to see an error than
> +           incorrect results.  */
> +        if (t != TYPE_FIELD_TYPE (resolved_type, i)
> +        && i != vla_field)
> +          error (_("Attempt to resolve a variably-sized type which appears "
> +               "in the interior of a structure type"));
> +
> +        TYPE_FIELD_TYPE (resolved_type, i) = t;
> +      }
> +
> +    /* Due to the above restrictions we can successfully compute
> +       the size of the resulting structure here, as the offset of
> +       the final field plus its size.  */
> +    TYPE_LENGTH (resolved_type)
> +      = (TYPE_FIELD_BITPOS (resolved_type, vla_field) / TARGET_CHAR_BIT
> +         + TYPE_LENGTH (TYPE_FIELD_TYPE (resolved_type, vla_field)));
> +      }
> +      break;
>     }
> 
>   return resolved_type;
> diff --git a/gdb/testsuite/gdb.base/vla-datatypes.c b/gdb/testsuite/gdb.base/vla-datatypes.c
> index 51e342e..8561a4e 100644
> --- a/gdb/testsuite/gdb.base/vla-datatypes.c
> +++ b/gdb/testsuite/gdb.base/vla-datatypes.c
> @@ -46,6 +46,18 @@ vla_factory (int n)
>   BAR             bar_vla[n];
>   int i;
> 
> +  struct vla_struct
> +  {
> +    int something;
> +    int vla_field[n];
> +  } vla_struct_object;
> +
> +  union vla_union
> +  {
> +    int vla_field[n];
> +  } vla_union_object;
> +
> +  vla_struct_object.something = n;
>   for (i = 0; i < n; i++)
>     {
>       int_vla[i] = i*2;
> @@ -61,6 +73,8 @@ vla_factory (int n)
>       foo_vla[i].a = i*2;
>       bar_vla[i].x = i*2;
>       bar_vla[i].y.a = i*2;
> +      vla_struct_object.vla_field[i] = i*2;
> +      vla_union_object.vla_field[i] = i*2;
>     }
> 
>   size_t int_size        = sizeof(int_vla);     /* vlas_filled */
> @@ -74,6 +88,8 @@ vla_factory (int n)
>   size_t uchar_size      = sizeof(unsigned_char_vla);
>   size_t foo_size        = sizeof(foo_vla);
>   size_t bar_size        = sizeof(bar_vla);
> +  size_t vla_struct_object_size = sizeof(vla_struct_object);
> +  size_t vla_union_object_size = sizeof(vla_union_object);
> 
>   return;                                 /* break_end_of_vla_factory */
> }
> diff --git a/gdb/testsuite/gdb.base/vla-datatypes.exp b/gdb/testsuite/gdb.base/vla-datatypes.exp
> index 8247658..36af38d 100644
> --- a/gdb/testsuite/gdb.base/vla-datatypes.exp
> +++ b/gdb/testsuite/gdb.base/vla-datatypes.exp
> @@ -53,6 +53,10 @@ gdb_test "print foo_vla" \
> gdb_test "print bar_vla" \
>          "\\\{\\\{x = 0, y = \\\{a = 0\\\}\\\}, \\\{x = 2, y = \\\{a = 2\\\}\\\}, \\\{x = 4, y = \\\{a = 4\\\}\\\}, \\\{x = 6, y = \\\{a = 6\\\}\\\}, \\\{x = 8, y = \\\{a = 8\\\}\\\}\\\}" \
>          "print bar_vla"
> +gdb_test "print vla_struct_object" \
> +    "\\\{something = 5, vla_field = \\\{0, 2, 4, 6, 8\\\}\\\}"
> +gdb_test "print vla_union_object" \
> +    "\\\{vla_field = \\\{0, 2, 4, 6, 8\\\}\\\}"
> 
> # Check whatis of VLA's.
> gdb_test "whatis int_vla" "type = int \\\[5\\\]" "whatis int_vla"
> @@ -74,6 +78,8 @@ gdb_test "whatis unsigned_char_vla" "type = unsigned char \\\[5\\\]" \
>          "whatis unsigned_char_vla"
> gdb_test "whatis foo_vla" "type = struct foo \\\[5\\\]" "whatis foo_vla"
> gdb_test "whatis bar_vla" "type = BAR \\\[5\\\]" "whatis bar_vla"
> +gdb_test "whatis vla_struct_object" "type = struct vla_struct"
> +gdb_test "whatis vla_union_object" "type = union vla_union"
> 
> # Check ptype of VLA's.
> gdb_test "ptype int_vla" "type = int \\\[5\\\]" "ptype int_vla"
> @@ -96,6 +102,10 @@ gdb_test "ptype foo_vla" "type = struct foo {\r\n\\s+int a;\r\n} \\\[5\\\]" \
> gdb_test "ptype bar_vla" \
>          "type = struct bar {\r\n\\s+int x;\r\n\\s+struct foo y;\r\n} \\\[5\\\]" \
>          "ptype bar_vla"
> +gdb_test "ptype vla_struct_object" \
> +    "type = struct vla_struct {\r\n\\s+int something;\r\n\\s+int vla_field\\\[5\\\];\r\n}"
> +gdb_test "ptype vla_union_object" \
> +    "type = union vla_union {\r\n\\s+int vla_field\\\[5\\\];\r\n}"
> 
> # Check the size of the VLA's.
> gdb_breakpoint [gdb_get_line_number "break_end_of_vla_factory"]
> @@ -119,6 +129,10 @@ gdb_test "print uchar_size == sizeof(unsigned_char_vla)" " = 1" \
>          "size of unsigned_char_vla"
> gdb_test "print foo_size == sizeof(foo_vla)" " = 1" "size of foo_vla"
> gdb_test "print bar_size == sizeof(bar_vla)" " = 1" "size of bar_vla"
> +gdb_test "print vla_struct_object_size == sizeof(vla_struct_object)" \
> +    " = 1" "size of vla_struct_object"
> +gdb_test "print vla_union_object_size == sizeof(vla_union_object)" \
> +    " = 1" "size of vla_union_object"
> 
> # Check side effects for sizeof argument.
> set sizeof_int [get_sizeof "int" 4]
> -- 
> 1.9.0
>
  

>> It is valid in C to have a VLA in a struct or union type, but gdb did
>> not handle this.

Andrew> In GNU C only. It is extension.

Ah, well, shows what I know.
I'll change the commit text; but I don't think this otherwise affects
the patch.

Tom
  

> > On May 8, 2014, at 11:46 AM, Tom Tromey <tromey@redhat.com> wrote:
> > 
> >   return 0;
> > @@ -1753,6 +1767,86 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
> >       case TYPE_CODE_RANGE:
> >    resolved_type = resolve_dynamic_range (type);
> >    break;
> > +
> > +    case TYPE_CODE_UNION:
> > +      {
> > +    int i;
> > +    unsigned int max_len;
Shouldn't max_len be initialised to 0 to start with ?

> > +
> > +    resolved_type = copy_type (type);
> > +    TYPE_FIELDS (resolved_type)
> > +      = TYPE_ALLOC (resolved_type,
> > +            TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> > +    memcpy (TYPE_FIELDS (resolved_type),
> > +        TYPE_FIELDS (type),
> > +        TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> > +    for (i = 0; i < TYPE_NFIELDS (resolved_type); ++i)
> > +      {
> > +        struct type *t;
> > +
> > +        if (field_is_static (&TYPE_FIELD (type, i)))
> > +          continue;
> > +
> > +        t = resolve_dynamic_type (TYPE_FIELD_TYPE (resolved_type, i),
> > +                      addr);
> > +
> > +        TYPE_FIELD_TYPE (resolved_type, i) = t;
> > +        if (TYPE_LENGTH (t) > max_len)
> > +          max_len = TYPE_LENGTH (t);
> > +      }
> > +
> > +    TYPE_LENGTH (resolved_type) = max_len;
> > +      }
> > +      break;
> > +
> > +    case TYPE_CODE_STRUCT:
> > +      {
> > +    int i;
> > +    int vla_field = TYPE_NFIELDS (type) - 1;
> > +
> > +    resolved_type = copy_type (type);
> > +    TYPE_FIELDS (resolved_type)
> > +      = TYPE_ALLOC (resolved_type,
> > +            TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> > +    memcpy (TYPE_FIELDS (resolved_type),
> > +        TYPE_FIELDS (type),
> > +        TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> > +    for (i = 0; i < TYPE_NFIELDS (resolved_type); ++i)
> > +      {
> > +        struct type *t;
> > +
> > +        if (field_is_static (&TYPE_FIELD (type, i)))
> > +          continue;
> > +
> > +        t = resolve_dynamic_type (TYPE_FIELD_TYPE (resolved_type, i),
> > +                      addr);
> > +
> > +        /* This is a bit odd.  We do not support a VLA in any
> > +           position of a struct except for the last.  GCC does
> > +           have an extension that allows a VLA in the middle of a
> > +           structure, but the DWARF it emits is relatively useless
> > +           to us, so we can't represent such a type properly --
> > +           and even if we could, we do not have enough information
> > +           to redo structure layout anyway.  Nevertheless, we
> > +           check all the fields in case something odd slips
> > +           through, since it's better to see an error than
> > +           incorrect results.  */
> > +        if (t != TYPE_FIELD_TYPE (resolved_type, i)
> > +        && i != vla_field)
> > +          error (_("Attempt to resolve a variably-sized type which appears "
> > +               "in the interior of a structure type"));
> > +
> > +        TYPE_FIELD_TYPE (resolved_type, i) = t;
> > +      }
> > +
> > +    /* Due to the above restrictions we can successfully compute
> > +       the size of the resulting structure here, as the offset of
> > +       the final field plus its size.  */
> > +    TYPE_LENGTH (resolved_type)
> > +      = (TYPE_FIELD_BITPOS (resolved_type, vla_field) / TARGET_CHAR_BIT
> > +         + TYPE_LENGTH (TYPE_FIELD_TYPE (resolved_type, vla_field)));
> > +      }
> > +      break;
> >     }
> > 
> >   return resolved_type;
> > diff --git a/gdb/testsuite/gdb.base/vla-datatypes.c b/gdb/testsuite/gdb.base/vla-datatypes.c
> > index 51e342e..8561a4e 100644
> > --- a/gdb/testsuite/gdb.base/vla-datatypes.c
> > +++ b/gdb/testsuite/gdb.base/vla-datatypes.c
> > @@ -46,6 +46,18 @@ vla_factory (int n)
> >   BAR             bar_vla[n];
> >   int i;
> > 
> > +  struct vla_struct
> > +  {
> > +    int something;
> > +    int vla_field[n];
> > +  } vla_struct_object;
> > +
> > +  union vla_union
> > +  {
> > +    int vla_field[n];
> > +  } vla_union_object;
> > +
> > +  vla_struct_object.something = n;
> >   for (i = 0; i < n; i++)
> >     {
> >       int_vla[i] = i*2;
> > @@ -61,6 +73,8 @@ vla_factory (int n)
> >       foo_vla[i].a = i*2;
> >       bar_vla[i].x = i*2;
> >       bar_vla[i].y.a = i*2;
> > +      vla_struct_object.vla_field[i] = i*2;
> > +      vla_union_object.vla_field[i] = i*2;
> >     }
> > 
> >   size_t int_size        = sizeof(int_vla);     /* vlas_filled */
> > @@ -74,6 +88,8 @@ vla_factory (int n)
> >   size_t uchar_size      = sizeof(unsigned_char_vla);
> >   size_t foo_size        = sizeof(foo_vla);
> >   size_t bar_size        = sizeof(bar_vla);
> > +  size_t vla_struct_object_size = sizeof(vla_struct_object);
> > +  size_t vla_union_object_size = sizeof(vla_union_object);
> > 
> >   return;                                 /* break_end_of_vla_factory */
> > }
> > diff --git a/gdb/testsuite/gdb.base/vla-datatypes.exp b/gdb/testsuite/gdb.base/vla-datatypes.exp
> > index 8247658..36af38d 100644
> > --- a/gdb/testsuite/gdb.base/vla-datatypes.exp
> > +++ b/gdb/testsuite/gdb.base/vla-datatypes.exp
> > @@ -53,6 +53,10 @@ gdb_test "print foo_vla" \
> > gdb_test "print bar_vla" \
> >          "\\\{\\\{x = 0, y = \\\{a = 0\\\}\\\}, \\\{x = 2, y = \\\{a = 2\\\}\\\}, \\\{x = 4, y = \\\{a = 4\\\}\\\}, \\\{x = 6, y = \\\{a = 6\\\}\\\}, \\\{x = 8, y = \\\{a = 8\\\}\\\}\\\}" \
> >          "print bar_vla"
> > +gdb_test "print vla_struct_object" \
> > +    "\\\{something = 5, vla_field = \\\{0, 2, 4, 6, 8\\\}\\\}"
> > +gdb_test "print vla_union_object" \
> > +    "\\\{vla_field = \\\{0, 2, 4, 6, 8\\\}\\\}"
> > 
> > # Check whatis of VLA's.
> > gdb_test "whatis int_vla" "type = int \\\[5\\\]" "whatis int_vla"
> > @@ -74,6 +78,8 @@ gdb_test "whatis unsigned_char_vla" "type = unsigned char \\\[5\\\]" \
> >          "whatis unsigned_char_vla"
> > gdb_test "whatis foo_vla" "type = struct foo \\\[5\\\]" "whatis foo_vla"
> > gdb_test "whatis bar_vla" "type = BAR \\\[5\\\]" "whatis bar_vla"
> > +gdb_test "whatis vla_struct_object" "type = struct vla_struct"
> > +gdb_test "whatis vla_union_object" "type = union vla_union"
> > 
> > # Check ptype of VLA's.
> > gdb_test "ptype int_vla" "type = int \\\[5\\\]" "ptype int_vla"
> > @@ -96,6 +102,10 @@ gdb_test "ptype foo_vla" "type = struct foo {\r\n\\s+int a;\r\n} \\\[5\\\]" \
> > gdb_test "ptype bar_vla" \
> >          "type = struct bar {\r\n\\s+int x;\r\n\\s+struct foo y;\r\n} \\\[5\\\]" \
> >          "ptype bar_vla"
> > +gdb_test "ptype vla_struct_object" \
> > +    "type = struct vla_struct {\r\n\\s+int something;\r\n\\s+int vla_field\\\[5\\\];\r\n}"
> > +gdb_test "ptype vla_union_object" \
> > +    "type = union vla_union {\r\n\\s+int vla_field\\\[5\\\];\r\n}"
> > 
> > # Check the size of the VLA's.
> > gdb_breakpoint [gdb_get_line_number "break_end_of_vla_factory"]
> > @@ -119,6 +129,10 @@ gdb_test "print uchar_size == sizeof(unsigned_char_vla)" " = 1" \
> >          "size of unsigned_char_vla"
> > gdb_test "print foo_size == sizeof(foo_vla)" " = 1" "size of foo_vla"
> > gdb_test "print bar_size == sizeof(bar_vla)" " = 1" "size of bar_vla"
> > +gdb_test "print vla_struct_object_size == sizeof(vla_struct_object)" \
> > +    " = 1" "size of vla_struct_object"
> > +gdb_test "print vla_union_object_size == sizeof(vla_union_object)" \
> > +    " = 1" "size of vla_union_object"
> > 
> > # Check side effects for sizeof argument.
> > set sizeof_int [get_sizeof "int" 4]
> > -- 
> > 1.9.0
> >
  

> However, before this goes in, I'd like to understand the oddity
> pointed out by a change in is_dynamic_type.  That is, this test:
> 
> 	      /* This can happen with Ada for reasons unknown.  */
> 	      && TYPE_FIELD_TYPE (type, i) != NULL
> 
> is needed to avoid a crash with the Ada "iwide.exp" test.  This type:
> 
>     (top-gdb) p real_type.main_type.name
>     $15 = 0x7ffff0354c6d "ada__tags__type_specific_data___XVE"
> 
> ... has a seemingly invalid field:
> 
>     (top-gdb) p real_type.main_type.nfields
>     $9 = 13
> [...]
>     (top-gdb) p real_type.main_type.flds_bnds.fields[12]
>     $12 = {
>       loc = {
> 	bitpos = 576,
> 	enumval = 576,
> 	physaddr = 576,
> 	physname = 0x240 <Address 0x240 out of bounds>,
> 	dwarf_block = 0x240
>       },
>       artificial = 0,
>       loc_kind = FIELD_LOC_KIND_BITPOS,
>       bitsize = 0,
>       type = 0x0,
>       name = 0x0
>     }
> 
> Joel, can you comment?  Thanks.

It's actually not the only testcase I see being impacted if I remove
the check. What happens is that we're in the middle of resolving
this very type (what we have been calling creating a fixed version
of the type). While doing so, we do...

    if (dval0 == NULL)
      {
        /* rtype's length is computed based on the run-time
           value of discriminants.  If the discriminants are not
           initialized, the type size may be completely bogus and
           GDB may fail to allocate a value for it.  So check the
           size first before creating the value.  */
        check_size (rtype);
        dval = value_from_contents_and_address (rtype, valaddr, address);
        rtype = value_type (dval);
      }

... where rtype is the resolved type being constructed, and therefore
a type that isn't completely consistent yet. the call to
value_from_contents_and_address eventually leads to the is_dynamic_type
call.

I think I need to review that code, as I do that a litte little
later in the function again. I'll send another update when I have
more info on this.
  

Philippe> Shouldn't max_len be initialised to 0 to start with ?

Yeah, thanks.

Tom
  

Joel> ... where rtype is the resolved type being constructed, and therefore
Joel> a type that isn't completely consistent yet. the call to
Joel> value_from_contents_and_address eventually leads to the is_dynamic_type
Joel> call.

Thanks Joel.

Joel> I think I need to review that code, as I do that a litte little
Joel> later in the function again. I'll send another update when I have
Joel> more info on this.

I will try to look at it again tomorrow, don't spend too much effort on
it.

Tom
  

> Joel> I think I need to review that code, as I do that a litte little
> Joel> later in the function again. I'll send another update when I have
> Joel> more info on this.
> 
> I will try to look at it again tomorrow, don't spend too much effort on
> it.

I think the call to value_from_contents_and_address with the
partially constructed type is borderline, to say the least.
So I think it's worth me looking at it anyways.

So far, I've tried to build the dval using the original type,
but that doesn't seem to be good enough. That's all I could do today,
but I will have another look at it tomorrow.
  

> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 95b861e..59f354a 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1636,6 +1636,20 @@ is_dynamic_type (struct type *type)
>  	  return 1;
>  	return is_dynamic_type (TYPE_TARGET_TYPE (type));
>        }
> +
> +    case TYPE_CODE_STRUCT:
> +    case TYPE_CODE_UNION:
> +      {
> +	int i;
> +
> +	for (i = 0; i < TYPE_NFIELDS (type); ++i)
> +	  if (!field_is_static (&TYPE_FIELD (type, i))
> +	      /* This can happen with Ada for reasons unknown.  */
> +	      && TYPE_FIELD_TYPE (type, i) != NULL
> +	      && is_dynamic_type (TYPE_FIELD_TYPE (type, i)))
> +	    return 1;
> +      }
> +      break;
>      }
> 
>    return 0;
> @@ -1753,6 +1767,86 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
>        case TYPE_CODE_RANGE:
>  	resolved_type = resolve_dynamic_range (type);
>  	break;
> +
> +    case TYPE_CODE_UNION:
>
As Joel has moved the heavy work out of the cases into separate function
how about doing the same for arrays and unions (resolve_dynamic_compound)?

> +      {
> +	int i;
> +	unsigned int max_len;
> +
> +	resolved_type = copy_type (type);
> +	TYPE_FIELDS (resolved_type)
> +	  = TYPE_ALLOC (resolved_type,
> +			TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> +	memcpy (TYPE_FIELDS (resolved_type),
> +		TYPE_FIELDS (type),
> +		TYPE_NFIELDS (resolved_type) * sizeof (struct field));
> +	for (i = 0; i < TYPE_NFIELDS (resolved_type); ++i)
> +	  {
> +	    struct type *t;
> +
> +	    if (field_is_static (&TYPE_FIELD (type, i)))
> +	      continue;
> +
> +	    t = resolve_dynamic_type (TYPE_FIELD_TYPE (resolved_type, i),
> +				      addr);
> +
> +	    TYPE_FIELD_TYPE (resolved_type, i) = t;
> +	    if (TYPE_LENGTH (t) > max_len)
> +	      max_len = TYPE_LENGTH (t);
> +	  }
> +
> +	TYPE_LENGTH (resolved_type) = max_len;
> +      }
> +      break;
> +
> +    case TYPE_CODE_STRUCT:
> +      {
> +	int i;
> +	int vla_field = TYPE_NFIELDS (type) - 1;
> +
How about adding a gdb_assert to ensure NFIELDS is > 0. I know this cannot
happen in this particular case but in others it might and one may assume
that a struct has at least a member which is not the case.

> diff --git a/gdb/testsuite/gdb.base/vla-datatypes.c b/gdb/testsuite/gdb.base/vla-
> datatypes.c
> index 51e342e..8561a4e 100644
> --- a/gdb/testsuite/gdb.base/vla-datatypes.c
> +++ b/gdb/testsuite/gdb.base/vla-datatypes.c
> @@ -46,6 +46,18 @@ vla_factory (int n)
>    BAR             bar_vla[n];
>    int i;
> 
> +  struct vla_struct
> +  {
> +    int something;
> +    int vla_field[n];
> +  } vla_struct_object;
> +
> +  union vla_union
> +  {
> +    int vla_field[n];
> +  } vla_union_object;
> +
As you have pointed out the limitation of vla within arrays, this might
as well serve as a test case that we deal with it.

 -Sanimir

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
  

Hey Tom,

> > Joel> I think I need to review that code, as I do that a litte little
> > Joel> later in the function again. I'll send another update when I have
> > Joel> more info on this.
[...]
> So far, I've tried to build the dval using the original type,
> but that doesn't seem to be good enough. That's all I could do today,
> but I will have another look at it tomorrow.

This is really annoying :-(. We can't build the dval using
the original type because the length of that type is bogus
by construction. The errors I am seeing when I try that happen
when trying to print tagged objects containing dynamic structures
whose size depend on a discriminant. For instance:

 | type Variant_Root (R : Integer) is tagged record
 |    Initial : String (1 .. R) := (others => 'j');
 | end record;
 |
 | type Variant_Derived is new Variant_Root with record
 |    Attribute : Integer := 1;
 | end record;
 |
 | V : Variant_Derived (2);

This declares the Ada equivalent of a C++ class called Variant_Root
inside which we have an array "Initial" whose index range is 1 .. R,
and therefore dependent of the value of that field (a discriminant).
It then creates a new "class" Variant_Derived which is derived from
Variant_Root and adds an extra field Attribute. I don't think that
the extra derivation is really needed to reproduce the problem, but
that's what I investigated...

In any case, what happens is that we have an ___XVE type as per
the GNAT encoding (see exp_dbug.ads in gcc/ada), and the size of
that type cannot be used. It so happens to have a size of 8, but
it could as easily be 0, or -1. And unfortunately, the field we
are interested in ("R") is at offset 8. So when we create the dval
using the original type, and try to get the field at offset 8,
we start reading undefined memory...  (one of these days, we should
guard against this)

Perhaps we might think of having a new value_from_contents_and_address
that produces the value without resolving the type? If Ada is the only
user, as it would be, we could possibly put implement that in ada-lang,
I think. Kind of ugly, but this would be medium-term only, since we are
slowly trying to get rid of the GNAT encodings.
  

Sanimir> As Joel has moved the heavy work out of the cases into separate
Sanimir> function how about doing the same for arrays and unions
Sanimir> (resolve_dynamic_compound)?

Done on my branch.

Sanimir> How about adding a gdb_assert to ensure NFIELDS is > 0. I know
Sanimir> this cannot happen in this particular case but in others it
Sanimir> might and one may assume that a struct has at least a member
Sanimir> which is not the case.

Also done.

Thanks for your review.

Sanimir> As you have pointed out the limitation of vla within arrays,
Sanimir> this might as well serve as a test case that we deal with it.

I didn't understand this one, sorry...

FWIW I filed a GCC bug about the issues with VLAs in the middle of a
struct.  I would not, however, expect a quick resolution to this issue,
considering the obscurity of the extension.

Tom
  

> Sanimir> As you have pointed out the limitation of vla within arrays,
> Sanimir> this might as well serve as a test case that we deal with it.
> 
> I didn't understand this one, sorry...
> 
Add a test for the following limitation:

> +	    if (t != TYPE_FIELD_TYPE (resolved_type, i)
> +		&& i != vla_field)
> +	      error (_("Attempt to resolve a variably-sized type which appears "
> +		       "in the interior of a structure type"));
>

 -Sanimir
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
  

>>>>> "Sanimir" == Agovic, Sanimir <sanimir.agovic@intel.com> writes:

Sanimir> As you have pointed out the limitation of vla within arrays,
Sanimir> this might as well serve as a test case that we deal with it.
>> 
>> I didn't understand this one, sorry...
>> 
Sanimir> Add a test for the following limitation:

Ok, I see.

I suppose I can add a test for the error case.

But, I don't like to add new [kx]fails to the tree, if that's what you
meant.

Tom
  

> I suppose I can add a test for the error case.
> 
> But, I don't like to add new [kx]fails to the tree, if that's what you
> meant.
> 
A 'pass' is fine. I expect some changes around resolve_dynamic_* for Fortran
and Ada vla support thus a good test coverage for C99 is beneficial.

 -Sanimir

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052