This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH V3] fixed inherit_abstract_dies infinite recursive call
- From: Doug Evans <xdje42 at gmail dot com>
- To: manjian2006 at gmail dot com
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, Tom Tromey <tromey at redhat dot com>, brobecker at adacore dot com, linzj <linzj at ucweb dot com>
- Date: Tue, 21 Jan 2014 20:31:53 -0800
- Subject: Re: [PATCH V3] fixed inherit_abstract_dies infinite recursive call
- Authentication-results: sourceware.org; auth=none
- References: <1390362244-3146-1-git-send-email-manjian2006 at gmail dot com>
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.