From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66336 invoked by alias); 17 Oct 2019 03:54:45 -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 66328 invoked by uid 89); 17 Oct 2019 03:54:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=kevin, Bs, Kevin, CUs 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; Thu, 17 Oct 2019 03:54: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 x9H3sXUc024437 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 16 Oct 2019 23:54:38 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca x9H3sXUc024437 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1571284479; bh=cwiL3GaEr+NPuD6Bmas/HwdpQ12BrwMcx0RIFQGAOc4=; h=Subject:To:References:From:Date:In-Reply-To:From; b=uauqaRAx9WJVDIQoFV3yF921aMn8qEFbLxdaliz5YxloQ/USSfM3G5Dvq7pEj6eKl 7gkOyXJtQgn881dyxoOFLwmkWbCiSOld2OwRxWP0ztEAAoclrlIx/CFLGz1ouUBHD3 JTQzrInAEHdtP80S3uxX6gOJdUF2dl7P0LV7Xa0g= Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 20C8A1E059; Wed, 16 Oct 2019 23:54:33 -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 References: <20191014001842.27413-1-kevinb@redhat.com> <20191014001842.27413-2-kevinb@redhat.com> <8c073683bc0b0a8e7918fdfb346cbb03@polymtl.ca> <20191015092743.7037f47c@f29-4.lan> From: Simon Marchi Message-ID: Date: Thu, 17 Oct 2019 03:54:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: <20191015092743.7037f47c@f29-4.lan> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00547.txt.bz2 On 2019-10-15 12:27 p.m., Kevin Buettner wrote: > Hi Simon, > > You raise some good questions. I modified the test associated with > this bug so that two different CUs attempt to inherit a third CU. > This has turned up another bug in GDB which I spent the rest of the > day looking at. (GDB goes into an infinite loop!) > > I'll make the following observations, however... > > - method_list is used solely for keeping track of C++ methods for > delayed physname computations. > > - method_list is cleared in process_full_comp_unit(), > process_full_type_unit(), and compute_delayed_physnames(). > That latter function(), compute_delayed_physnames(), is called > after DIE processing in the first two functions. So method_list > is made to be empty prior to DIE processing and then made empty > (as a result of delayed physname computation) again after DIE > processing (in the above mentioned functions). > > So, what is the right thing to do with regard to method_list for > inherit_abstract_dies()? > > Yesterday, as part of my investigations, I made inherit_abstract_dies() > call compute_delayed_physnames in place of the code in my posted > patch. That also works, at least for my test case. I'll note that > for my original (posted) patch, compute_delayed_physnames will be > called with the inheriting CU. In the alternate approach (in which > compute_delayed_physnames is called from inherit_abstract_dies), > compute_delayed_physnames is called with the origin CU. I don't > yet know which is more correct or if there are cases where it'll > make a difference. > > So... I'm continuing my investigations and will let you know when > I have some answers. In the interim, if anyone has some insights > about these matters, I'd very much like to hear them. > > Kevin > Hi Kevin, I've spent a bit of time to better understand what inherited abstract DIEs are and to step in that code. I observed that when a DIE A inherits from another DIE X, we process DIE X completely from scratch, going through process_die, creating the struct type instances, adding items to method_list for delayed name computation. If another DIE B inherits from DIE X, then we'll go through process_die again, creating another struct type, adding items to method_list for delayed name computation again. There is no (and there shouldn't be) any state shared between the processing of X while inherited by A and the processing of X while inherited by B. In theory, the compiler could have decided to have two completely separate DIE trees under A and under B. But to reduce duplication, it decided to use that abstract origin thing, so A and B could share some children. However, we should still conceptually treat them as separate trees. Btw, I'm writing all this so I assimilate it better, but also so you can tell me if you think I'm wrong. I am confident that what you do in this patch is right. Let's say A, B and X, the same DIEs as above, are all in different CUs. Before your patch, while processing A, the delayed method info for things described in X would be put in X's CU's method_info list and stay there, and it would never get used. When processing B, the delayed method info for things described in X would also be put in X's CU's method_info list and never get used. If, we then happen to process the CU that contains X, we'll start by clearing that CU's method_info list in process_full_comp_unit and start from scratch. With your patch, the entries generated while parsing X as inherited by A get moved to A's CU, and the entries generated while parsing X as inherited by B get transferred to B's CU. That's as if A and B had their own independent subtree, which is what we want. If we then happen to process X's CU, that CU's method_list will be empty, as expected. 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. Also, note this in inherit_abstract_dies: /* We're inheriting ORIGIN's children into the scope we'd put DIE's symbols in. */ origin_previous_list_in_scope = origin_cu->list_in_scope; origin_cu->list_in_scope = cu->list_in_scope; ... origin_cu->list_in_scope = origin_previous_list_in_scope; I think this code is solving the same kind of problem for `list_in_scope`, but differently: it temporarily moves A's CU's list in X's CU. Here too, I think it would be clearer if `list_in_scope` wasn't attached to a CU, but something passed down the stack. Again, we wouldn't have to do anything special in inherit_abstract_dies, just pass that list down. One last thing, I think that those method_list.clear() calls in process_full_comp_unit and process_full_type_unit are unnecessary, and are confusing because they suggest that there might be something in there. When we start processing a CU with either of these functions, I don't think there should ever be some content. I'd try to change them to gdb_assert (cu->method_list.empty ()); Simon