* [PATCH] fixed inherit_abstract_dies infinite recursive call @ 2014-01-20 4:25 manjian2006 2014-01-20 5:53 ` Tom Tromey 2014-01-20 6:30 ` manjian2006 0 siblings, 2 replies; 7+ messages in thread From: manjian2006 @ 2014-01-20 4:25 UTC (permalink / raw) To: gdb-patches; +Cc: linzj From: linzj <linzj@ucweb.com> 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. --- gdb/dwarf2read.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 7ca527d..4532251 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; + /* True if this die is in process */ + unsigned char in_process : 1; /* Abbrev number */ unsigned int abbrev; @@ -8013,6 +8015,10 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) static void process_die (struct die_info *die, struct dwarf2_cu *cu) { + /* Only process those who are not in process */ + if(die->in_process) + return; + die->in_process = 1; switch (die->tag) { case DW_TAG_padding: @@ -8100,6 +8106,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) new_symbol (die, NULL, cu); break; } + die->in_process = 0; } \f /* DWARF name computation. */ -- 1.8.3.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed inherit_abstract_dies infinite recursive call 2014-01-20 4:25 [PATCH] fixed inherit_abstract_dies infinite recursive call manjian2006 @ 2014-01-20 5:53 ` Tom Tromey 2014-01-21 1:43 ` manjian2006 2014-01-20 6:30 ` manjian2006 1 sibling, 1 reply; 7+ messages in thread From: Tom Tromey @ 2014-01-20 5:53 UTC (permalink / raw) To: manjian2006; +Cc: gdb-patches, linzj >>>>> ">" == manjian2006 <manjian2006@gmail.com> writes: >> From: linzj <linzj@ucweb.com> >> A dwarf die may have the following relationship: >> 564860c<----------------------------- >> | | >> |(abstract origin) | >> | | >> V | >> 5816d55 | (abstract origin) >> | | >> |(child) | >> | | >> V | >> ... | >> 5812c34------------------------------ What does it mean when this happens? It seems very strange to me, like it must be a compiler bug. >> + /* True if this die is in process */ Comments must end with a period and two spaces. I would write something like "True if this DIE is currently being processed." though. >> + /* Only process those who are not in process */ Ditto about the formatting. >> + if(die->in_process) Space before open paren. I think this needs a test case. It should be possible, I think, to write one using the DWARF assembler. Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] fixed inherit_abstract_dies infinite recursive call 2014-01-20 5:53 ` Tom Tromey @ 2014-01-21 1:43 ` manjian2006 2014-01-21 7:54 ` Joel Brobecker 0 siblings, 1 reply; 7+ messages in thread From: manjian2006 @ 2014-01-21 1:43 UTC (permalink / raw) To: gdb-patches, tromey; +Cc: linzj From: linzj <linzj@ucweb.com> 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. --- gdb/dwarf2read.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 7ca527d..c226a52 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; + /* True if this die is in process. */ + unsigned char in_process : 1; /* Abbrev number */ unsigned int abbrev; @@ -8013,6 +8015,10 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) static void process_die (struct die_info *die, struct dwarf2_cu *cu) { + /* Only process those who are not in process. */ + if (die->in_process) + return; + die->in_process = 1; switch (die->tag) { case DW_TAG_padding: @@ -8100,6 +8106,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) new_symbol (die, NULL, cu); break; } + die->in_process = 0; } \f /* DWARF name computation. */ -- 1.8.3.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed inherit_abstract_dies infinite recursive call 2014-01-21 1:43 ` manjian2006 @ 2014-01-21 7:54 ` Joel Brobecker 2014-01-21 10:17 ` manjian2006 0 siblings, 1 reply; 7+ messages in thread From: Joel Brobecker @ 2014-01-21 7:54 UTC (permalink / raw) To: manjian2006; +Cc: gdb-patches, tromey, linzj For the record, we've actually seen the exact same behavior from with GNAT a few years ago. The problem occured when when we had the following situation: procedure Outer (...) is [...] procedure Inner (...) is [...] -- Recurse by calling Outer.. Outer (...); end Inner; begin [...] Inner (...); In that case, when compiled at -O3, the compiler generated an Abstract Instance Root (AIR) for procedure Outer, which owned/contained a DIE defining procedure Inner (an out-of-line instance), which itself contains a Concrete Instance Entry (CIIE) corresponding to the inlined version of Outer's AIR. The CIIE has a reference to its AIR via the DW_AT_abstract_origin attribute, hence the cycle. Not being all quite sure how to make sense of the cycle in terms of inheritance, we initially tried to fix in the compiler. Although the patch was initially approved in GCC, it was noted that the output appeared to be conformant with the DWARF specifications (version 3,i at the time, now version 4), particularly in light of the examples in section D.7 of the specifications (3rd example). Eventually, it was found that our GCC patch was causing some issues, and thus was reverted. So far, we've kept the patch in AdaCore's GCC tree, with a note to look into a GDB fix at some point. We are indeed very close to the example from the DWARF specs cited above, but it does not deal with recursion as we do here, so I think that the DWARF specifications do have a hole when it comes to that. Logically speaking, its seems that the sensible thing to do is to inherit the whole Abstract Instance Tree (AIT) but excluding oneself. If that's what the proposed patch is doing, then I think it's a step in the right direction. It's not completely obvious to me what the actual impact is, however. I'll need to study the rest of the code a little more. A review from this file's gurus, with the above info as complement to this patch, would probably make better sense - and be greatly appreciated! Now, linzj@ucweb.com/manjian2006@gmail.com, a few comments and suggestions: . Your patch is not complete, it's missing at least a ChangeLog entry, and I think a testcase would be nice. Did you have a chance to read our little checklist for submitting patches? https://sourceware.org/gdb/wiki/ContributionChecklist For the testcase, I might be able to help. . You re-posted the patch several times, without indicating why you did so, or what changed from version to version. May I kindly request, when you repost a version: + Make sure to add a version number (check for "PATCH v" in the archives at http://www.sourceware.org/ml/gdb-patches/2014-01/, plenty of examples there) + Always indicate what changed. . Do you have an FSF assignment on file? (it'd be nice to know how to address you, btw...) Thank you. On Tue, Jan 21, 2014 at 09:43:28AM +0800, manjian2006@gmail.com wrote: > From: linzj <linzj@ucweb.com> > > 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. > --- > gdb/dwarf2read.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 7ca527d..c226a52 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; > + /* True if this die is in process. */ > + unsigned char in_process : 1; > > /* Abbrev number */ > unsigned int abbrev; > @@ -8013,6 +8015,10 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) > static void > process_die (struct die_info *die, struct dwarf2_cu *cu) > { > + /* Only process those who are not in process. */ > + if (die->in_process) > + return; > + die->in_process = 1; > switch (die->tag) > { > case DW_TAG_padding: > @@ -8100,6 +8106,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) > new_symbol (die, NULL, cu); > break; > } > + die->in_process = 0; > } > \f > /* DWARF name computation. */ > -- > 1.8.3.2 -- Joel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Re: [PATCH] fixed inherit_abstract_dies infinite recursive call 2014-01-21 7:54 ` Joel Brobecker @ 2014-01-21 10:17 ` manjian2006 0 siblings, 0 replies; 7+ messages in thread From: manjian2006 @ 2014-01-21 10:17 UTC (permalink / raw) To: gdb-patches, tromey, brobecker; +Cc: linzj From: linzj <linzj@ucweb.com> I don't think my patch is able to fill the hole of DWARF specs,and there my not be any hole. My patch is adding a bit to die and actully form a bitmap when processing dwarf byte code. If you think process_die as graph traversal,that bitmap is to avoid visiting a node twice. I post duplicatly simply because I don't know how to use mail list. I will add the change logs in next patch. I change the source so few,I don't think an FSF assignment is needed(not more than 10 lines). ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] fixed inherit_abstract_dies infinite recursive call 2014-01-20 4:25 [PATCH] fixed inherit_abstract_dies infinite recursive call manjian2006 2014-01-20 5:53 ` Tom Tromey @ 2014-01-20 6:30 ` manjian2006 2014-01-22 2:45 ` Doug Evans 1 sibling, 1 reply; 7+ messages in thread From: manjian2006 @ 2014-01-20 6:30 UTC (permalink / raw) To: gdb-patches, tromey; +Cc: linzj From: linzj <linzj@ucweb.com> 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. --- gdb/dwarf2read.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 7ca527d..c226a52 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; + /* True if this die is in process. */ + unsigned char in_process : 1; /* Abbrev number */ unsigned int abbrev; @@ -8013,6 +8015,10 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) static void process_die (struct die_info *die, struct dwarf2_cu *cu) { + /* Only process those who are not in process. */ + if (die->in_process) + return; + die->in_process = 1; switch (die->tag) { case DW_TAG_padding: @@ -8100,6 +8106,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) new_symbol (die, NULL, cu); break; } + die->in_process = 0; } \f /* DWARF name computation. */ -- 1.8.3.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed inherit_abstract_dies infinite recursive call 2014-01-20 6:30 ` manjian2006 @ 2014-01-22 2:45 ` Doug Evans 0 siblings, 0 replies; 7+ messages in thread From: Doug Evans @ 2014-01-22 2:45 UTC (permalink / raw) To: manjian2006; +Cc: gdb-patches, Tom Tromey, linzj On Sun, Jan 19, 2014 at 10:29 PM, <manjian2006@gmail.com> wrote: > From: linzj <linzj@ucweb.com> > > 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. > --- > gdb/dwarf2read.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 7ca527d..c226a52 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; > + /* True if this die is in process. */ > + unsigned char in_process : 1; > > /* Abbrev number */ > unsigned int abbrev; > @@ -8013,6 +8015,10 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) > static void > process_die (struct die_info *die, struct dwarf2_cu *cu) > { > + /* Only process those who are not in process. */ > + if (die->in_process) > + return; > + die->in_process = 1; > switch (die->tag) > { > case DW_TAG_padding: > @@ -8100,6 +8106,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) > new_symbol (die, NULL, cu); > break; > } > + die->in_process = 0; > } > > /* DWARF name computation. */ Hi. Setting aside whether this is The Right solution (I haven't absorbed enough to say for sure, and I don't mind this patch if it's a step in the right direction) ... While gdb shouldn't throw an error while reading dwarf info, there are times when the situation is rare enough and sufficiently hard enough to avoid that we throw an error anyway. If an error happens and the bit is left set, is the resulting behaviour ok? Possibly, but it feels cleaner, and keeps things simpler to reason about, if we use a cleanup here to reset the bit. [E.g., it's not clear to me that gdb will necessarily always avoid rereading the debug info, now and in the future, if an error happens when reading it the first time] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-01-22 2:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-20 4:25 [PATCH] fixed inherit_abstract_dies infinite recursive call manjian2006 2014-01-20 5:53 ` Tom Tromey 2014-01-21 1:43 ` manjian2006 2014-01-21 7:54 ` Joel Brobecker 2014-01-21 10:17 ` manjian2006 2014-01-20 6:30 ` manjian2006 2014-01-22 2:45 ` Doug Evans
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox