Re: [PATCH] Introduce linked list for dynamic attributes

[Joel Brobecker <brobecker at adacore dot com>]

Keven,

On Mon, Mar 02, 2015 at 08:37:56AM +0100, Keven Boell wrote:
> This patch introduces a linked list for dynamic
> attributes of a type. This is a pre-work for
> the Fortran dynamic array support. The Fortran
> dynamic array support will add more dynamic
> attributes to a type. As only a few types
> will have such dynamic attributes set, a linked
> list is more efficient in terms of memory
> consumption than adding multiple attributes
> to main_type.
> 
> Transformed the data_location dynamic attribute
> to use the linked list.

Thanks for working on this.

Comments below. I have a bit of time this week for follow up
reviews, if you have time also.

> 2015-02-23  Keven Boell  <keven.boell@intel.com>
> 
> 	* gdbtypes.c (resolve_dynamic_type_internal):
> 	Adapted data_location usage to linked list.

We use the imperattive style... So "Adapt data_location [...]".

> 	(resolve_dynamic_type_internal): Adapted
> 	data_location usage to linked list.
> 	(get_dyn_attr): New function.
> 	(add_dyn_attr): New function.
> 	(copy_type_recursive): Add copy of linked
> 	list.
> 	(copy_type): Add copy of linked list.
> 	* gdbtypes.h (enum dynamic_prop_kind): Kind
> 	of the dynamic attribute in linked list.
> 	(struct dynamic_prop_list): Dynamic list
> 	node.
> 	* dwarf2read.c (set_die_type): Add data_location
> 	data to linked list using helper functions.

Lots of little comments, but the patch is going in the correct
direction, IMO, so we should be able to converge relatively
quickly, I think.

The comments are in the same order as the patch, which is a little
bit the wrong order. So, you'll see that I make suggestions earlier
that will need fixing due to comments  made afterwards. I thought
it was simpler for me to do it this way, rather than trying to
reorder the patch and risk missing something. I hope this is OK.

> ---
>  gdb/dwarf2read.c |    4 +-
>  gdb/gdbtypes.c   |  120 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  gdb/gdbtypes.h   |   47 ++++++++++++++++++---
>  3 files changed, 148 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index ac78165..9923758 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -22077,9 +22077,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
>    attr = dwarf2_attr (die, DW_AT_data_location, cu);
>    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;
> +      add_dyn_attr (type, objfile, prop, DYN_ATTR_DATA_LOCATION);
>      }

Can you remove the curly braces. It's part of the GDB coding style
where we only use the curly braces when required; if you have only
one statement inside the if block, then we omit the curly braces.
The exception to that rule is when you have a comment in addition
to the statment. Visually, the comment looks the same as a statement,
so we then use curly braces.

