Re: [Patch v2 02/10] type: add c99 variable length array support

[Tom Tromey <tromey at redhat dot com>]

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

Sanimir> -/* DWARF 2 location expression support for GDB.
Sanimir> +/* DWARF location expression support for GDB.

While I agree with this change, it's really unrelated to the patch at
hand.

Sanimir> +static int
Sanimir> +attr_to_dwarf2_prop (const struct attribute *attr, struct die_info *die,
Sanimir> +		     struct dwarf2_cu *cu, struct dwarf2_prop *prop)
Sanimir> +{
Sanimir> +  if (die == NULL || attr == NULL || cu == NULL || prop == NULL)
Sanimir> +    return 0;

Can all of these cases actually occur?
I would guess that at least 'die' and 'cu' can never be NULL.

Sanimir> +  else if (attr_form_is_ref (attr))
Sanimir> +    {
Sanimir> +      struct dwarf2_cu *target_cu = cu;
Sanimir> +      struct die_info *target_die;
Sanimir> +      struct attribute *target_attr;
Sanimir> +      const gdb_byte append_ops[] = { DW_OP_deref };

I think this won't work in all cases.  For example, suppose the
reference is to a DIE that has a DWARF expression that evaluates to a
register.  In this case, you want the value of the register -- but this
will do something else.

I appreciate what the deref is trying to do here but I think a different
approach is probably needed.

Sanimir> +static int
Sanimir> +resolve_dynamic_prop (const struct dwarf2_prop *prop, CORE_ADDR address,
Sanimir> +		      CORE_ADDR *value)
Sanimir> +{
Sanimir> +  if (prop == NULL)
Sanimir> +    return 0;
Sanimir> +
Sanimir> +  switch (prop->kind)
Sanimir> +    {
Sanimir> +    case DWARF_LOCEXPR:
Sanimir> +      {
Sanimir> +	const struct dwarf2_locexpr_baton *baton = prop->data.locexpr;
Sanimir> +
Sanimir> +	return dwarf2_locexpr_baton_eval (baton, address, value);
Sanimir> +      }
Sanimir> +    case DWARF_CONST:
Sanimir> +      break;
Sanimir> +    }

This is a switch not covering all the cases.
I think it's fine to use a simple "if" instead.
Or, if you really want a switch, put the remaining constant in there too.

Sanimir> +static int
Sanimir> +is_dynamic_type (const struct type *type)
Sanimir> +{
Sanimir> +  if (type == NULL)
Sanimir> +    return 0;

Can this happen?

Sanimir> +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
Sanimir> +      && TYPE_NFIELDS (type) == 1)
Sanimir> +    {
Sanimir> +      const struct type *range_type = TYPE_INDEX_TYPE (type);

Probably should have a check_typedef.

Sanimir> +  if (TYPE_CODE (type) == TYPE_CODE_PTR
Sanimir> +      || TYPE_CODE (type) == TYPE_CODE_REF
Sanimir> +      || TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
Sanimir> +    return is_dynamic_type (TYPE_TARGET_TYPE (type));

check_typedef here too.

Sanimir> +struct type *
Sanimir> +resolve_dynamic_type (struct type *type, CORE_ADDR address)
Sanimir> +{
[...]
Sanimir> +  /* Make a deep copy of the type.  */
Sanimir> +  copied_types = create_copied_types_hash (TYPE_OBJFILE (type));
Sanimir> +  cleanup = make_cleanup_htab_delete (copied_types);
Sanimir> +  resolved_type = copy_type_recursive
Sanimir> +    (TYPE_OBJFILE (type), type, copied_types);

A couple problems here actually.

copy_type_recursive is actually not very general purpose.

It allocates temporary data on the objfile obstack, because it "knows"
it is only called when the objfile is being destroyed and so some extra
allocations there are unimportant.  (This is of course fixable.)

It doesn't copy everything -- it doesn't preserve some C++ data.

Also it deep copies the things it does copy, so including things like
field names.

On the whole this seems pretty expensive.  Don't you just need to copy a
top-level array type?  If so maybe copy_type would work better.


Also I noticed that the intro comments for resolve_dynamic_prop,
resolve_dynamic_bounds, and resolve_dynamic_type do not document the
meaning of the ADDRESS parameter.

Sanimir> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
Sanimir> index d01f75e..5500e94 100644
Sanimir> --- a/gdb/gdbtypes.h
Sanimir> +++ b/gdb/gdbtypes.h
Sanimir> @@ -1094,6 +1094,7 @@ extern void allocate_gnat_aux_type (struct type *);
Sanimir>  #define TYPE_LOW_BOUND_KIND(range_type) \
Sanimir>    TYPE_RANGE_DATA(range_type)->low.kind
 
Sanimir> +
Sanimir>  /* Moto-specific stuff for FORTRAN arrays.  */

Random change.

Sanimir> +  store_typed_address (value_contents_raw (val), check_typedef (resolved_type), addr);

Line too long.

Tom