* [PATCH V3] fixed inherit_abstract_dies infinite recursive call
@ 2014-01-22 3:44 manjian2006
2014-01-22 4:31 ` Doug Evans
0 siblings, 1 reply; 2+ messages in thread
From: manjian2006 @ 2014-01-22 3:44 UTC (permalink / raw)
To: gdb-patches, tromey, brobecker, xdje42; +Cc: linzj
From: linzj <linzj@ucweb.com>
Reset the die's in_process bit using cleanup facility.
>> ChangeLog added.
>> Please Joel Brobecker <brobecker@adacore.com> 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 <manjian2006@gmail.com>
+ * 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.
2013-12-19 Keven Boell <keven.boell@intel.com>
* 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;
+ /* 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. */
+static void
+reset_die_in_process(void *arg)
+{
+ 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;
+
+ /* Only process those who are not in process. */
+ if (die->in_process)
+ return;
+ die->in_process = 1;
+ in_process = make_cleanup(reset_die_in_process,die);
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);
}
\f
/* DWARF name computation. */
--
1.8.3.2
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH V3] fixed inherit_abstract_dies infinite recursive call
2014-01-22 3:44 [PATCH V3] fixed inherit_abstract_dies infinite recursive call manjian2006
@ 2014-01-22 4:31 ` Doug Evans
0 siblings, 0 replies; 2+ messages in thread
From: Doug Evans @ 2014-01-22 4:31 UTC (permalink / raw)
To: manjian2006; +Cc: gdb-patches, Tom Tromey, brobecker, linzj
On Tue, Jan 21, 2014 at 7:44 PM, <manjian2006@gmail.com> wrote:
> From: linzj <linzj@ucweb.com>
>
> 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 <brobecker@adacore.com> 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 <manjian2006@gmail.com>
> + * 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 <manjian2006@gmail.com>
* 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 <keven.boell@intel.com>
>
> * 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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-01-22 4:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-22 3:44 [PATCH V3] fixed inherit_abstract_dies infinite recursive call manjian2006
2014-01-22 4:31 ` Doug Evans
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).