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

  Thanks for your review.

Seems like some code and comments were not adjusted after the migrating
from deep copying type to only array/range/typdef copies. I removed not
needed code and adjusted the comments to reflect the actual code.
Thanks a lot for pointing out.

I addressed all issues (see below) except for these two:

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

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

  1| int foo(int vla[n][m])

  (gdb) ptype vla
  type = int (*)[] 
  (gdb) print vla[0]
  Cannot perform pointer math on incomplete types, try casting to a known type, or void *.

So we included TYPE_CODE_PTR to support such use case. 

> > +
> > +static struct type *
> > +resolve_dynamic_bounds (struct type *type, CORE_ADDR addr)
> > +{
> > +  CORE_ADDR value;
> > +  struct type *array_type;
> > +  struct type *range_type;
> > +  struct type *ary_dim;
> > +  const struct dynamic_prop *prop;
> > +  const struct dwarf2_locexpr_baton *baton;
> > +  struct dynamic_prop low_bound, high_bound;
> > +
> > +  if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF
> > +      || TYPE_CODE (type) == TYPE_CODE_PTR)
> > +    {
> > +      struct type *copy = copy_type (type);
> > +
> > +      TYPE_TARGET_TYPE (copy)
> > +	= resolve_dynamic_bounds (TYPE_TARGET_TYPE (type), addr);
> > +
> > +      return copy;
> > +    }
> 
> Same remark as above for the handling of typedefs. I think this will
> help avoid an unnecessary type copy.
> 
> Also, same question about handling pointers. Why do it here?
> 
We need to copy typedef to preserve the correct typename otherwise we
end up with the real type. The reasoning behind TYPE_CODE_PTR is pretty
much the same as for the above case.

 -Sanimir

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Friday, February 28, 2014 05:27 PM
> To: Agovic, Sanimir
> Cc: tromey@redhat.com; Boell, Keven; gdb-patches@sourceware.org
> Subject: Re: [PATCH v5 03/15] type: add c99 variable length array support
> 
> Hi Sanimir,
> 
> > 2013-10-18  Sanimir Agovic  <sanimir.agovic@intel.com>
> >             Keven Boell  <keven.boell@intel.com>
> >
> > 	* dwarf2loc.c (dwarf2_locexpr_baton_eval): New function.
> > 	(dwarf2_evaluate_property): New function.
> > 	* dwarf2loc.h (dwarf2_evaluate_property): New function prototype.
> > 	* dwarf2read.c (attr_to_dynamic_prop): New function.
> > 	(read_subrange_type): Use attr_to_dynamic_prop to read high bound
> > 	attribute.
> > 	* gdbtypes.c: Include dwarf2loc.h.
> > 	(is_dynamic_type): New function.
> > 	(resolve_dynamic_type): New function.
> > 	(resolve_dynamic_bounds): New function.
> > 	(get_type_length): New function.
> > 	(check_typedef): Use get_type_length to compute type length.
> > 	* gdbtypes.h (TYPE_HIGH_BOUND_KIND): New macro.
> > 	(TYPE_LOW_BOUND_KIND): New macro.
> > 	(is_dynamic_type): New function prototype.
> > 	* value.c (value_from_contents_and_address): Call resolve_dynamic_type
> > 	to resolve dynamic properties of the type. Update comment.
> > 	* valops.c (get_value_at, value_at, value_at_lazy): Update comment.
> 
> Tough review (took a while). Some questions and comments, but nothing
> too serious. See below.
> 
> I apologize in advance if I appear to nit-pick - I only allow myself to
> do so when touching the patch for other reasons too.  Thank you!
> 
> > Signed-off-by: Sanimir Agovic <sanimir.agovic@intel.com>
> > ---
> >  gdb/dwarf2loc.c  | 120 +++++++++++++++++++++++++
> >  gdb/dwarf2loc.h  |  24 +++++
> >  gdb/dwarf2read.c | 100 +++++++++++++++------
> >  gdb/gdbtypes.c   | 264 ++++++++++++++++++++++++++++++++++++++++++++-----------
> >  gdb/gdbtypes.h   |  10 +++
> >  gdb/valops.c     |  12 ++-
> >  gdb/value.c      |  17 ++--
> >  7 files changed, 459 insertions(+), 88 deletions(-)
> >
> > diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> > index 2b1f323..72bc727 100644
> > --- a/gdb/dwarf2loc.c
> > +++ b/gdb/dwarf2loc.c
> > @@ -2431,6 +2431,126 @@ dwarf2_evaluate_loc_desc (struct type *type, struct frame_info
> *frame,
> >    return dwarf2_evaluate_loc_desc_full (type, frame, data, size, per_cu, 0);
> >  }
> >
> > +/* Evaluates a dwarf expression and stores the result in VAL, expecting
> > +   that the dwarf expression only produces a single CORE_ADDR.  ADDR is a
> > +   context (location of a variable) and might be needed to evaluate the
> > +   location expression.
> > +   Returns 1 on success, 0 otherwise.   */
> > +
> > +static int
> > +dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
> > +			   CORE_ADDR addr, CORE_ADDR *valp)
> > +{
> > +  struct dwarf_expr_context *ctx;
> > +  struct dwarf_expr_baton baton;
> > +  struct objfile *objfile;
> > +  struct cleanup *cleanup;
> > +
> > +  if (dlbaton == NULL || dlbaton->size == 0)
> > +    return 0;
> > +
> > +  ctx = new_dwarf_expr_context ();
> > +  cleanup = make_cleanup_free_dwarf_expr_context (ctx);
> > +
> > +  baton.frame = get_selected_frame (NULL);
> > +  baton.per_cu = dlbaton->per_cu;
> > +
> > +  objfile = dwarf2_per_cu_objfile (dlbaton->per_cu);
> > +
> > +  ctx->gdbarch = get_objfile_arch (objfile);
> > +  ctx->addr_size = dwarf2_per_cu_addr_size (dlbaton->per_cu);
> > +  ctx->ref_addr_size = dwarf2_per_cu_ref_addr_size (dlbaton->per_cu);
> > +  ctx->offset = dwarf2_per_cu_text_offset (dlbaton->per_cu);
> > +  ctx->funcs = &dwarf_expr_ctx_funcs;
> > +  ctx->baton = &baton;
> > +
> > +  dwarf_expr_eval (ctx, dlbaton->data, dlbaton->size);
> > +
> > +  switch (ctx->location)
> > +    {
> > +    case DWARF_VALUE_REGISTER:
> > +    case DWARF_VALUE_MEMORY:
> > +    case DWARF_VALUE_STACK:
> > +      *valp = dwarf_expr_fetch_address (ctx, 0);
> > +      if (ctx->location == DWARF_VALUE_REGISTER)
> > +	*valp = dwarf_expr_read_addr_from_reg (&baton, *valp);
> > +      do_cleanups (cleanup);
> > +      return 1;
> > +    case DWARF_VALUE_LITERAL:
> > +      *valp = extract_signed_integer (ctx->data, ctx->len,
> > +				      gdbarch_byte_order (ctx->gdbarch));
> > +      do_cleanups (cleanup);
> > +      return 1;
> > +      /* Not supported dwarf values.  */
> > +    case DWARF_VALUE_OPTIMIZED_OUT:
> > +    case DWARF_VALUE_IMPLICIT_POINTER:
> 
> Would "Unsupported" be more idiomatic? I'm not a native English
> speaker, so I am not sure. But "unsupported" somehow sounds better.
> Feel free to choose either option.
> 
Chose "unsupported".

> > +      break;
> > +    }
> > +
> > +  do_cleanups (cleanup);
> > +
> > +  return 0;
> 
> Can you remove the empty line betwee do_cleanups and return?
> It's not very important, but compresses a bit the code, I think.
> 
Done.

> > +}
> > +
> > +/* See dwarf2loc.h.  */
> > +
> > +int
> > +dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR address,
> > +			  CORE_ADDR *value)
> > +{
> > +  if (prop == NULL)
> > +    return 0;
> > +
> > +  switch (prop->kind)
> > +    {
> > +    case PROP_LOCEXPR:
> > +      {
> > +	const struct dwarf2_property_baton *baton = prop->data.baton;
> > +
> > +	if (dwarf2_locexpr_baton_eval (&baton->locexpr, address, value))
> > +	  {
> > +	    if (baton->referenced_type)
> > +	      {
> > +		struct value *val = value_at (baton->referenced_type, *value);
> > +
> > +		*value = value_as_address (val);
> > +	      }
> > +	    return 1;
> > +	  }
> > +      }
> > +      break;
> > +
> > +    case PROP_LOCLIST:
> > +      {
> > +	struct dwarf2_property_baton *baton = prop->data.baton;
> > +	struct frame_info *frame = get_selected_frame (NULL);
> > +	CORE_ADDR pc = get_frame_address_in_block (frame);
> > +	const gdb_byte *data;
> > +	struct value *val;
> > +	size_t size;
> > +
> > +	data = dwarf2_find_location_expression (&baton->loclist, &size, pc);
> > +	if (data != NULL)
> > +	  {
> > +	    val = dwarf2_evaluate_loc_desc (baton->referenced_type, frame, data,
> > +					    size, baton->loclist.per_cu);
> > +	    if (!value_optimized_out (val))
> > +	      {
> > +		*value = value_as_address (val);
> > +		return 1;
> > +	      }
> > +	  }
> > +      }
> > +      break;
> > +
> > +    case PROP_CONST:
> > +      *value = prop->data.const_val;
> > +      return 1;
> > +    }
> > +
> > +  return 0;
> > +}
> > +
> >  

