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

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

[Tom Tromey]

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(+)

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

[PATCH 0/2] VLA in a struct or union

[Tom Tromey]

These patches fix the use of a VLA in a struct or union.  I don't
recall whether this was addressed in the VLA patch series, but it's
definitely missing from gdb master.

The first patch is just a minor cleanup.  The second patch implements
the feature.

Tom

[PATCH 1/2] minor cleanups in is_dynamic_type

[Tom Tromey]

I noticed that gdbtypes.c:is_dynamic_type has some unneeded "break"s.
This patch cleans up the function a bit, removing those and removing
the switch's default case so that the end of the function is a bit
clearer.

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

	* gdbtypes.c (is_dynamic_type): Remove unneeded "break"s.
---
 gdb/ChangeLog  |  4 ++++
 gdb/gdbtypes.c | 10 +++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 8e6631a..95b861e 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1625,7 +1625,6 @@ is_dynamic_type (struct type *type)
     {
     case TYPE_CODE_RANGE:
       return !has_static_range (TYPE_RANGE_DATA (type));
-      break;
 
     case TYPE_CODE_ARRAY:
       {
@@ -1635,14 +1634,11 @@ is_dynamic_type (struct type *type)
 	   or the elements it contains have a dynamic contents.  */
 	if (is_dynamic_type (TYPE_INDEX_TYPE (type)))
 	  return 1;
-	else
-	  return is_dynamic_type (TYPE_TARGET_TYPE (type));
-	break;
+	return is_dynamic_type (TYPE_TARGET_TYPE (type));
       }
-    default:
-      return 0;
-      break;
     }
+
+  return 0;
 }
 
 static struct type *
-- 
1.9.0

Re: [PATCH 2/2] handle VLA in a struct or union

[pinskia]

> 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
> 

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

>> 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

Re: [PATCH 2/2] handle VLA in a struct or union

[Philippe Waroquiers]

> > 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
> > 

Re: [PATCH 2/2] handle VLA in a struct or union

[Joel Brobecker]

> 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.

-- 
Joel

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

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

Yeah, thanks.

Tom

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

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

Re: [PATCH 2/2] handle VLA in a struct or union

[Joel Brobecker]

> 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.

-- 
Joel

RE: [PATCH 2/2] handle VLA in a struct or union

[Agovic, Sanimir]

> 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

Re: [PATCH 2/2] handle VLA in a struct or union

[Joel Brobecker]

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.

-- 
Joel

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

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

RE: [PATCH 2/2] handle VLA in a struct or union

[Agovic, Sanimir]

> 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

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

>>>>> "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

RE: [PATCH 2/2] handle VLA in a struct or union

[Agovic, Sanimir]

> 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

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

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

I put it in value.c, since I generally like to keep the whole module
together, having been bitten before by code living in the wrong spot.
I don't really care much though, in case you do.

Here's the updated patch, which I think addresses all review comments.

Tom

b/gdb/ChangeLog:
2014-05-21  Tom Tromey  <tromey@redhat.com>

	* ada-lang.c (ada_template_to_fixed_record_type_1): Use
	value_from_contents_and_address_unresolved.
	(ada_template_to_fixed_record_type_1): Likewise.
	(ada_which_variant_applies): Likewise.
	* value.h (value_from_contents_and_address_unresolved): Declare.
	* value.c (value_from_contents_and_address_unresolved): New
	function.
	* gdbtypes.c (is_dynamic_type, resolve_dynamic_type)
	<TYPE_CODE_STRUCT, TYPE_CODE_UNION>: New cases.
	(resolve_dynamic_struct, resolve_dynamic_union): New functions.

b/gdb/testsuite/ChangeLog:
2014-05-21  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,
	inner_vla_struct, vla_union types.  Initialize objects of those
	types and compute their sizes.

