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

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

On 5/12/21 3:20 PM, Tom de Vries wrote:
> 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).

Well, that didn't take long...

I found this example (minimized from gdb.cp/shadow.cc):
...
$ cat shadow.cc
namespace A {
}

int
main()
{
  using namespace A;
  return 0;
}
$ g++-10 -g shadow.cc -flto -o shadow
$ ./gdb -q -batch ./shadow  -ex "b main"
Aborted (core dumped)
...

With patch reverted:
...
$ ./gdb -q -batch ./shadow  -ex "b main"
Breakpoint 1 at 0x4004ca: file
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.cp/shadow.cc, line 8.
$
...

This clearly is a regression, so I'm reverting this.

Thanks,
- Tom