> >  /* Helper functions and baton for dwarf2_loc_desc_needs_frame.  */
> >
> > diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
> > index 9bc8ca5..9be3e24 100644
> > --- a/gdb/dwarf2loc.h
> > +++ b/gdb/dwarf2loc.h
> > @@ -90,6 +90,13 @@ struct value *dwarf2_evaluate_loc_desc (struct type *type,
> >  					size_t size,
> >  					struct dwarf2_per_cu_data *per_cu);
> >
> > +/* Converts a dynamic property into a static one.  ADDR is the address of
> > +   the object currently being evaluated and might be nedded.
> > +   Returns 1 if PROP could be converted and the static value is passed back
> > +   into VALUE, otherwise returns 0.  */
> > +int dwarf2_evaluate_property (const struct dynamic_prop *prop,
> > +			      CORE_ADDR addr, CORE_ADDR *value);
> > +
> 
> Would you mind adding an empty line betwee documentation and
> declaration? It's a bit of a nit-pick, but since we're adjusting
> this patch, we might as well.
> 
Done.

> >  CORE_ADDR dwarf2_read_addr_index (struct dwarf2_per_cu_data *per_cu,
> >  				  unsigned int addr_index);
> >
> > @@ -135,6 +142,23 @@ struct dwarf2_loclist_baton
> >    unsigned char from_dwo;
> >  };
> >
> > +/* A dynamic property is either expressed as a single location expression
> > +   or a location list in the context of TYPE.  */
> > +
> > +struct dwarf2_property_baton
> > +{
> > +  /* Keep track of referenced type.  */
> > +  struct type *referenced_type;
> 
> Can you explain in this comment what the referenced type is?
> If it helps, giving an example could be used.
> 
Done, see my inlined my changes:
-- >8 --

> > +  union
> > +  {
> > +    /* Location expression.  */
> > +    struct dwarf2_locexpr_baton locexpr;
> > +
> > +    /* Location list to be evaluated in the context of TYPE.  */
> > +    struct dwarf2_loclist_baton loclist;
> > +  };
> > +};
> > +
> >  extern const struct symbol_computed_ops dwarf2_locexpr_funcs;
> >  extern const struct symbol_computed_ops dwarf2_loclist_funcs;
> >
> > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> > index 101065b..1d5fa3d 100644
> > --- a/gdb/dwarf2read.c
> > +++ b/gdb/dwarf2read.c
> > @@ -14252,6 +14252,78 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
> >    return set_die_type (die, type, cu);
> >  }
> >
> > +/* Parse dwarf attribute if it's a block, reference or constant and put the
> > +   resulting value of the attribute into struct bound_prop.
> > +   Returns 1 if ATTR could be resolved into PROP, 0 otherwise.  */
> > +
> > +static int
> > +attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
> > +		      struct dwarf2_cu *cu, struct dynamic_prop *prop)
> > +{
> > +  struct dwarf2_property_baton *baton;
> > +  struct obstack *obstack = &cu->objfile->objfile_obstack;
> > +
> > +  if (attr == NULL || prop == NULL)
> > +    return 0;
> > +
> > +  if (attr_form_is_block (attr))
> > +    {
> > +      baton = obstack_alloc (obstack, sizeof (*baton));
> > +      baton->referenced_type = NULL;
> > +      baton->locexpr.per_cu = cu->per_cu;
> > +      baton->locexpr.size = DW_BLOCK (attr)->size;
> > +      baton->locexpr.data = DW_BLOCK (attr)->data;
> > +      prop->data.baton = baton;
> > +      prop->kind = PROP_LOCEXPR;
> > +      gdb_assert (prop->data.baton != NULL);
> > +    }
> > +  else if (attr_form_is_ref (attr))
> > +    {
> > +      struct dwarf2_cu *target_cu = cu;
> > +      struct die_info *target_die;
> > +      struct attribute *target_attr;
> > +
> > +      target_die = follow_die_ref (die, attr, &target_cu);
> > +      target_attr = dwarf2_attr (target_die, DW_AT_location, target_cu);
> > +      if (target_attr == NULL)
> > +	return 0;
> > +
> > +      if (attr_form_is_section_offset (target_attr))
> > +	{
> > +	  baton = obstack_alloc (obstack, sizeof (*baton));
> > +	  baton->referenced_type = die_type (target_die, target_cu);
> > +	  fill_in_loclist_baton (cu, &baton->loclist, target_attr);
> > +	  prop->data.baton = baton;
> > +	  prop->kind = PROP_LOCLIST;
> > +	  gdb_assert (prop->data.baton != NULL);
> > +	}
> > +      else if (attr_form_is_block (target_attr))
> > +	{
> > +	  baton = obstack_alloc (obstack, sizeof (*baton));
> > +	  baton->referenced_type = die_type (target_die, target_cu);
> > +	  baton->locexpr.per_cu = cu->per_cu;
> > +	  baton->locexpr.size = DW_BLOCK (target_attr)->size;
> > +	  baton->locexpr.data = DW_BLOCK (target_attr)->data;
> > +	  prop->data.baton = baton;
> > +	  prop->kind = PROP_LOCEXPR;
> > +	  gdb_assert (prop->data.baton != NULL);
> > +	}
> 
> It looks like, if TARGET_ATTR is neither a section offset
> nor a block, you're getting through to the "return 1" at the end
> of the function even though you've haven't set PROP. From my
> reading of the DWARF v4 standard, DW_AT_location attributes
> should not have any other forms, but let's protect ourselves
> against it. 
>
> I would add a complaint and then return 0.
> 
Done.

> > +    }
> > +  else if (attr_form_is_constant (attr))
> > +    {
> > +      prop->data.const_val = dwarf2_get_attr_constant_value (attr, 0);
> > +      prop->kind = PROP_CONST;
> > +    }
> > +  else
> > +    {
> > +      dwarf2_invalid_attrib_class_complaint (dwarf_form_name (attr->form),
> > +					     dwarf2_name (die, cu));
> > +      return 0;
> > +    }
> > +
> > +  return 1;
> > +}
> > +
> >  /* Read the given DW_AT_subrange DIE.  */
> >
> > @@ -14409,12 +14461,6 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
> >
> >    range_type = create_range_type (NULL, orig_base_type, &low, &high);
> >
> > -  /* Mark arrays with dynamic length at least as an array of unspecified
> > -     length.  GDB could check the boundary but before it gets implemented at
> > -     least allow accessing the array elements.  */
> > -  if (attr && attr_form_is_block (attr))
> > -    TYPE_HIGH_BOUND_KIND (range_type) = PROP_UNDEFINED;
> > -
> >    /* Ada expects an empty array on no boundary attributes.  */
> >    if (attr == NULL && cu->language != language_ada)
> >      TYPE_HIGH_BOUND_KIND (range_type) = PROP_UNDEFINED;
> > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> > index 7f0269c..f5bd236 100644
> > --- a/gdb/gdbtypes.c
> > +++ b/gdb/gdbtypes.c
> > @@ -853,6 +853,17 @@ create_static_range_type (struct type *result_type, struct type
> *index_type,
> >    return result_type;
> >  }
> >
> > +/* Predicate tests whether BOUNDS are static.  Returns 1 if all bounds values
> > +   are static, otherwise returns 0.  */
> > +
> > +static int
> > +has_static_range (const struct range_bounds *bounds)
> > +{
> > +  return (bounds->low.kind == PROP_CONST
> > +	  && bounds->high.kind == PROP_CONST);
> > +}
> > +
> > +
> >  /* Set *LOWP and *HIGHP to the lower and upper bounds of discrete type
> >     TYPE.  Return 1 if type is a range type, 0 if it is discrete (and
> >     bounds will fit in LONGEST), or -1 otherwise.  */
> > @@ -982,24 +993,28 @@ create_array_type (struct type *result_type,
> >  		   struct type *element_type,
> >  		   struct type *range_type)
> >  {
> > -  LONGEST low_bound, high_bound;
> > -
> >    if (result_type == NULL)
> >      result_type = alloc_type_copy (range_type);
> >
> >    TYPE_CODE (result_type) = TYPE_CODE_ARRAY;
> >    TYPE_TARGET_TYPE (result_type) = element_type;
> > -  if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
> > -    low_bound = high_bound = 0;
> > -  CHECK_TYPEDEF (element_type);
> > -  /* Be careful when setting the array length.  Ada arrays can be
> > -     empty arrays with the high_bound being smaller than the low_bound.
> > -     In such cases, the array length should be zero.  */
> > -  if (high_bound < low_bound)
> > -    TYPE_LENGTH (result_type) = 0;
> > -  else
> > -    TYPE_LENGTH (result_type) =
> > -      TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
> > +
> > +  if (has_static_range (TYPE_RANGE_DATA (range_type)))
> > +    {
> > +      LONGEST low_bound, high_bound;
> > +
> > +      if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
> > +	low_bound = high_bound = 0;
> > +      CHECK_TYPEDEF (element_type);
> > +      /* Be careful when setting the array length.  Ada arrays can be
> > +	 empty arrays with the high_bound being smaller than the low_bound.
> > +	 In such cases, the array length should be zero.  */
> > +      if (high_bound < low_bound)
> > +	TYPE_LENGTH (result_type) = 0;
> > +      else
> > +	TYPE_LENGTH (result_type) =
> > +	  TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
> > +    }
> >    TYPE_NFIELDS (result_type) = 1;
> >    TYPE_FIELDS (result_type) =
> >      (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
> 
> Does it look like you might be leaving the TYPE_LENGTH undefined
> for the case of the non-static array index? (Eg when result_type
> is not passed as NULL).
> 
> It may not matter in practice, as we are probably supposed to ignore
> that field for those arrays anyway. But in the case we're not, leaving
> the length undefined could cause all sorts of nastiness. For instance,
> a struct value fetching its contents could allocate a buffer using
> that size, which potentially means randomly large memory regions,
> leading to a memory error, always very scary for the typical user.
> 
> Any objection to setting the length to 0 in this case?
> 
Set length to 0 as proposed.

> > @@ -1530,7 +1545,181 @@ stub_noname_complaint (void)
> >    complaint (&symfile_complaints, _("stub type has NULL name"));
> >  }
> >
> > -/* Find the real type of TYPE.  This function returns the real type,
> > +/* Calculates the size of a type.  If TYPE has static bound values takes upper
> > +   and lower bound into account, otherwise only the TYPE length is returned.
> > +   TYPE is expected not to be a typedef.  */
> 
> Would you mind explaining this part to me?
> 
> Intuitively, if it is an array with static bounds, then haven't we
> already set the correct array length during the call to "create_array_type"?
> And if the type is indeed dynamic, is the TYPE's TYPE_LENGTH correct?
> 
We moved from deep copying types to only copy the range/array types and forgot to
remove the get_type_length function which was needed in the former case. Removed
the code.

> > +
> > +static ULONGEST
> > +get_type_length (const struct type *type)
> > +{
> > +  const struct type *range_type, *target_type;
> > +  ULONGEST len = TYPE_LENGTH (type);
> > +  LONGEST low_bound, high_bound;
> > +
> > +  gdb_assert (TYPE_CODE (type) != TYPE_CODE_TYPEDEF);
> > +
> > +  if (TYPE_CODE (type) != TYPE_CODE_ARRAY
> > +      && TYPE_CODE (type) != TYPE_CODE_STRING)
> > +    return len;
> > +
> > +  range_type = check_typedef (TYPE_INDEX_TYPE (type));
> > +
> > +  if (!has_static_range (TYPE_RANGE_DATA (range_type)))
> > +    return len;
> > +
> > +  target_type = check_typedef (TYPE_TARGET_TYPE (type));
> > +
> > +  /* Now recompute the length of the array type, based on its
> > +     number of elements and the target type's length.
> > +     Watch out for Ada null Ada arrays where the high bound
> > +     is smaller than the low bound.  */
> > +  low_bound = TYPE_LOW_BOUND (range_type);
> > +  high_bound = TYPE_HIGH_BOUND (range_type);
> > +
> > +  if (high_bound < low_bound)
> > +    len = 0;
> > +  else
> > +    {
> > +      /* For now, we conservatively take the array length to be 0
> > +         if its length exceeds UINT_MAX.  The code below assumes
> > +         that for x < 0, (ULONGEST) x == -x + ULONGEST_MAX + 1,
> > +         which is technically not guaranteed by C, but is usually true
> > +         (because it would be true if x were unsigned with its
> > +         high-order bit on).  It uses the fact that
> > +         high_bound-low_bound is always representable in
> > +         ULONGEST and that if high_bound-low_bound+1 overflows,
> > +         it overflows to 0.  We must change these tests if we
> > +         decide to increase the representation of TYPE_LENGTH
> > +         from unsigned int to ULONGEST.  */
> > +      ULONGEST ulow = low_bound, uhigh = high_bound;
> > +      ULONGEST tlen = get_type_length (target_type);
> > +
> > +      len = tlen * (uhigh - ulow + 1);
> > +      if (tlen == 0 || (len / tlen - 1 + ulow) != uhigh || len > UINT_MAX)
> > +        len = 0;
> > +    }
> > +
> > +  return len;
> > +}
> > +
> > +/* See gdbtypes.h.  */
> > +
> > +int
> > +is_dynamic_type (const struct type *type)
> > +{
> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > +      && TYPE_NFIELDS (type) == 1)
> > +    {
> > +      const struct type *range_type = TYPE_INDEX_TYPE (type);
> > +
> > +      if (!has_static_range (TYPE_RANGE_DATA (range_type)))
> > +	return 1;
> > +    }
> > +
> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > +      || TYPE_CODE (type) == TYPE_CODE_PTR
> > +      || TYPE_CODE (type) == TYPE_CODE_REF
> > +      || TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
> > +    return is_dynamic_type (check_typedef (TYPE_TARGET_TYPE (type)));
> 
> As discussed on IRC, I think that TYPE_NFIELDS should always
> be 1 for TYPE_CODE_ARRAY. So let's transform that into an assert,
> if you don't mind, and see what happens?
> 
Added assert on nfields == 1.

> > +
> > +  return 0;
> > +}
> > +
> > +/* Resolves dynamic bound values of an array type to static ones.
> > +   TYPE is modified in place and is expected not to be a typedef.
> > +   ADDRESS might be needed to resolve the subrange bounds, it is the location
> > +   of the associated array.  */
> 
> In this description, you mention that TYPE is expected NOT to be
> a typedef, and yet the code tests for it...
> 
Modified the comment to reflect the actual code.

