From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25092 invoked by alias); 3 Mar 2015 13:43:23 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 25080 invoked by uid 89); 3 Mar 2015 13:43:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mga01.intel.com Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Mar 2015 13:43:18 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 03 Mar 2015 05:43:17 -0800 X-ExtLoop1: 1 Received: from kboell-mobl2.ger.corp.intel.com (HELO [172.28.205.49]) ([172.28.205.49]) by orsmga001.jf.intel.com with ESMTP; 03 Mar 2015 05:43:15 -0800 Message-ID: <54F5BA71.2050004@linux.intel.com> Date: Tue, 03 Mar 2015 13:43:00 -0000 From: Keven Boell User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Joel Brobecker , Keven Boell CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Introduce linked list for dynamic attributes References: <1425281876-20302-1-git-send-email-keven.boell@intel.com> <20150302194959.GD4449@adacore.com> In-Reply-To: <20150302194959.GD4449@adacore.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00079.txt.bz2 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 >> >> * 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