Re: [PATCH 02/23] dwarf: add DW_AT_data_location support

[Joel Brobecker <brobecker@adacore.com>]

Hello,

> An object might have a descriptor proceeding the actual value.
> To point the debugger to the actually value of an object
> DW_AT_data_location is used for. For example the compile may
> emit for this entity:
> 
>   1| int foo[N];
> 
> the following descriptor:
> 
> struct array {
>   size_t size;
>   void*  data; // DW_AT_data_location describes this location
> }
> 
> This allows GDB to print the actual data of an type.
> 
> 2014-05-28  Sanimir Agovic  <sanimir.agovic@intel.com>
>             Keven Boell  <keven.boell@intel.com>
> 
> 	* dwarf2read.c (set_die_type): Parse and save DW_AT_data_location
> 	attribute.
> 	* gdbtypes.c (is_dynamic_type): Consider a type being dynamic if
> 	the data location has not yet been resolved.
> 	(resolve_dynamic_type): Evaluate data location baton
> 	if present and save its value.
> 	* gdbtypes.h <main_type>: Add data_location.
> 	(TYPE_DATA_LOCATION): New macro.
> 	(TYPE_DATA_LOCATION_ADDR): New macro.
> 	(TYPE_DATA_LOCATION_IS_ADDRESS): New macro.
> 	* value.c: Include dwarf2loc.h.
> 	(value_fetch_lazy): Use data location addres to read value from
> 	memory.
> 	(coerce_ref): Construct new value from data location.

Here are some comments and questions, but it would be nice,  IMO, if
the patch was also reviewed by Tom, particularly since it introduces
a new field in struct main_type, which is a size-sensitive, I think.

Also, it would be nice if you could include a copy of the actual
DWARF output in the revision log of your patch, for easy reference
of what you're trying to support.

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 6ebfffc..7a0f7f4 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -21499,6 +21499,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
>  {
>    struct dwarf2_per_cu_offset_and_type **slot, ofs;
>    struct objfile *objfile = cu->objfile;
> +  struct attribute *attr;
>  
>    /* For Ada types, make sure that the gnat-specific data is always
>       initialized (if not already set).  There are a few types where
> @@ -21513,6 +21514,20 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
>        && !HAVE_GNAT_AUX_INFO (type))
>      INIT_GNAT_SPECIFIC (type);
>  
> +  /* Read DW_AT_data_location and set in type.  */
> +  attr = dwarf2_attr (die, DW_AT_data_location, cu);
> +  if (attr_form_is_block (attr))

Here (in set_die_type), why do you limit the processing the block-form
data_location attributes? Why not just call attr_to_dynamic_prop
regardless of the form, and let that function deal with whatever
the form is? In other words, trop the form check and just keep
the following bit:

> +      struct dynamic_prop prop;
> +
> +      if (attr_to_dynamic_prop (attr, die, cu, &prop))
> +	{
> +	  TYPE_DATA_LOCATION (type)
> +	    = obstack_alloc (&objfile->objfile_obstack, sizeof (prop));
> +	  *TYPE_DATA_LOCATION (type) = prop;
> +	}

> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1635,8 +1635,12 @@ 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));
> +	else if (TYPE_DATA_LOCATION (type) != NULL
> +          && (TYPE_DATA_LOCATION_KIND (type) == PROP_LOCEXPR
> +              || TYPE_DATA_LOCATION_KIND (type) == PROP_LOCLIST))
> +    return 1;
> +  else
> +    return is_dynamic_type (TYPE_TARGET_TYPE (type));

The comment needs to be updated. Your change is also splitting
the "if bounds or contents is dynamic" logic. Perhaps you could
simply add the data-location check at the start of the function
with a command that says: A type which has a data_location which
[bla bla] is a dynamic type


> +  type = resolved_type;
> +
> +  /* Resolve data_location attribute.  */
> +  prop = TYPE_DATA_LOCATION (type);
> +  if (dwarf2_evaluate_property (prop, addr, &value))
> +    {
> +      TYPE_DATA_LOCATION_ADDR (type) = value;
> +      TYPE_DATA_LOCATION_KIND (type) = PROP_CONST;
> +    }
> +  else
> +    TYPE_DATA_LOCATION (type) = NULL;

Here, I do not understand why you override TYPE, instead of just
using RESOLVED_TYPE directly.

> @@ -722,6 +722,10 @@ struct main_type
>  
>      struct func_type *func_stuff;
>    } type_specific;
> +
> +  /* * Indirection to actual data.  */
> +
> +  struct dynamic_prop *data_location;

I'd like to have a comment which is a little more descriptive of
what this field contains. Someone who reads this comment should be
able to understand it without having to grep for this field to
get an idea of how this field is used.

> +/* Attribute accessors for VLA support.  */

I think this comment is too specific. Although this field is
instroduced to support VLAs, descriptors can probably be used
in other contexts. I don't think you really need a comment,
in this case, though.

> +#define TYPE_DATA_LOCATION(thistype) \
> +  TYPE_MAIN_TYPE(thistype)->data_location
> +#define TYPE_DATA_LOCATION_BATON(thistype) \
> +  TYPE_DATA_LOCATION (thistype)->data.baton
> +#define TYPE_DATA_LOCATION_ADDR(thistype) \
> +  TYPE_DATA_LOCATION (thistype)->data.const_val
> +#define TYPE_DATA_LOCATION_KIND(thistype) \
> +  TYPE_DATA_LOCATION (thistype)->kind
> +
>  /* Moto-specific stuff for FORTRAN arrays.  */
>  
>  #define TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED(arraytype) \
> diff --git a/gdb/value.c b/gdb/value.c
> index d125a09..1c88dfd 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -3635,8 +3635,14 @@ value_fetch_lazy (struct value *val)
>      }
>    else if (VALUE_LVAL (val) == lval_memory)
>      {
> -      CORE_ADDR addr = value_address (val);
>        struct type *type = check_typedef (value_enclosing_type (val));
> +      CORE_ADDR addr;
> +
> +      if (TYPE_DATA_LOCATION (type) != NULL
> +	  && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> +	addr = TYPE_DATA_LOCATION_ADDR (type);
> +      else
> +	addr = value_address (val);

I am wondering if this part shouldn't be in value_address itself.
WDYT? Tom?

>  
>        if (TYPE_LENGTH (type))
>  	read_value_memory (val, 0, value_stack (val),
> -- 
> 1.7.9.5

-- 
Joel