[PATCH] fixed inherit_abstract_dies infinite recursive call

[PATCH] fixed inherit_abstract_dies infinite recursive call

[manjian2006]

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

Re: [PATCH] fixed inherit_abstract_dies infinite recursive call

[Doug Evans]

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]

Re: [PATCH] fixed inherit_abstract_dies infinite recursive call

[Joel Brobecker]

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

[PATCH] fixed inherit_abstract_dies infinite recursive call

[manjian2006]

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

[PATCH] fixed inherit_abstract_dies infinite recursive call

[manjian2006]

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

Re: [PATCH] fixed inherit_abstract_dies infinite recursive call

[Tom Tromey]

>>>>> ">" == 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

[PATCH] fixed inherit_abstract_dies infinite recursive call

[manjian2006]

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

[PATCH] fixed inherit_abstract_dies infinite recursive call

[manjian2006]

From: manjian2006<manjian2006@gmail.com>

---
 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