This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: PING: [PATCH v4] fixed inherit_abstract_dies infinite recursive call
- From: Doug Evans <xdje42 at gmail dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: æäå <manjian2006 at gmail dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, Tom Tromey <tromey at redhat dot com>, linzj <linzj at ucweb dot com>
- Date: Mon, 10 Feb 2014 09:37:12 -0800
- Subject: Re: PING: [PATCH v4] fixed inherit_abstract_dies infinite recursive call
- Authentication-results: sourceware.org; auth=none
- References: <1390374431-17981-1-git-send-email-manjian2006 at gmail dot com> <20140128120600 dot GG4101 at adacore dot com> <20140210142831 dot GY5485 at adacore dot com>
On Mon, Feb 10, 2014 at 6:28 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Ping!
>
> It would be interesting to have a formal review of this patch,
> to know if it is an acceptable fix or not. If not, I can schedule
> some time to follow any recommendation that might come out of
> this review.
>
> Thank you!
>
> On Tue, Jan 28, 2014 at 04:06:00PM +0400, Joel Brobecker wrote:
>> > > 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.
>> >
>> > I am a Chinese guy,and Chinese have not clue about the copyright.
>> > (A joke.I don't need copyright.)
>>
>> It's actually not for your personal benefit, but rather to help the FSF
>> enforce the GPL license on the code you are contributing, thus helping
>> it defend the freedom of our collective code. See:
>> http://www.gnu.org/licenses/why-assign.html
>>
>> > >> Please Joel Brobecker <brobecker@adacore.com> helps with the testcases.
>>
>> Attached is a testcase that causes the debugger to crash on
>> x86_64-linux. It should work on all ELF targets.
>>
>> A plea to the dwarf2read.c gurus:
>>
>> Would it be possible to take a look at this patch, to see if it is
>> going in the right direction? Otherwise, I'll take a deeper look,
>> and see if I can solve it better. Intuitively, I think it may work,
>> but almost as a side-effect. Could the recursion check introduced
>> here do more than what we'd want to, for instance?
>>
>> Thanks!
>>
>> > >>> 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 | 4 ++++
>> > gdb/dwarf2read.c | 20 ++++++++++++++++++++
>> > 2 files changed, 24 insertions(+)
>> >
>> > diff --git a/ChangeLog b/ChangeLog
>> > index 9b1cbfa..0098a72 100644
>> > --- a/ChangeLog
>> > +++ b/ChangeLog
>> > @@ -1,3 +1,7 @@
>> > +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.
>> > 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..ffedde5 100644
>> > --- a/gdb/dwarf2read.c
>> > +++ b/gdb/dwarf2read.c
>> > @@ -1225,6 +1225,9 @@ struct die_info
>> > 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 +8011,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)
>> > {
>> > + struct cleanup *in_process;
>> > +
>> > + /* 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);
>> > switch (die->tag)
>> > {
>> > case DW_TAG_padding:
>> > @@ -8100,6 +8119,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
>> > new_symbol (die, NULL, cu);
>> > break;
>> > }
>> > + do_cleanups (in_process);
>> > }
>> >
>> > /* DWARF name computation. */
>> > --
>> > 1.8.3.2
>>
Hi.
How hard is it to write a testcase that triggers the problem?