From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 112840 invoked by alias); 18 Oct 2019 15:07:46 -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 112832 invoked by uid 89); 18 Oct 2019 15:07:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.1 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=Buettner, buettner X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 18 Oct 2019 15:07:43 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id x9IF7WDP016887 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 18 Oct 2019 11:07:37 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca x9IF7WDP016887 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1571411258; bh=T+p0gPQQ6T5XPcCbXnh6o8ziyp/mUOqKeH6lnRJh6bA=; h=Subject:To:References:From:Date:In-Reply-To:From; b=uADkwvgFeoE8G06YrwJEFRzg1A+r3XVpiCFCcYxbYAl9MwOtXqGcpYpBhr3o/xV0V u2uqmhpmZut8uxIXVrCdpb7ayGbAxw0Kaw0Vx7RDnC7+M5UHGxbh2V1R7+Q3GLcp3n JV/ouSikeGpcLwwpmBNMXCRXmFLuLKBiUU9A/yKs= Received: from [172.16.0.148] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 389F11E508; Fri, 18 Oct 2019 11:07:32 -0400 (EDT) Subject: Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs To: Kevin Buettner , gdb-patches@sourceware.org, Keith Seitz 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> From: Simon Marchi Message-ID: <8b301205-e8f6-ead5-0484-eacfb82e5b7a@polymtl.ca> Date: Fri, 18 Oct 2019 15:07:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20191017180851.7db69958@f29-4.lan> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00636.txt.bz2 On 2019-10-17 9:08 p.m., Kevin Buettner wrote: > On Thu, 17 Oct 2019 01:30:12 -0400 > Simon Marchi wrote: > >> On 2019-10-16 11:54 p.m., Simon Marchi wrote: >>> I think that what's confusing in all this is the fact that the >>> method_info list is currently attached to a particular CU. >>> Instead, I think it should be attached to the operation of >>> processing of a CU, and used to collect all delayed method infos >>> while processing that CU, even if some of these infos come from >>> inherited DIEs from another CU. Concretely, it would mean to have >>> a local instance of std::vector in >>> process_full_comp_unit/process_full_type_unit and to pass it by >>> pointer/reference through the call stack to any code who might >>> want to append to it. We wouldn't have to do anything special in >>> inherit_abstract_dies, just pass this reference to the list down >>> the stack. I don't know how feasible it would be in practice to >>> do that change, maybe it's too much work or would end up ugly. >>> I'll give it a try. But your patch gives essentially the same >>> result, and works with what we have today. >> >> A little follow up to the above. >> >> I prototyped that change here in a WIP patch (so, not intended to be >> reviewed): >> >> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/128 >> >> I got no regression in gdb.dwarf2. However, it's a bit invasive. >> If we want to pass other objects in the same fashion, it will >> quickly become very heavy. >> >> What we could do though, is to introduce a new type (e.g. struct >> dwarf2_cu_processing_context) and pass that around instead. Its >> lifetime would be the duration of process_full_comp_unit / >> process_full_type_unit, just like std::vector in the patch above, >> but could contain many fields. > > 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? >> I found something potentially problematic though (applies to both >> your and my patch). When we process the delayed_method_info objects >> in compute_delayed_physnames, we call: >> >> dwarf2_physname (mi.name, mi.die, cu); >> >> mi.die could be a die coming from X's CU (to keep the example from >> my previous message), but the cu in the call above is A's CU (the CU >> we are processing). I am pretty sure that this function (and what >> it calls) expect the passed DIE to come from the passed CU. If they >> don't match, I guess we could have some bad surprises. > > I spent a little time trying to figure out what CU is used for in > dwarf2_physname() and its callees. What I noticed most is that > cu->language is used to figure out some appropriate thing to do. > I seem to remember Keith running into problems with mismatched > languages in different CUs. So there might be problems if the > languages associated with the CUs are different. Yeah, I remember this saga, but I did not really follow it. Keith, would you have an idea of what would be the right CU to pass here? > There are also obstacks associated with each CU. I noticed this > call in dwarf2_compute_name(): > > dwarf2_const_value_attr (attr, type, name, > &cu->comp_unit_obstack, cu, > &value, &bytes, &baton); > > For this matter, I think that we want the inheriting CU's obstack to > be used, so this might be okay. However, due to potentially differing > lifetimes, there could be problems if data structures allocated in one > obstack point to data structures in another; I don't know if that could > happen or not. Hmm right. Maybe it's fine in practice since the lifetimes of the two dwarf_cu objects are probably similar. But I agree it's not ideal. > > There are also errors which fetch the objfile name via... > > objfile_name (cu->per_cu->dwarf2_per_objfile->objfile) > > I don't think this will cause a crash or anything, but it has > the potential to misidentify the problematic objfile. Aren't the two CUs necessarily in the same objfile? > 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. > Also, while looking at all of that, it occurred to me that struct > dwarf2_cu already contains elements of a CU processing context. E.g. > the rust_unions member looks very similar to method_list. Plus the > comment for dwarf2_cu says "Internal state when decoding a particular > compilation unit." That led me to wonder if it truly makes sense to > pass around both a struct dwarf2_cu and a struct > dwarf2_cu_processing_context. (I don't know the answer, but it's > something to mull over...) I'll check closer when actually trying to write that patch, but for now I believe that it's fine to pass both objects, as they have different meanings. The dwarf2_cu_processing_ctx would contain things related to the root CU we are parsing. The dwarf2_cu would represent things related to the CU we are in right now. So when crossing a CU boundary (like in the abstract origin case that started this thread), the passed dwarf2_cu_processing_ctx wouldn't change, but the passed dwarf2_cu would change. Simon