Re: [PATCH] Prevent inline function parameters from crashing the DWARF reader

[Keith Seitz <keiths@redhat.com>]

On 10/16/18 3:15 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> However, as demonstrated in this Fedora bugzilla:
> Keith>   https://bugzilla.redhat.com/show_bug.cgi?id=1638798
> 
> Keith> the abstract_origin of the parameter DIE may be in an entirely different
> Keith> CU.  This CU will have no builder defined for it, so the call to
> Keith> get_current_context_stack is made on an uninitialized builder, and GDB
> Keith> segfaults.
> 
> I was curious what happened before, so I checked out the commit
> before 804d27291b2922f01fe5cc2d9ae759533d4a2f6f, and ran it on the
> executable in that bug.
> 
> I set a breakpoint conditional on curr==0 and it never hit.
> 

What ends up happening, IIRC, is that we simply only ever checked (previously)
the depth level. It was 0.

> Now, I am not totally sure that this was correct before, but I suppose
> what was going on was that the earlier code was installing these symbols
> into the referencing CU's symtab.

Yeah, I was wondering about something like that, too, but had convinced myself
temporarily that this was appropriate. The immediate problem is that the
formal_parameter DIE lives in a partial_unit in its own CU.

After troubleshooting other similar problems, I'm now not so sure. [I
was coming around to another (better?) solution, see below.]

> Abstractly, at least, that seems reasonable.  So maybe that approach
> should be taken again.
> 
> Also, when I look at new_symbol, I see a bunch of uses of cu->builder,
> so I wonder if there are ways to make those spots crash as well.
> So maybe this indicates some different approach is needed as well.

It's not just new_symbol. It happens in other places, too, like
read_import_statement (where it calls using_directives).

> Sorry about this.  I didn't anticipate this at all.  I can try to take
> it over if you'd like.

So far, I'm still investigating, but from the Fedora side -- which seems
like it may be in really bad shape right now for some reason.

> Maybe one idea would be to temporarily set the referenced CU's builder
> to the referencing CU's builder somehow.

Yeah, that is an approach not too far off (I think?) from what I was
attempting: When we "import" DIEs from other CUs, I add them to an
sort of CU inheritance chain. Then we crawl up the chain looking for the
last valid builder. [This can happen frequently with DW_TAG_partial_unit
DIEs in the debuginfo.]

If I need more help, I will certainly let you know.

Let's consider this patch shelved for the moment.

Keith