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

Tom de Vries
2021-05-12T13:21:02Z

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