From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47704 invoked by alias); 18 Oct 2019 01:08:55 -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 47695 invoked by uid 89); 18 Oct 2019 01:08:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= 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; Fri, 18 Oct 2019 01:08:53 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 377A5C01092E; Fri, 18 Oct 2019 01:08:52 +0000 (UTC) Received: from f29-4.lan (ovpn-118-26.phx2.redhat.com [10.3.118.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0DC501001B33; Fri, 18 Oct 2019 01:08:52 +0000 (UTC) Date: Fri, 18 Oct 2019 01:08:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Message-ID: <20191017180851.7db69958@f29-4.lan> In-Reply-To: References: <20191014001842.27413-1-kevinb@redhat.com> <20191014001842.27413-2-kevinb@redhat.com> <8c073683bc0b0a8e7918fdfb346cbb03@polymtl.ca> <20191015092743.7037f47c@f29-4.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00616.txt.bz2 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.) > 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. 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. 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. 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. 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...) Kevin