From patchwork Tue Oct 22 22:23:07 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Buettner X-Patchwork-Id: 35238 Received: (qmail 94872 invoked by alias); 22 Oct 2019 22:23:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 94864 invoked by uid 89); 22 Oct 2019 22:23:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=H*f:sk:8b30120, computed, CUs, H*f:sk:8c07368 X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 22 Oct 2019 22:23:23 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1571783001; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fxDInODBIhQPcr9M1Yva46B+3h2yn5zsL6SrRoXPwaE=; b=YIqrjcvCcCzqZafkavIdfOLXAShayFdKJ0dPSSWsHEyQgo7A8luCyeKWhRBRKLBOVoTY7e xUlyzNeW/icQvIc+Fr8wETYXILyjB0kHbqmBLubIPYdAJ9SLgm398ywqCsA1BEMsiU1HlN z1Pv55gLTPdtjKLAsHluHHk5fgkM2Pk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-192-J9H2XdwiPqWGjFrf0bu8bA-1; Tue, 22 Oct 2019 18:23:09 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9BC1F800D53; Tue, 22 Oct 2019 22:23:08 +0000 (UTC) Received: from f29-4.lan (ovpn-116-114.phx2.redhat.com [10.3.116.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 64B94194BB; Tue, 22 Oct 2019 22:23:08 +0000 (UTC) Date: Tue, 22 Oct 2019 15:23:07 -0700 From: Kevin Buettner To: Simon Marchi Cc: gdb-patches@sourceware.org, Keith Seitz Subject: Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Message-ID: <20191022152307.37dea0d1@f29-4.lan> In-Reply-To: <8b301205-e8f6-ead5-0484-eacfb82e5b7a@polymtl.ca> References: <20191014001842.27413-1-kevinb@redhat.com> <20191014001842.27413-2-kevinb@redhat.com> <8c073683bc0b0a8e7918fdfb346cbb03@polymtl.ca> <20191015092743.7037f47c@f29-4.lan> <20191017180851.7db69958@f29-4.lan> <8b301205-e8f6-ead5-0484-eacfb82e5b7a@polymtl.ca> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-IsSubscribed: yes On Fri, 18 Oct 2019 11:07:31 -0400 Simon Marchi wrote: > > I looked over your patch; it looks reasonable to me. If we go that > > route, I like the idea of introducing a dwarf2_cu_processing_context > > struct. (But see below for some later misgivings that I have/had.) > > Ok, I can try to make something cleaner, but I don't know when it > would be ready, and I wouldn't want that to block the GDB 9.1 branch > creation (or release) for that. Would you like to still push your > patch (or a perhaps updated version of it) so that we have the fix > in GDB 9.1? [...] > > There may be other concerns too; I'm certain that I didn't look at all > > of the ways that CU is used in dwarf2_physname and its callees. > > I don't think it's humanly possible to manually check all the > possible branches this code can take. I say, let's do a quick pass > to check for the obvious (like what you found above), but otherwise > I'm fine with this patch, it already makes things better than they > are now. My testing shows that the patch below still fixes the problem while also avoiding the poential problems of passing a CU to compute_delayed_physnames() which is different from the CU of the methods for which want to compute physnames. I think that this patch is safer than the one I originally proposed and is, therefore, a better short term solution. What do you think? - - - Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs This is a fix for BZ 25065. GDB segfaults when running either gdb.cp/subtypes.exp or gdb.cp/local.exp in conjunction with using the -flto compiler/linker flag. A much simpler program, which was used to help create the test for this fix, is: -- doit.cc -- int main() { class Foo { public: int doit () { return 0; } }; Foo foo; return foo.doit (); } -- end doit.cc -- gcc -o doit -flto -g doit.cc gdb -q doit Reading symbols from doit... (gdb) ptype main::Foo type = class Foo { Segmentation fault (core dumped) The segfault occurs due to a NULL physname in c_type_print_base_struct_union in c-typeprint.c. Specifically, calling is_constructor_name() eventually causes the SIGSEGV is this code in c-typeprint.c: const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j); int is_full_physname_constructor = TYPE_FN_FIELD_CONSTRUCTOR (f, j) || is_constructor_name (physname) || is_destructor_name (physname) || method_name[0] == '~'; However, looking at compute_delayed_physnames(), we see that the TYPE_FN_FIELD_PHYSNAME field should never be NULL. This field will be set to "" for NULL physnames: physname = dwarf2_physname (mi.name, mi.die, cu); TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index) = physname ? physname : ""; For this particular case, it turns out that compute_delayed_physnames wasn't being called, which left TYPE_FN_FIELD_PHYSNAME set to the NULL value that it started with when that data structure was allocated. The place to fix it, I think, is towards the end of inherit_abstract_dies(). My first attempt at fix caused the origin CU's method_list (which is simply the list of methods whose physnames still need to be computed) to be added to the CU which is doing the inheriting. One drawback with this approach is that compute_delayed_physnames is (eventually) called with a CU that's different than the CU in which the methods were found. It's not clear whether this will cause problems or not. A safer approach, which is what I ultimately settled on, is to call compute_delayed_physnames() from inherit_abstract_dies(). One potential drawback is that all needed types might not be known at that point. However, in my testing, I haven't seen a problem along these lines. gdb/ChangeLog: * dwarf2read.c (inherit_abstract_dies): Ensure that delayed physnames are computed for inherited DIEs. Change-Id: I6c6ffe96b301a9daab9f653956b89e3a33fa9445 --- gdb/dwarf2read.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index ee9df34ed2..976153640a 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -13666,6 +13666,9 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu) origin_child_die = sibling_die (origin_child_die); } origin_cu->list_in_scope = origin_previous_list_in_scope; + + if (cu != origin_cu) + compute_delayed_physnames (origin_cu); } static void