From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92451 invoked by alias); 8 Jan 2019 19:56:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 92431 invoked by uid 89); 8 Jan 2019 19:56:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=1,000, Keith, Missing, gdbpatches X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Jan 2019 19:56:10 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2CF01C0C6C09; Tue, 8 Jan 2019 19:56:09 +0000 (UTC) Received: from theo.uglyboxes.com (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 359B7620CE; Tue, 8 Jan 2019 19:56:01 +0000 (UTC) Subject: Re: [RFC PATCH] Introduce dwarf2_cu::get_builder To: Tom Tromey Cc: gdb-patches@sourceware.org References: <20181023185715.23082-1-keiths@redhat.com> <87a7lqmhpg.fsf@tromey.com> From: Keith Seitz Message-ID: Date: Tue, 08 Jan 2019 19:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <87a7lqmhpg.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00151.txt.bz2 On 11/30/18 12:30 PM, Tom Tromey wrote: >>>>>> "Keith" == Keith Seitz 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 builder; Keith> + std::unique_ptr 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