>    if (dwarf2_per_objfile->die_type_hash == NULL)
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index a80151c..65a6897 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2021,7 +2021,7 @@ resolve_dynamic_type_internal (struct type *type,
>  {
>    struct type *real_type = check_typedef (type);
>    struct type *resolved_type = type;
> -  const struct dynamic_prop *prop;
> +  struct dynamic_prop *prop;
>    CORE_ADDR value;
>  
>    if (!is_dynamic_type_internal (real_type, top_level))
> @@ -2078,11 +2078,11 @@ resolve_dynamic_type_internal (struct type *type,
>    prop = TYPE_DATA_LOCATION (resolved_type);
>    if (dwarf2_evaluate_property (prop, addr_stack, &value))
>      {
> -      TYPE_DATA_LOCATION_ADDR (resolved_type) = value;
> -      TYPE_DATA_LOCATION_KIND (resolved_type) = PROP_CONST;
> +      TYPE_DYN_ATTR_ADDR (prop) = value;
> +      TYPE_DYN_ATTR_KIND (prop) = PROP_CONST;
>      }
>    else
> -    TYPE_DATA_LOCATION (resolved_type) = NULL;
> +    prop = NULL;

I think we can do better, in this case, as we don't really need
to set prop to NULL. In fact, this appears to be useless to do so?

So, how about:

  prop = TYPE_DATA_LOCATION (resolved_type);
  if (prop != NULL && dwarf2_evaluate_property (prop, addr_stack, &value))
    {
      TYPE_DYN_ATTR_ADDR (prop) = value;
      TYPE_DYN_ATTR_KIND (prop) = PROP_CONST;
    }

This avoids a function call if prop is NULL, even if
dwarf2_evaluate_property does handle it the way we expect it to.

FTR, a thought occured to me, that we might perhaps want
dwarf2_evaluate_property to evaluate in place, but after thinking
about it some more, I think it's going to be more practical to
leave things as is.

> +/* See gdbtypes.h  */
> +
> +struct dynamic_prop * get_dyn_attr (const struct type * type,
> +  enum dynamic_prop_kind kind)

The formatting needs to be fixed:
  - function names for function implementations should be at the start
    of the line
  - no space after '*'

Also, would you mind if I nit-picked a bit, and asked that parameters
be in the following order (generally speaking, not just limited to
that function):
  - prop_kind
  - prop
  - type
  - objfile

Thus:

struct dynamic_prop *
get_dyn_attr (enum dynamic_prop_kind prop_kind, const struct type *type)

(I also changed "kind" to "prop_kind" which, in my opinion, is
a little easier to grasp what it is when reading the code).

Feel free to disagree, of course, but I find that order a little
more logical in terms of how I think about this feature...


> +{
> +  struct dynamic_prop_list * head =
> +    (TYPE_MAIN_TYPE (type))->dyn_attribs;

Formatting:
  - No space after '*';
  - The '=' should be at the start of the next line (GNU coding
    style regarding the formatting where binary operators are involved)

Also, please forgive the nitpick, but I think it would be better
if you renamed "head" to something like "node". Anything that does
not suggest that your variable points to the head of the list.

> +  while (head != NULL)
> +    {
> +      if (head->kind == kind)
> +        return head->prop;
> +      head = head->next;
> +    }
> +  return NULL;
> +}
> +
> +/* See gdbtypes.h  */
> +
> +void add_dyn_attr (struct type * type, struct objfile *objfile,
> +  struct dynamic_prop prop, enum dynamic_prop_kind kind)
> +{
> +  struct dynamic_prop_list * temp
> +    = obstack_alloc (&objfile->objfile_obstack,
> +              sizeof (struct dynamic_prop_list));
> +  temp->prop = obstack_alloc (&objfile->objfile_obstack, sizeof (prop));
> +  *temp->prop = prop;

Same remarks as above (formatting, parameter order and names, etc).
So:

void
add_dyn_attr (enum dynamic_prop_kind prop_kind, struct dynamic_prop prop,
              struct type *type, struct objfile *objfile)

> +  temp->kind = kind;
> +
> +  if (TYPE_MAIN_TYPE (type)->dyn_attribs == NULL)
> +    {
> +      TYPE_MAIN_TYPE (type)->dyn_attribs = temp;
> +      temp->next = NULL;
> +    }
> +  else
> +    {
> +      temp->next = TYPE_MAIN_TYPE (type)->dyn_attribs;
> +      TYPE_MAIN_TYPE (type)->dyn_attribs = temp;
> +    }
> +}

I think you can simply the above with:

    temp->next = TYPE_MAIN_TYPE (type)->dyn_attribs;
    TYPE_MAIN_TYPE (type)->dyn_attribs = temp;

>  /* Find the real type of TYPE.  This function returns the real type,
>     after removing all layers of typedefs, and completing opaque or stub
>     types.  Completion changes the TYPE argument, but stripping of
> @@ -4321,15 +4363,39 @@ copy_type_recursive (struct objfile *objfile,
>        *TYPE_RANGE_DATA (new_type) = *TYPE_RANGE_DATA (type);
>      }
>  
> -  /* Copy the data location information.  */
> -  if (TYPE_DATA_LOCATION (type) != NULL)
> +  if (TYPE_DYN_ATTRIBS (type) != NULL)
>      {
> -      TYPE_DATA_LOCATION (new_type)
> -	= TYPE_ALLOC (new_type, sizeof (struct dynamic_prop));
> -      memcpy (TYPE_DATA_LOCATION (new_type), TYPE_DATA_LOCATION (type),
> -	      sizeof (struct dynamic_prop));
> +      struct dynamic_prop_list * p;
> +      struct dynamic_prop_list * trav = TYPE_DYN_ATTRIBS (type);
> +
> +      /* Copy head.  */
> +      struct dynamic_prop_list * new_head =
> +        TYPE_ALLOC (new_type, sizeof (struct dynamic_prop_list));
> +      memcpy (new_head, TYPE_DYN_ATTRIBS (type),
> +        sizeof (struct dynamic_prop_list));
> +      new_head->prop = TYPE_ALLOC (new_type, sizeof (struct dynamic_prop));
> +      memcpy (new_head->prop, TYPE_DYN_ATTRIBS (type)->prop,
> +        sizeof (struct dynamic_prop));
> +
> +      /* Rest of list.  */
> +      p = new_head;
> +      trav = trav->next;
> +      while (trav != NULL)
> +        {
> +          p->next = TYPE_ALLOC (new_type, sizeof (struct dynamic_prop_list));
> +          memcpy (p->next, trav,
> +            sizeof (struct dynamic_prop_list));
> +          p = p->next;
> +          p->prop = TYPE_ALLOC (new_type, sizeof (struct dynamic_prop));
> +          memcpy (p->prop, trav->prop, sizeof (struct dynamic_prop));
> +          trav = trav->next;
> +        }
> +      p->next = NULL;
> +
> +      TYPE_DYN_ATTRIBS (new_type) = new_head;

The same is done at two locations, so let's factorize this code.
Suggest we create a function which, given a prop list, returns
a copy of the prop list. That way, I suspect the code will just
become:

    if (TYPE_DYN_ATTRIBS (type) != NULL)
      TYPE_DYN_ATTRIBS (new_type)
        =  dynamic_prop_list_copy (TYPE_DYN_ATTRIBS (type));

And the function will probably not have to worry about the "head"
vs "the rest" thing.

Watch out for the formatting issues I've been mentioning earlier.

> +
>    /* Copy pointers to other types.  */
>    if (TYPE_TARGET_TYPE (type))
>      TYPE_TARGET_TYPE (new_type) = 
> @@ -4392,12 +4458,36 @@ copy_type (const struct type *type)
>    TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>    memcpy (TYPE_MAIN_TYPE (new_type), TYPE_MAIN_TYPE (type),
>  	  sizeof (struct main_type));
> -  if (TYPE_DATA_LOCATION (type) != NULL)
> +  if (TYPE_DYN_ATTRIBS (type) != NULL)
>      {
> -      TYPE_DATA_LOCATION (new_type)
> -	= TYPE_ALLOC (new_type, sizeof (struct dynamic_prop));
> -      memcpy (TYPE_DATA_LOCATION (new_type), TYPE_DATA_LOCATION (type),
> -	      sizeof (struct dynamic_prop));
> +      struct dynamic_prop_list * p;
> +      struct dynamic_prop_list * trav = TYPE_DYN_ATTRIBS (type);
> +
> +      /* Copy head.  */
> +      struct dynamic_prop_list * new_head =
> +        TYPE_ALLOC (new_type, sizeof (struct dynamic_prop_list));
> +      memcpy (new_head, TYPE_DYN_ATTRIBS (type),
> +        sizeof (struct dynamic_prop_list));
> +      new_head->prop = TYPE_ALLOC (new_type, sizeof (struct dynamic_prop));
> +      memcpy (new_head->prop, TYPE_DYN_ATTRIBS (type)->prop,
> +        sizeof (struct dynamic_prop));
> +
> +      /* Rest of list.  */
> +      p = new_head;
> +      trav = trav->next;
> +      while (trav != NULL)
> +        {
> +          p->next = TYPE_ALLOC (new_type, sizeof (struct dynamic_prop_list));
> +          memcpy (p->next, trav,
> +            sizeof (struct dynamic_prop_list));
> +          p = p->next;
> +          p->prop = TYPE_ALLOC (new_type, sizeof (struct dynamic_prop));
> +          memcpy (p->prop, trav->prop, sizeof (struct dynamic_prop));
> +          trav = trav->next;
> +        }
> +      p->next = NULL;
> +
> +      TYPE_DYN_ATTRIBS (new_type) = new_head;

Same as above - factorize and simplify.

>      }
>  
>    return new_type;
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index ef6d92c..ab3e2cc 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -430,6 +430,25 @@ struct dynamic_prop
>    } data;
>  };
>  
> +/* * Defines the kind of a dynamic attribute of a type. The dynamic attributes
> +   can be the following:
> +
> +   * DYN_ATTR_DATA_LOCATION:
> +     Contains a location description value for the current type.
> +     Evaluating this field yields to the location of the data for an object.
> +*/
> +enum dynamic_prop_kind
> +{
> +  DYN_ATTR_DATA_LOCATION
> +};

"Defines" -> "Define" (we use the imperative in our function
documentation). But I'm having difficulties understanding
that sentence, so let me try to suggest something else (see
proposal below).

Two spaces after periods. But I think this becomes moot if we document
each enum kind individually (inside the enum definition), which is what
we usually do. This will also avoid risks of the documentation
bit-rotting on us when we add/remove/change names...

Therefore:

/* * Define a type's dynamic property kind.  */

enum dynamic_prop_kind
{
  /* A property providing a type's data location.
     Evaluating this field yields to the location of an object's data.  */
  DYN_ATTR_DATA_LOCATION,
};

(I've added a coma at the end of DYN_ATTR_DATA_LOCATION. It's not
necessary, but avoids having to touch that line when adding extra
enumerates)

> +/* * List for dynamic type attributes.  */
> +struct dynamic_prop_list
> +{
> +  enum dynamic_prop_kind kind;
> +  struct dynamic_prop *prop;
> +  struct dynamic_prop_list *next;
> +};

If you don't mind, let's rename "kind" to "prop_kind" to be consistent
with the naming used elsewhere?

Also, I know I propose to make "prop" a pointer, but I think the code
will be simpler if we remove the pointer indirection. And we'll also
having to store a pointer, which saves us 4-8 bytes per dynamic
property...


>  /* * Determine which field of the union main_type.fields[x].loc is
>     used.  */
> @@ -704,10 +723,8 @@ struct main_type
>      struct type *self_type;
>    } type_specific;
>  
> -  /* * Contains a location description value for the current type. Evaluating
> -     this field yields to the location of the data for an object.  */
> -
> -  struct dynamic_prop *data_location;
> +  /* * Contains all dynamic type attributes.  */
> +  struct dynamic_prop_list *dyn_attribs;

We have been calling these "properties", so can we remain consistent
with that? Can we rename the field to "prop_list", for instance?
Or something else that you might prefer? Also, let's adjust the
comment to use "property" as well.

>  };
>  
>  /* * A ``struct type'' describes a particular instance of a type, with
> @@ -1221,7 +1238,7 @@ extern void allocate_gnat_aux_type (struct type *);
>  
>  /* Attribute accessors for the type data location.  */
>  #define TYPE_DATA_LOCATION(thistype) \
> -  TYPE_MAIN_TYPE(thistype)->data_location
> +  get_dyn_attr (thistype, DYN_ATTR_DATA_LOCATION)
>  #define TYPE_DATA_LOCATION_BATON(thistype) \
>    TYPE_DATA_LOCATION (thistype)->data.baton
>  #define TYPE_DATA_LOCATION_ADDR(thistype) \

