Re: [PATCH][gdb/symtab] Fix infinite recursion in dwarf2_cu::get_builder()

[Tom de Vries <tdevries@suse.de>]

On 5/6/21 5:23 PM, Simon Marchi wrote:
> On 2021-05-06 8:02 a.m., Tom de Vries wrote:
>> Hi,
>>
>> With the test-case attached in PR26327, gdb aborts:
>> ...
>> $ gdb -q -batch 447.dealII -ex "b main"
>> Aborted (core dumped)
>> ...
>> when running out of stack due to infinite recursion:
>> ...
>>  #8  0x00000000006aaba6 in dwarf2_cu::get_builder (this=0x35e4b40)
>>      at src/gdb/dwarf2/read.c:700
>>  #9  0x00000000006aaba6 in dwarf2_cu::get_builder (this=0x22ee2c0)
>>      at src/gdb/dwarf2/read.c:700
>>  #10 0x00000000006aaba6 in dwarf2_cu::get_builder (this=0x35e4b40)
>>      at src/gdb/dwarf2/read.c:700
>>  #11 0x00000000006aaba6 in dwarf2_cu::get_builder (this=0x22ee2c0)
>>      at src/gdb/dwarf2/read.c:700
>> ...
>>
>> We're recursing in this code in dwarf2_cu::get_builder():
>> ...
>>      /* Otherwise, search ancestors for a valid builder.  */
>>      if (ancestor != nullptr)
>>        return ancestor->get_builder ();
>> ...
>> due to the fact that the ancestor chain is a cycle.
>>
>> Higher up in the call stack, we find some code that is responsible for
>> triggering this, in new_symbol:
>> ...
>>        case DW_TAG_formal_parameter:
>>          {
>>            /* If we are inside a function, mark this as an argument.  If
>>               not, we might be looking at an argument to an inlined function
>>               when we do not have enough information to show inlined frames;
>>               pretend it's a local variable in that case so that the user can
>>               still see it.  */
>>            struct context_stack *curr
>>              = cu->get_builder ()->get_current_context_stack ();
>>            if (curr != nullptr && curr->name != nullptr)
>>              SYMBOL_IS_ARGUMENT (sym) = 1;
>> ...
>>
>> This is code that was added to support pre-4.1 gcc, to be able to show
>> arguments of inlined functions as locals, in the absense of sufficiently
>> correct debug information.
>>
>> Removing this code (that is, doing SYMBOL_IS_ARGUMENT (sym) = 1
>> unconditially), fixes the crash.  The ancestor variable also seems to have
>> been added specifically to deal with fallout from this code, so remove that as
>> well.
>>
>> Tested on x86_64-linux:
>> - openSUSE Leap 15.2 with gcc 7.5.0, and
>> - openSUSE Tumbleweed with gcc 10.3.0.
>>
>> Any comments?
> 
> I did not study the problem in depth like you did, but based on your
> explanation I think this is reasonable.  If support for ancient stuff
> gets in the way of supporting modern stuff (like LTO), then it makes
> sense to remove the support for the ancient stuff.

I committed this, but now realized that the cases I was actually trying
to fix: gcc-10 -flto code, aren't fixed, they just changed failure mode
from hang to abort.

So it looks like I did a point fix which just fixes the 447.dealII
test-case, not the generic case.

I may revert this patch (although atm I don't see the immediate need,
given that all the examples I looked at sofar also were problematic
before this patch).

Thanks,
- Tom