This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Introduce linked list for dynamic attributes
- From: Keven Boell <keven dot boell at linux dot intel dot com>
- To: Joel Brobecker <brobecker at adacore dot com>, Keven Boell <keven dot boell at intel dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 03 Mar 2015 14:43:13 +0100
- Subject: Re: [PATCH] Introduce linked list for dynamic attributes
- Authentication-results: sourceware.org; auth=none
- References: <1425281876-20302-1-git-send-email-keven dot boell at intel dot com> <20150302194959 dot GD4449 at adacore dot 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