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

[lin zuojian <manjian2006 at gmail dot com>]

Hi Joel,
I am not really familiar with these routines,please help me with that.
Thank you.
(I carelessly used the company email in the last reply,sorry).

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