Re: [PATCH] Introduce linked list for dynamic attributes

[Keven Boell <keven.boell@linux.intel.com>]

Joel,

Thanks for the review and all the feedback. I've addressed most of the things you mentioned.
Please see my comments below.

On 02.03.2015 20:49, Joel Brobecker wrote:
> 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.

Done.

> 
>>    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.

Done.

> 
> 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...
> 

Makes sense, I was not thinking about the order of the attributes to be honest.
I've changed them as proposed.

> 
>> +{
>> +  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.
> 

Done.

>> +  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)

Done.

> 
>> +  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;

Done.

> 
>>  /* 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.

I've refactored and simplified the code.

> 
>> +
>>    /* 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.

Done.

> 
>>      }
>>  
>>    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)
> 

Done.

>> +/* * 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?
> 

Done.

> 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...
> 

I would like to keep the pointer for now as the data_location was a pointer before.
It would otherwise require some refactoring of the callers. Is this ok with you?

> 
>>  /* * 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.

Done.

> 
>>  };
>>  
>>  /* * 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.
> 

I think it makes the patch bigger, as all callers need to be refactored. Not much work actually, but this is kind of unrelated to this patch. So I would like to keep it as is for now. Agree?

>> @@ -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.

Done.

> 
>> +#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.

Done.
> 
>>  /* 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"?

Done.

> 
> 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
> 

Isn't this the case here? I don't understand what to change here.

>> +/* * 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,
> 

Thanks,
Keven