commit 81f06c4113ba4c263057000a3f3e576b0b36681c
Author: Tom Tromey <tromey@redhat.com>
Date:   Thu May 8 11:26:44 2014 -0600

    handle VLA in a struct or union
    
    It is valid in GNU 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.
    
    2014-05-21  Tom Tromey  <tromey@redhat.com>
    
    	* ada-lang.c (ada_template_to_fixed_record_type_1): Use
    	value_from_contents_and_address_unresolved.
    	(ada_template_to_fixed_record_type_1): Likewise.
    	(ada_which_variant_applies): Likewise.
    	* value.h (value_from_contents_and_address_unresolved): Declare.
    	* value.c (value_from_contents_and_address_unresolved): New
    	function.
    	* gdbtypes.c (is_dynamic_type, resolve_dynamic_type)
    	<TYPE_CODE_STRUCT, TYPE_CODE_UNION>: New cases.
    	(resolve_dynamic_struct, resolve_dynamic_union): New functions.
    
    2014-05-21  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,
    	inner_vla_struct, vla_union types.  Initialize objects of those
    	types and compute their sizes.

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 38972c6..c12fbb8 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -7385,7 +7385,11 @@ ada_which_variant_applies (struct type *var_type, struct type *outer_type,
   struct value *discrim;
   LONGEST discrim_val;
 
-  outer = value_from_contents_and_address (outer_type, outer_valaddr, 0);
+  /* Using plain value_from_contents_and_address here causes problems
+     because we will end up trying to resolve a type that is currently
+     being constructed.  */
+  outer = value_from_contents_and_address_unresolved (outer_type,
+						      outer_valaddr, 0);
   discrim = ada_value_struct_elt (outer, discrim_name, 1);
   if (discrim == NULL)
     return -1;
@@ -7925,7 +7929,13 @@ ada_template_to_fixed_record_type_1 (struct type *type,
 		 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);
+	      /* Using plain value_from_contents_and_address here
+		 causes problems because we will end up trying to
+		 resolve a type that is currently being
+		 constructed.  */
+	      dval = value_from_contents_and_address_unresolved (rtype,
+								 valaddr,
+								 address);
 	      rtype = value_type (dval);
 	    }
           else
@@ -8030,7 +8040,11 @@ ada_template_to_fixed_record_type_1 (struct type *type,
 
       if (dval0 == NULL)
 	{
-	  dval = value_from_contents_and_address (rtype, valaddr, address);
+	  /* Using plain value_from_contents_and_address here causes
+	     problems because we will end up trying to resolve a type
+	     that is currently being constructed.  */
+	  dval = value_from_contents_and_address_unresolved (rtype, valaddr,
+							     address);
 	  rtype = value_type (dval);
 	}
       else
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 94d6fe9..0035d36 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1636,6 +1636,18 @@ 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))
+	      && is_dynamic_type (TYPE_FIELD_TYPE (type, i)))
+	    return 1;
+      }
+      break;
     }
 
   return 0;
@@ -1717,6 +1729,97 @@ resolve_dynamic_array (struct type *type)
 			    range_type);
 }
 
+/* Resolve dynamic bounds of members of the union TYPE to static
+   bounds.  */
+
+static struct type *
+resolve_dynamic_union (struct type *type, CORE_ADDR addr)
+{
+  struct type *resolved_type;
+  int i;
+  unsigned int max_len = 0;
+
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION);
+
+  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;
+  return resolved_type;
+}
+
+/* Resolve dynamic bounds of members of the struct TYPE to static
+   bounds.  */
+
+static struct type *
+resolve_dynamic_struct (struct type *type, CORE_ADDR addr)
+{
+  struct type *resolved_type;
+  int i;
+  int vla_field = TYPE_NFIELDS (type) - 1;
+
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_STRUCT);
+  gdb_assert (TYPE_NFIELDS (type) > 0);
+
+  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)));
+  return resolved_type;
+}
+
 /* See gdbtypes.h  */
 
 struct type *
@@ -1753,6 +1856,14 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
       case TYPE_CODE_RANGE:
 	resolved_type = resolve_dynamic_range (type);
 	break;
+
+    case TYPE_CODE_UNION:
+      resolved_type = resolve_dynamic_union (type, addr);
+      break;
+
+    case TYPE_CODE_STRUCT:
+      resolved_type = resolve_dynamic_struct (type, addr);
+      break;
     }
 
   return resolved_type;
