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

[Tom Tromey <tromey@redhat.com>]

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