IMO, I would remove all these macros, and let users call
get_dyn_attr, and then manipulate the property using the generic
macros that you're defining instead.

If it makes the patch bigger, let's keep that activity as patch #2.
It can be done independently.

> @@ -1229,6 +1246,17 @@ extern void allocate_gnat_aux_type (struct type *);
>  #define TYPE_DATA_LOCATION_KIND(thistype) \
>    TYPE_DATA_LOCATION (thistype)->kind
>  
> +  /* Attribute accessors for dynamic attributes.  */

Properties.

> +#define TYPE_DYN_ATTRIBS(thistype) \
> +  TYPE_MAIN_TYPE(thistype)->dyn_attribs
> +#define TYPE_DYN_ATTR_BATON(dynprop) \
> +  dynprop->data.baton
> +#define TYPE_DYN_ATTR_ADDR(dynprop) \
> +  dynprop->data.const_val
> +#define TYPE_DYN_ATTR_KIND(dynprop) \
> +  dynprop->kind

Let's also s/_ATTR_/_PROP_/ if you don't mind.

>  /* Moto-specific stuff for FORTRAN arrays.  */
>  
>  #define TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED(arraytype) \
> @@ -1748,6 +1776,15 @@ extern struct type *resolve_dynamic_type (struct type *type, CORE_ADDR addr);
>  /* * Predicate if the type has dynamic values, which are not resolved yet.  */
>  extern int is_dynamic_type (struct type *type);
>  
> +/* * Fetches a dynamic attribute of a type out of the dynamic attribute
> +   list.  */
> +extern struct dynamic_prop * get_dyn_attr
> +  (const struct type * type, enum dynamic_prop_kind kind);

"Fetches" -> "Fetch"; "attribute" -> "property";
"out of" -> "from".
Also, can you document the fact that the function returns NULL
of the dynamic property cannnot be found.

Also, can you change the function's name to say "prop" instead of
"attr"?

Please also fix the function's formatting. In this case, since this
is only a declaration, the function's name should  NOT be at the start

> +/* * Adds a dynamic attribute to a type.  */
> +extern void add_dyn_attr (struct type * type, struct objfile *objfile,
> +  struct dynamic_prop prop, enum dynamic_prop_kind kind);

Same kind of remarks as above.

> +
>  extern struct type *check_typedef (struct type *);
>  
>  #define CHECK_TYPEDEF(TYPE)			\
> -- 
> 1.7.9.5

Thanks again for the patch,
-- 
Joel