diff --git a/gdb/testsuite/gdb.base/vla-datatypes.c b/gdb/testsuite/gdb.base/vla-datatypes.c
index 51e342e..1ef30a5 100644
--- a/gdb/testsuite/gdb.base/vla-datatypes.c
+++ b/gdb/testsuite/gdb.base/vla-datatypes.c
@@ -46,6 +46,27 @@ vla_factory (int n)
   BAR             bar_vla[n];
   int i;
 
+  struct vla_struct
+  {
+    int something;
+    int vla_field[n];
+  } vla_struct_object;
+
+  struct inner_vla_struct
+  {
+    int something;
+    int vla_field[n];
+    int after;
+  } inner_vla_struct_object;
+
+  union vla_union
+  {
+    int vla_field[n];
+  } vla_union_object;
+
+  vla_struct_object.something = n;
+  inner_vla_struct_object.something = n;
+  inner_vla_struct_object.after = n;
   for (i = 0; i < n; i++)
     {
       int_vla[i] = i*2;
@@ -61,6 +82,9 @@ 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;
+      inner_vla_struct_object.vla_field[i] = i*2;
     }
 
   size_t int_size        = sizeof(int_vla);     /* vlas_filled */
@@ -74,6 +98,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..0e56bd7 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]
@@ -137,3 +151,7 @@ gdb_test "print int_vla\[0\]" " = 42" \
 gdb_test "whatis ++int_vla\[0\]" "type = int" "whatis ++int_vla\[0\]"
 gdb_test "print int_vla\[0\]" " = 42" \
          "print int_vla\[0\] - whatis no side effects"
+
+# This gives an error for now.
+gdb_test "print sizeof(inner_vla_struct_object)" \
+    "appears in the interior of a structure type"
diff --git a/gdb/value.c b/gdb/value.c
index d125a09..1fa72df 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3338,6 +3338,29 @@ value_from_pointer (struct type *type, CORE_ADDR addr)
 /* Create a value of type TYPE whose contents come from VALADDR, if it
    is non-null, and whose memory address (in the inferior) is
    ADDRESS.  The type of the created value may differ from the passed
+   type TYPE.  Make sure to retrieve values new type after this call.
+   Note that TYPE is not passed through resolve_dynamic_type; this is
+   a special API intended for use only by Ada.  */
+
+struct value *
+value_from_contents_and_address_unresolved (struct type *type,
+					    const gdb_byte *valaddr,
+					    CORE_ADDR address)
+{
+  struct value *v;
+
+  if (valaddr == NULL)
+    v = allocate_value_lazy (type);
+  else
+    v = value_from_contents (type, valaddr);
+  set_value_address (v, address);
+  VALUE_LVAL (v) = lval_memory;
+  return v;
+}
+
+/* Create a value of type TYPE whose contents come from VALADDR, if it
+   is non-null, and whose memory address (in the inferior) is
+   ADDRESS.  The type of the created value may differ from the passed
    type TYPE.  Make sure to retrieve values new type after this call.  */
 
 struct value *
diff --git a/gdb/value.h b/gdb/value.h
index 144e182..ce82376 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -574,6 +574,8 @@ extern struct value *value_from_history_ref (char *, char **);
 extern struct value *value_at (struct type *type, CORE_ADDR addr);
 extern struct value *value_at_lazy (struct type *type, CORE_ADDR addr);
 
+extern struct value *value_from_contents_and_address_unresolved
+     (struct type *, const gdb_byte *, CORE_ADDR);
 extern struct value *value_from_contents_and_address (struct type *,
 						      const gdb_byte *,
 						      CORE_ADDR);

Re: [PATCH 2/2] handle VLA in a struct or union

[Joel Brobecker]

Hey Tom,

