From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17310 invoked by alias); 22 Jan 2014 04:31:59 -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 17301 invoked by uid 89); 22 Jan 2014 04:31:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f49.google.com Received: from mail-wg0-f49.google.com (HELO mail-wg0-f49.google.com) (74.125.82.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 22 Jan 2014 04:31:57 +0000 Received: by mail-wg0-f49.google.com with SMTP id a1so8618377wgh.4 for ; Tue, 21 Jan 2014 20:31:53 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.194.59.210 with SMTP id b18mr239555wjr.60.1390365113623; Tue, 21 Jan 2014 20:31:53 -0800 (PST) Received: by 10.194.17.104 with HTTP; Tue, 21 Jan 2014 20:31:53 -0800 (PST) In-Reply-To: <1390362244-3146-1-git-send-email-manjian2006@gmail.com> References: <1390362244-3146-1-git-send-email-manjian2006@gmail.com> Date: Wed, 22 Jan 2014 04:31:00 -0000 Message-ID: Subject: Re: [PATCH V3] fixed inherit_abstract_dies infinite recursive call From: Doug Evans To: manjian2006@gmail.com Cc: "gdb-patches@sourceware.org" , Tom Tromey , brobecker@adacore.com, linzj Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2014-01/txt/msg00832.txt.bz2 On Tue, Jan 21, 2014 at 7:44 PM, wrote: > From: linzj > > Reset the die's in_process bit using cleanup facility. Cool, thanks. A couple more nits inline. [apologies. gdb's coding standards are that pedantic, which in some sense is a good thing - the consistency tends to make adhering to them easier to achieve] btw, do you have a copyright assignment on file? This change feels small enough to me to not need one, but it's not clear. >>> ChangeLog added. >>> Please Joel Brobecker helps with the testcases. >>> >> The c++ code causing the problem is: >>> >> >>> >> // Integer variants of certain metrics, used for HTML rendering. >>> >> int ascent(FontBaseline baselineType = AlphabeticBaseline) const >>> >> { >>> >> if (baselineType == AlphabeticBaseline) >>> >> return lroundf(m_ascent); >>> >> return height() - height() / 2; >>> >> } >>> >> >>> >> int height(FontBaseline baselineType = AlphabeticBaseline) const >>> >> { >>> >> return ascent(baselineType) + descent(baselineType); >>> >> } >>> >> >>> >> As you can see,ascent(0x5816d55) calls height(0x5812c1b),and height calls >>> >> ascent(0x5816d55) recursivly.And the compiler generates these dwarf code >>> >> representing this relationship preciously. >>> >> >>> >> A dwarf die may have the following relationship: >>> >> 564860c<----------------------------- >>> >> | | >>> >> |(abstract origin) | >>> >> | | >>> >> V | >>> >> 5816d55 | (abstract origin) >>> >> | | >>> >> |(child) | >>> >> | | >>> >> V | >>> >> ... | >>> >> 5812c34------------------------------ >>> >> So inherit_abstract_dies may results in infinite recursive call. >>> >> A bit field call in_process has been add to struct die_info to fix this problem. >>> >> process_die would first check if a die is in processing state, if so,just return. >>> >> Then in_process bit is set.Before process_die returns,this bit field is unset. > --- > ChangeLog | 8 ++++++++ > gdb/dwarf2read.c | 19 +++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/ChangeLog b/ChangeLog > index 9b1cbfa..c17af8d 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,11 @@ > +2013-01-20 lin Zuojian > + * gdb/dwarf2read.c(struct die_info): Add a > + bit to form a bitmap to avoid visit twice > + when process_die. > + * gdb/dwarf2read.c(process_die): Test in_process > + bit to avoid visit twice.Set in_process bit > + before doing actual jobs.Unset in_process bit > + before process_die returns. 1) Don't include the diff to the ChangeLog, cut-n-paste the text as is to the top of the patch in the email submission. 2) Don't preface file names with gdb/ in the ChangeLog. If the patch involves changes to files in other directories for which gdb's ChangeLog file is the correct one to use, *then* include the directory. E.g., common/queue.h. If the patch involves changes to files in other directories that have their own ChangeLog file, then write the ChangeLog entry in the email as, e.g., testsuite/ * lib/gdb.exp (foo): mumble. But remove the line with "testsuite/" when committing the change. 3) Don't repeat the file name within one ChangeLog entry. [well, for some particularly complex entries it could be useful, but not in general] 4) You don't need to explain the change in the changelog entry, just state what was changed. 5) Blank line after the date/name line. 6) Use the tab character to indent each line. 7) Space after the file name. Combining all of that yields: [assuming cut-n-paste preserved the tabs] 2013-01-20 lin Zuojian * dwarf2read.c (struct die_info): New member in_process. (reset_die_in_process): New function. (process_die): Set it at the start, reset when returning. Most of these rules should be self-evident from reading existing entries, but I can understand there still being some indecision as to "what's right". > 2013-12-19 Keven Boell > > * cp-namespace.c (cp_lookup_nested_symbol): Enable > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 7ca527d..cf27283 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -1224,6 +1224,8 @@ struct die_info > /* True if we're presently building the full type name for the > type derived from this DIE. */ > unsigned char building_fullname : 1; It would be consistent with the rest of the definition of this struct to have a blank line here (though I realize it's a bit of a toss up). > + /* True if this die is in process. */ > + unsigned char in_process : 1; > > /* Abbrev number */ > unsigned int abbrev; > @@ -8008,11 +8010,27 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) > } > } > > +/* Reset the in_process bit of a die. */ Blank line between the comment describing the function and its definition. > +static void > +reset_die_in_process(void *arg) Space before (. > +{ > + struct die_info *die = arg; > + die->in_process = 0; > +} > + > /* Process a die and its children. */ > > static void > process_die (struct die_info *die, struct dwarf2_cu *cu) > { > + /* The in process bit of a die's cleanup. */ > + struct cleanup * in_process; No space after *. I'm ambivalent on keeping the comment. I could see deleting it, but if you want to keep it it's fine with me. > + > + /* Only process those who are not in process. */ English wasn't my best subject in school, but this reads better to me: /* Only process those not already in process. */ > + if (die->in_process) > + return; > + die->in_process = 1; > + in_process = make_cleanup(reset_die_in_process,die); Space before (. > switch (die->tag) > { > case DW_TAG_padding: > @@ -8100,6 +8118,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) > new_symbol (die, NULL, cu); > break; > } > + do_cleanups(in_process); Space before (. > } > > /* DWARF name computation. */ > -- > 1.8.3.2 > I realize it's a pain to fix all these nits even before the actual core of the patch has been accepted. It's fine with me, but I would wait a bit to see if others have any comments.