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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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.

Simon