> Joel> Perhaps we might think of having a new value_from_contents_and_address
> Joel> that produces the value without resolving the type? If Ada is the only
> Joel> user, as it would be, we could possibly put implement that in ada-lang,
> Joel> I think. Kind of ugly, but this would be medium-term only, since we are
> Joel> slowly trying to get rid of the GNAT encodings.
> 
> I put it in value.c, since I generally like to keep the whole module
> together, having been bitten before by code living in the wrong spot.
> I don't really care much though, in case you do.

Great minds think alike. I was offering alternatives in case people
don't want to keep stuff that is not necessarily specific to Ada
but only used by Ada in the common area. But that would usually be
nonsense to me, since that encourages proliferation of the same code
at multiple locations. But sometimes it's only a bandaid and we don't
want to encourage the use of that code, so keeping it close the few
spots where it is still necessary might make better sense.

I can't remember. Is there a dash in "overthinking"? :-)

> Here's the updated patch, which I think addresses all review comments.

I'll try to review the patch again, but you should feel free to
push it anytime you're ready. I am fairly busy at the moment :-(.

Thanks, Tom!
-- 
Joel

Re: [PATCH 2/2] handle VLA in a struct or union

[Joel Brobecker]

Hi Tom,

> b/gdb/ChangeLog:
> 2014-05-21  Tom Tromey  <tromey@redhat.com>
> 
> 	* ada-lang.c (ada_template_to_fixed_record_type_1): Use
> 	value_from_contents_and_address_unresolved.
> 	(ada_template_to_fixed_record_type_1): Likewise.
> 	(ada_which_variant_applies): Likewise.
> 	* value.h (value_from_contents_and_address_unresolved): Declare.
> 	* value.c (value_from_contents_and_address_unresolved): New
> 	function.
> 	* gdbtypes.c (is_dynamic_type, resolve_dynamic_type)
> 	<TYPE_CODE_STRUCT, TYPE_CODE_UNION>: New cases.
> 	(resolve_dynamic_struct, resolve_dynamic_union): New functions.
> 
> b/gdb/testsuite/ChangeLog:
> 2014-05-21  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,
> 	inner_vla_struct, vla_union types.  Initialize objects of those
> 	types and compute their sizes.

I decided to take the time to look at it today (or else I fear I'd never
take it ;-)). The patch looks good to me, and you should feel free to
go ahead and push.

I have one small comment (but that's not a request for change):

> +static struct type *
> +resolve_dynamic_struct (struct type *type, CORE_ADDR addr)
> +{
> +  struct type *resolved_type;
> +  int i;
> +  int vla_field = TYPE_NFIELDS (type) - 1;
> +
> +  gdb_assert (TYPE_CODE (type) == TYPE_CODE_STRUCT);
> +  gdb_assert (TYPE_NFIELDS (type) > 0);
> +
> +  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)));
> +  return resolved_type;

Ada allows dynamic fields being in the middle. For instance:

  type Really_Dyn (S1, S2 : Integer) is record
     T1 : Table (1 .. S1);
     T2 : Table (1 .. S2);
     V : Integer;
  end Really_Dyn;

We currently are having trouble describing bounds referencing a field
of the containing record like in this case, but our intent is to
eventually produce the correct debugging information both for array
bounds as well as field location.

We don't need to worry about this today. It's just a heads-up that
I will like fix this area up as soon as we have a compiler that
produces this kind of info. In the meantime, it's really great that
you're generating an error, it's going to be make this limitation
really easy to notice when it needs to be lifted.

-- 
Joel

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> We don't need to worry about this today. It's just a heads-up that
Joel> I will like fix this area up as soon as we have a compiler that
Joel> produces this kind of info. In the meantime, it's really great that
Joel> you're generating an error, it's going to be make this limitation
Joel> really easy to notice when it needs to be lifted.

FWIW there's also a bug in GCC bugzilla about the bad debuginfo
generated here for C.  If that's even fixed, I will fix up gdb.

Tom

Re: [PATCH 2/2] handle VLA in a struct or union

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I decided to take the time to look at it today (or else I fear I'd never
Joel> take it ;-)). The patch looks good to me, and you should feel free to
Joel> go ahead and push.

I'm pushing this series now.  Thanks, Joel.

Tom