> 
> > +
> > +  gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY);
> > +
> > +  array_type = check_typedef (type);
> > +  range_type = check_typedef (TYPE_INDEX_TYPE (array_type));
> 
> If the assert above passed, you should not need the check_typedef
> on TYPE to get ARRAY_TYPE.
> 
Removed.

> 
> > +
> > +  prop = &TYPE_RANGE_DATA (range_type)->low;
> > +  if (dwarf2_evaluate_property (prop, addr, &value))
> > +    {
> > +      low_bound.kind = PROP_CONST;
> > +      low_bound.data.const_val = value;
> > +    }
> > +  else
> > +    {
> > +      low_bound.kind = PROP_UNDEFINED;
> > +      low_bound.data.const_val = 0;
> > +    }
> > +
> > +  prop = &TYPE_RANGE_DATA (range_type)->high;
> > +  if (dwarf2_evaluate_property (prop, addr, &value))
> > +    {
> > +      high_bound.kind = PROP_CONST;
> > +      high_bound.data.const_val = value;
> > +    }
> > +  else
> > +    {
> > +      high_bound.kind = PROP_UNDEFINED;
> > +      high_bound.data.const_val = 0;
> > +    }
> > +
> > +  ary_dim = check_typedef (TYPE_TARGET_TYPE (array_type));
> > +
> > +  if (ary_dim != NULL && TYPE_CODE (ary_dim) == TYPE_CODE_ARRAY)
> > +    array_type = resolve_dynamic_bounds (TYPE_TARGET_TYPE (type), addr);
> > +  else
> > +    array_type = TYPE_TARGET_TYPE (type);
> 
> I know this is going to sound a little silly, but would you mind
> calling "array_type" something like "elt_type" instead. The else
> block where you have "array_type = TYPE_TARGET_TYPE" really threw me
> at first, thinking you had a bug in your code.
> 
Done.

