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.129.124]) by sourceware.org (Postfix) with ESMTPS id 47C4E3858C27 for ; Wed, 24 Nov 2021 17:09:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 47C4E3858C27 Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-217-MFEk3uJHNVqwgIywdtgrwg-1; Wed, 24 Nov 2021 12:09:36 -0500 X-MC-Unique: MFEk3uJHNVqwgIywdtgrwg-1 Received: by mail-wr1-f72.google.com with SMTP id y4-20020adfd084000000b00186b16950f3so648772wrh.14 for ; Wed, 24 Nov 2021 09:09:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ogpg1xBNeD/1Q+5B5ZMUYGmm4IHb8HYGOpAWl62pptw=; b=V3Ed5s0dZ/Fs/4lopd4gYau6vyC1Fg3LivnzgIB4M5uhEDiBFF5c+rhkcghgZIAQ/X wGod8N/r3mmw+jhIsm8G5MmdG1hD7HP+63h6XgjJgHBToWPhs2hosrxX3xZYODJjOepm o2fTihtJyBsSnd0KKbQQvcZnL7OeRA1+nnXYu0zSRBepIednHK2fHuTnue1N8g8/ID8Y Y//BPWqd0IrRR40G9YQ0diEP/HgUB32QFhQregkXAe0ewMbdh7vGYkmApLd5Mo6W7to+ 2VsySJ0Tkg2gAcTG+7OZztr2KvpvklJ6Xy4R8R+WOMY9vZqjH2u/qMdehtYQD51ySzz/ YNEQ== X-Gm-Message-State: AOAM533I+xnBkDWbvsbl6GkBgwd2KIQNN5jLc5DDwCAw2KGXsse//bSO 7bFCJ8jZQbZgBsGG0DTI6JN4FrF0mo1VYU+k+l+WYRJzwa2EbSlvkeA6/Qh3FXw36g/Fj5NvvTH VFhdsXPd8ZOTZq/4NYZmRFA== X-Received: by 2002:a05:600c:6016:: with SMTP id az22mr17484105wmb.11.1637773775423; Wed, 24 Nov 2021 09:09:35 -0800 (PST) X-Google-Smtp-Source: ABdhPJwnFrTt6XYj7X/8hJUoxPJ9GAtwagKg1jXqCkgsOD+Oq7txi+HhI30QTZ5eQWrdG/3Op2xiow== X-Received: by 2002:a05:600c:6016:: with SMTP id az22mr17484053wmb.11.1637773775135; Wed, 24 Nov 2021 09:09:35 -0800 (PST) Received: from localhost (host86-166-129-255.range86-166.btcentralplus.com. [86.166.129.255]) by smtp.gmail.com with ESMTPSA id j134sm307265wmj.3.2021.11.24.09.09.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Nov 2021 09:09:34 -0800 (PST) Date: Wed, 24 Nov 2021 17:09:33 +0000 From: Andrew Burgess To: Bruno Larsen Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] PR gdb/28480: Improve ambiguous member detection Message-ID: <20211124170933.GO2662946@redhat.com> References: <20211108182722.29510-1-blarsen@redhat.com> <20211122180011.GG2514@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 16:31:36 up 5 days, 5:30, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Wed, 24 Nov 2021 17:09:39 -0000 * 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. 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. If I'm not understanding something, please do let me know. Thanks, Andrew