Re: PING: [PATCH v4] fixed inherit_abstract_dies infinite recursive call

[Doug Evans <xdje42@gmail.com>]

On Mon, Feb 10, 2014 at 6:19 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> How hard is it to write a testcase that triggers the problem?
>
> I wrote one and I thought I posted it here. But I had forgotten
> to attach the patch! (git reflog just saved my life).
>
> Here it is again.
>
> gdb/testsuite/ChangeLog:
>
>         * gdb.dwarf2/dw2-icycle.S, gdb.dwarf2/dw2-icycle.c,
>         gdb.dwarf2/dw2-icycle.exp: New files.
>
> This testcase fails without the patch being proposed.
>
> Thanks,
> --
> Joel

Hi.  Thanks very much for the testcase!

I don't have much experience with abstract_origin.  I've read what I
can from DWARF4.pdf.
I can imagine that the bug is really in inherit_abstract_dies, and
thus the check to avoid re-processing the die belongs there.
Then we could maybe assert-fail in process_die if it's already being processed.

E.g., where inherit_abstract_dies has this:

            /* Found that ORIGIN_CHILD_DIE is really not referenced.  */
            process_die (origin_child_die, origin_cu);

add a check for origin_child_die->in_process here, and only call
process_die if it's zero,
and add a comment saying the check is to avoid the case of mutually
referenced abstract_origins.
And include a reference to a bug number because for me one high order
bit here is finding the testcase that exercises this check.

And then in process_die add at the start:

  gdb_assert (!die->in_process);

I don't have a strong opinion on which way to go though.
I *do* have a strong opinion on understanding *why* the code is
checking die->in_process.
If we go with the current patch, which is fine with me, though I'm
slightly leading towards the above instead but am happy to defer to
others, then I would replace this part of your patch:

+  /* Only process those not already in process.  */
+  if (die->in_process)
+    return;

with:

+  /* Only process those not already in process.  PR 12345.  */
+  if (die->in_process)
+    return;

And in either case file a bug that includes a description of the
problem (obviously :-)) and a reference to the testcase name.

Thoughts?