[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;
>  }
>  
>  /* DWARF name computation.  */
> -- 
> 1.8.3.2

-- 
Joel