> > +
> > +  range_type
> > +    = create_range_type (NULL,
> > +			 TYPE_TARGET_TYPE (range_type),
> > +			 &low_bound, &high_bound);
> 
> Small formatting nit-pick (while we're touching this code):
> 
Done.

>   range_type = create_range_type (NULL,
>                                   TYPE_TARGET_TYPE (range_type),
>                                   &low_bound, &high_bound);
> 
> > +  array_type = create_array_type (copy_type (type),
> > +				  array_type,
> > +				  range_type);
> > +
> > +  return array_type;
> > +}
> > +
> > +/* See gdbtypes.h  */
> > +
> > +struct type *
> > +resolve_dynamic_type (struct type *type, CORE_ADDR addr)
> > +{
> > +  struct type *real_type = check_typedef (type);
> > +  struct type *resolved_type;
> > +
> > +  if (!TYPE_OBJFILE_OWNED (real_type))
> > +    return type;
> 
> Can you explain this? This definitely deserves a comment.
> 
This is a left over from the deep-type-copy patch series. Removed.

> > +  if (!is_dynamic_type (real_type))
> > +    return type;
> > +
> > +  resolved_type = resolve_dynamic_bounds (type, addr);
> > +  resolved_type->length = get_type_length (check_typedef (resolved_type));
> 
> Same as above regarding get_type_length. It seems to me that the length
> should have already been correctly computed by create_array_type,
> called from resolve_dynamic_bounds.
> 
Indeed. Removed old deep-type-copy code.
> > +
> > +  return resolved_type;
> > +}
> > +
> > +/* find the real type of TYPE.  This function returns the real type,
>       ^^^^
> 
> I think you unintentionally modified the first line of this comment...
> This line should not be part of the diff.
> 
Done.

> 
> 
> >     after removing all layers of typedefs, and completing opaque or stub
> >     types.  Completion changes the TYPE argument, but stripping of
> >     typedefs does not.
> > @@ -1706,44 +1895,15 @@ check_typedef (struct type *type)
> >  	  /* Nothing we can do.  */
> >  	}
> >        else if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > -	       && TYPE_NFIELDS (type) == 1
> > -	       && (TYPE_CODE (range_type = TYPE_INDEX_TYPE (type))
> > -		   == TYPE_CODE_RANGE))
> > -	{
> > -	  /* Now recompute the length of the array type, based on its
> > -	     number of elements and the target type's length.
> > -	     Watch out for Ada null Ada arrays where the high bound
> > -	     is smaller than the low bound.  */
> > -	  const LONGEST low_bound = TYPE_LOW_BOUND (range_type);
> > -	  const LONGEST high_bound = TYPE_HIGH_BOUND (range_type);
> > -	  ULONGEST len;
> > -
> > -	  if (high_bound < low_bound)
> > -	    len = 0;
> > -	  else
> > -	    {
> > -	      /* For now, we conservatively take the array length to be 0
> > -		 if its length exceeds UINT_MAX.  The code below assumes
> > -		 that for x < 0, (ULONGEST) x == -x + ULONGEST_MAX + 1,
> > -		 which is technically not guaranteed by C, but is usually true
> > -		 (because it would be true if x were unsigned with its
> > -		 high-order bit on).  It uses the fact that
> > -		 high_bound-low_bound is always representable in
> > -		 ULONGEST and that if high_bound-low_bound+1 overflows,
> > -		 it overflows to 0.  We must change these tests if we
> > -		 decide to increase the representation of TYPE_LENGTH
> > -		 from unsigned int to ULONGEST.  */
> > -	      ULONGEST ulow = low_bound, uhigh = high_bound;
> > -	      ULONGEST tlen = TYPE_LENGTH (target_type);
> > -
> > -	      len = tlen * (uhigh - ulow + 1);
> > -	      if (tlen == 0 || (len / tlen - 1 + ulow) != uhigh
> > -		  || len > UINT_MAX)
> > -		len = 0;
> > -	    }
> > -	  TYPE_LENGTH (type) = len;
> > -	  TYPE_TARGET_STUB (type) = 0;
> > -	}
> > +	       || TYPE_CODE (type) == TYPE_CODE_STRING)
> > +        {
> > +          range_type = TYPE_INDEX_TYPE (type);
> > +          if (has_static_range (TYPE_RANGE_DATA (range_type)))
> > +            {
> > +              TYPE_LENGTH (type) = get_type_length (type);
> > +              TYPE_TARGET_STUB (type) = 0;
> > +            }
> > +        }
> >        else if (TYPE_CODE (type) == TYPE_CODE_RANGE)
> >  	{
> >  	  TYPE_LENGTH (type) = TYPE_LENGTH (target_type);
> > diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> > index f6e68c5..38cd240 100644
> > --- a/gdb/gdbtypes.h
> > +++ b/gdb/gdbtypes.h
> > @@ -1,3 +1,4 @@
> > +
> >  /* Internal type definitions for GDB.
> >
> >     Copyright (C) 1992-2013 Free Software Foundation, Inc.
> > @@ -1574,6 +1575,15 @@ extern struct type *lookup_unsigned_typename (const struct
> language_defn *,
> >  extern struct type *lookup_signed_typename (const struct language_defn *,
> >  					    struct gdbarch *, const char *);
> >
> > +/* Resolves all dynamic values of a type e.g. array bounds to static values.
> > +   ADDR specifies the location of the variable the type is bound to.
> > +   If TYPE has no dynamic values returns TYPE otherwise a new type with static
> > +   values is returned.  */
> > +extern struct type *resolve_dynamic_type (struct type *type, CORE_ADDR addr);
> > +
> > +/* Predicates if the type has dynamic values, which are not resolved yet.  */
> > +extern int is_dynamic_type (const struct type *type);
> > +
> 
> While making adjustments to this patch, would you mind adjusting
> the style of the function descriptions, please. We use the imperative
> style when describing the actions of the function. For instance:
> 
Done.

> /* Resolve (no s) all dynamic values of a type.
> 
> "ADDR specifies" should still be with an 's'...
> 
> 
> >  extern struct type *check_typedef (struct type *);
> >
> >  #define CHECK_TYPEDEF(TYPE)			\
> > diff --git a/gdb/valops.c b/gdb/valops.c
> > index 5c7bb89..a7260b9 100644
> > --- a/gdb/valops.c
> > +++ b/gdb/valops.c
> > @@ -902,7 +902,9 @@ value_one (struct type *type)
> >    return val;
> >  }
> >
> > -/* Helper function for value_at, value_at_lazy, and value_at_lazy_stack.  */
> > +/* Helper function for value_at, value_at_lazy, and value_at_lazy_stack.
> > +   The type of the created value may differ from the passed type TYPE.
> > +   Make sure to retrieve values new type after this call.  */
>                            ^^^^^^^^
> 
> Suggest: Make sure to retrieve the returned value's new type after
> this call.
> 
> I'd suggest also an example of when the value's type might be
> different...
> 
> [...]
> 
> Same remark as above. And in value.c below as well.
> 
Done.

> >
> >  struct value *
> >  value_at_lazy (struct type *type, CORE_ADDR addr)
> > diff --git a/gdb/value.c b/gdb/value.c
> > index a64e7e1..12726a1 100644
> > --- a/gdb/value.c
> > +++ b/gdb/value.c
> > @@ -3178,32 +3178,37 @@ value_from_ulongest (struct type *type, ULONGEST num)
> >
> >
> >  /* Create a value representing a pointer of type TYPE to the address
> > -   ADDR.  */
> > +   ADDR.  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 *
> >  value_from_pointer (struct type *type, CORE_ADDR addr)
> >  {
> > -  struct value *val = allocate_value (type);
> > +  struct type *resolved_type = resolve_dynamic_type (type, addr);
> > +  struct value *val = allocate_value (resolved_type);
> >
> > -  store_typed_address (value_contents_raw (val), check_typedef (type), addr);
> > +  store_typed_address (value_contents_raw (val),
> > +		       check_typedef (resolved_type), addr);
> >    return val;
> >  }
> >
> >
> >  /* 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.  */
> > +   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 *
> >  value_from_contents_and_address (struct type *type,
> >  				 const gdb_byte *valaddr,
> >  				 CORE_ADDR address)
> >  {
> > +  struct type *resolved_type = resolve_dynamic_type (type, address);
> >    struct value *v;
> >
> >    if (valaddr == NULL)
> > -    v = allocate_value_lazy (type);
> > +    v = allocate_value_lazy (resolved_type);
> >    else
> > -    v = value_from_contents (type, valaddr);
> > +    v = value_from_contents (resolved_type, valaddr);
> >    set_value_address (v, address);
> >    VALUE_LVAL (v) = lval_memory;
> >    return v;
> > --
> > 1.8.4.2
> 
> --
> Joel
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

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

Commit Message

Comments

Patch