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

[Joel Brobecker <brobecker at adacore dot com>]

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