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

[Joel Brobecker <brobecker at adacore dot com>]

Hi Doug,

> Hi.  Thanks very much for the testcase!

You are very welcome. Surprisingly, it took me a 2-3 hours to reduce
it down. I tried using the DWARF assembler, but to no avail, so had
to work from an asm file instead. We might be able to reproduce
that asm file using the DWARF assembler, but I don't want to spend
any more time on the testcase, unless I really have to.

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

Same here.

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

I agree with your all you suggestions.

manjian2006@gmail.com, do you want to take care of this, or would you
like me to? This involves first creating a PR, change your patch
according to Doug's suggestion, re-test, and re-submit.

-- 
Joel