From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id C39BF385AC21 for ; Thu, 25 Nov 2021 12:01:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C39BF385AC21 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-330-Kuji7yHjPcGqVJWcLtdnMw-1; Thu, 25 Nov 2021 07:01:50 -0500 X-MC-Unique: Kuji7yHjPcGqVJWcLtdnMw-1 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id C383A835E39 for ; Thu, 25 Nov 2021 12:01:48 +0000 (UTC) Received: from [10.97.116.34] (ovpn-116-34.gru2.redhat.com [10.97.116.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1094767842; Thu, 25 Nov 2021 12:01:46 +0000 (UTC) Message-ID: <44bc816d-bb02-4be8-b500-abfb10fa4a20@redhat.com> Date: Thu, 25 Nov 2021 09:01:42 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Subject: Re: [PATCH v2] PR gdb/28480: Improve ambiguous member detection To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <20211108182722.29510-1-blarsen@redhat.com> <20211122180011.GG2514@redhat.com> <20211124170933.GO2662946@redhat.com> From: Bruno Larsen In-Reply-To: <20211124170933.GO2662946@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-15.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Nov 2021 12:01:54 -0000 On 11/24/21 14:09, Andrew Burgess wrote: > * Bruno Larsen via Gdb-patches [2021-11-22 15:35:29 -0300]: > >> Hi Andrew, >> >> Thanks for the review. All of the easy addressable comments will be changed. >> >> On 11/22/21 15:00, Andrew Burgess wrote: >>> * Bruno Larsen via Gdb-patches [2021-11-08 15:27:22 -0300]: >>> >> >>>> diff --git a/gdb/valops.c b/gdb/valops.c >>>> index 9787cdbb513..2989a93df1a 100644 >>>> --- a/gdb/valops.c >>>> +++ b/gdb/valops.c >>>> @@ -1962,6 +1962,33 @@ struct_field_searcher::update_result (struct value *v, LONGEST boffset) >>>> space. */ >>>> if (m_fields.empty () || m_last_boffset != boffset) >>>> m_fields.push_back ({m_struct_path, v}); >>>> + else >>>> + /* Some fields may occupy the same space and still be ambiguous. >>>> + This happens when [[no_unique_address]] is used by a member >>>> + of the class. We assume that this only happens when the types are >>>> + different. This is not necessarily complete, but a situation where >>>> + this assumption is incorrect is currently (2021) impossible. */ >>> >>> This comment should be moved inside the "{ ... }" block. >>> >>> I found this comment difficult to understand. When you say "...when >>> the types are different", I guess this is referring to the path check >>> below maybe? In which case I wonder if we can find a different way to >>> phrase this, rather than "different types" ... "paths to the two >>> fields are different" maybe? >>> >>> Additional the whole final sentence just leaves me confused, it seems >>> to hint that there is a situation not covered by this code "This is >>> not necessarily complete...", but also that there is no such situation >>> "... is currently impossible". >>> >>> I wonder if you are saying that should we ever have two fields of the >>> same name, in the same class, that occur at the same address, then >>> this code wouldn't cover that case? But that seems a pretty weird >>> thing to worry about, so I assume I'm not understand you correctly. >>> >>> Could you rephrase the last part please? >> >> How does the following sound: >> >> Some fields may occupy the same space and still be ambiguous. This >> happens when [[no_unique_address]] is used in the inferior's >> code. The current solution assumes that the compiler will only place >> 2 struct members in the same location if they are of different >> types. As of 2021, this is mandatory, but this may change in the >> future > > I agree with you about what the standard says, but what I'm not > understanding is why that's relevant here. We're looking up something > by name, right? And we're trying to handle the case where two things > might exist at the same address _and_ have the same name. > > So in this check: > > if(field.path.back () != m_struct_path.back ()) > ... > else > ... > > We get to the if block when two things have the same address, but > their containing classes are different, in your original example A::e > and B::e, the 'A' and 'B' are different. > > We get to the else block when two things have the same address, but > their containing classes are the same, so this might be A::e and A::e, > which I'm pretty sure can only happen when virtual inheritance is in > use, right?> > But my point is, where in this logic do we check the type of 'e'? I > don't see it. If we imagine a world where A::e and B::e were the same > type, and placed at the same address, I think the existing code would > be fine. Good catch. I thought that field.path included the type of 'e', but reviewing the code, you're right in saying that it doesn't. However, as far as I understand, this is still fine, seeing as otherwise, we'd have two different A classes in the same address, which is also blocked by the C++ standard. > > My claim then, is that the following describes your code: > > Fields can occupy the same space and have the same name (be > ambiguous). This can happen when fields in two different base > classes are marked [[no_unique_address]] and have the same name. > The C++ standard says that such fields can only occupy the same > space if they are of different type, but we don't rely on that in > the following code. I feel like this is a good description, I'll add it to the code and push it. Thanks again for the review! > > If I'm not understanding something, please do let me know. > > Thanks, > Andrew > -- Cheers! Bruno Larsen