Re: [RFC PATCH] Introduce dwarf2_cu::get_builder

[Keith Seitz <keiths@redhat.com>]

On 11/30/18 12:30 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> This patch is an attempt to deal with a variety of bugs reported where
> Keith> GDB segfaults attempting to access a dwarf2_cu's builder.  In certain
> Keith> circumstances, this builder can be NULL.
> 
> I think it would be good to describe one such case in the commit
> message.  I suppose there must be some spot that works on an inherited
> DIE in the context of its own CU?  I found myself wondering where.

I've changed the commit log to `hint' at a common pathway that this occurs
with a further suggestion to see either the test or the PR for a concrete
example:

    ... In certain
    circumstances, this builder can be NULL.  This is especially common
    when inheriting DIEs via inlined subroutines in other CUs.  The test
    case demonstrates one such situation reported by users.  See gdb/23773,
    rhbz1638798, and dups for other concrete examples.

That's my "picture is worth 1,000 words" description. Would that be sufficient?

> Keith> I'm submitting this as an RFC rather than an actual patch because of the
> Keith> lack of coverage testing for all the places where get_builder() is used.
> 
> I wouldn't be surprised if some of them aren't really reachable in the
> sense of ever being able to take the ancestor path.  I'm not concerned
> about needing tests for every single point.

Great.

> Keith> +# This tests a segfault that occurs when reading inline_subroutine DIEs
> 
> inlined_subroutine (missing the "d")

Fixed.
 
> Keith> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have
> 
> Missing an "l" in bugzilla.

And fixed.

> The rest seems good to me.  Thank you for the patch, and for reducing
> and rewriting the test case like this.

Thank you for the review!

To save additional messages, I'll reply to the other "official" submission
message (https://sourceware.org/ml/gdb-patches/2018-11/msg00450.html) here:

Keith> +private:
Keith>    /* The symtab builder for this CU.  This is only non-NULL when full
Keith>       symbols are being read.  */
Keith> -  std::unique_ptr<buildsym_compunit> builder;
Keith> +  std::unique_ptr<buildsym_compunit> m_builder;

> Good idea, thanks for doing this.
>
> I think the previous nits still apply; though this email didn't include
> the test so I don't know for certain about those.

The test is unchanged, I simply forgot to include them (for $uknown reasons).

Keith