From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 203083858D28 for ; Fri, 5 Nov 2021 17:54:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 203083858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (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 BB14B1ECEB; Fri, 5 Nov 2021 13:54:13 -0400 (EDT) Subject: Re: [PATCH] PR gdb/28480: Improve ambiguous member detection To: Bruno Larsen , gdb-patches@sourceware.org References: <20211104210457.35512-1-blarsen@redhat.com> From: Simon Marchi Message-ID: <0e7f3dbb-ef25-9d60-42da-b01c57eb8efa@simark.ca> Date: Fri, 5 Nov 2021 13:54:13 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20211104210457.35512-1-blarsen@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, 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: Fri, 05 Nov 2021 17:54:17 -0000 On 2021-11-04 5:04 p.m., Bruno Larsen via Gdb-patches wrote: > Basic ambiguity detection assumes that when 2 fields with the same name > have the same boffset, it must be an unambiguous request. This is not > always correct. Consider the following code: > > class empty { }; > > class A { > public: > [[no_unique_address]] empty e; > }; > > class B { > public: > int e; > }; > > class C: public A, public B { }; > > if we tried to use c.e in code, the compiler would warn of an ambiguity, > however, since A::e does not demand an unique address, it gets the same > address (and thus boffset) of the members, making A::e and B::e have the > same address. however, "print c.e" would fail to report the ambiguity, > and would instead print it as an empty class (first path found). > > The new code solves this by checking for other found_fields that have > different m_struct_path.back() (final class that the member was found > in), despite having the same boffset. > > The testcase gdb.cp/ambiguous.exp was also changed to test for this > behavior. > --- > gdb/testsuite/gdb.cp/ambiguous.cc | 13 +++++++++++++ > gdb/testsuite/gdb.cp/ambiguous.exp | 7 +++++++ > gdb/valops.c | 26 ++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/gdb/testsuite/gdb.cp/ambiguous.cc b/gdb/testsuite/gdb.cp/ambiguous.cc > index a55686547f2..b2be7297b28 100644 > --- a/gdb/testsuite/gdb.cp/ambiguous.cc > +++ b/gdb/testsuite/gdb.cp/ambiguous.cc > @@ -1,3 +1,4 @@ > +class empty { }; > > class A1 { > public: > @@ -17,6 +18,11 @@ public: > int y; > }; > > +class A4 { > +public: > + [[no_unique_address]] empty x; In theory, no_unique_address appeared in C++20. Having [[no_unique_address]] in the source code means that compiling this file with an older compiler that doesn't know about it, or in a standard mode where it isn't recognized, will lead to the entire test being skipped. In practice, it looks like older-but-not-too-old g++s cope well with it. For example, g++ 7 prints a warning: $ g++-7 ambiguous.cc ambiguous.cc:23:33: warning: ‘no_unique_address’ attribute directive ignored [-Wattributes] [[no_unique_address]] empty x; ^ ... but since we disable all warnings in that tests for g++ < 10, then it still compiles. The no_unique_address attribute has no effect, but that's fine. With older g++s, like 4.8, it fails though: $ make check TESTS="gdb.cp/ambiguous.exp" RUNTESTFLAGS='CC_FOR_TARGET=gcc-4.8 CXX_FOR_TARGET=g++-4.8' ... Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/ambiguous.exp ... Gdb compile failed, /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/ambiguous.cc:23:5: error: expected unqualified-id before '[' token [[no_unique_address]] empty x; ^ I don't know if others care about this test running with older compilers, but if so we could make that case optional with some preprocessor conditionals. > diff --git a/gdb/testsuite/gdb.cp/ambiguous.exp b/gdb/testsuite/gdb.cp/ambiguous.exp > index 008898c5818..68b82d45b68 100644 > --- a/gdb/testsuite/gdb.cp/ambiguous.exp > +++ b/gdb/testsuite/gdb.cp/ambiguous.exp > @@ -264,3 +264,10 @@ gdb_test "print (A1)(KV)jva1" " = \{x = 3, y = 4\}" > # JVA1V is derived from A1; A1 is a virtual base indirectly > # and also directly; must not report ambiguity when a JVA1V is cast to an A1. > gdb_test "print (A1)jva1v" " = {x = 1, y = 2}" > + > +#unique_ptr is a weird edge-case that interacts differently with the > +#ambiguity detection, so we should test it directly > +test_ambiguous "je.x" "x" "JE" { > + "'int A1::x' (JE -> A1)" > + "'empty A4::x' (JE -> A4)" > +} I think the comment needs to be re-worded to not talk about unique_ptr, but talk about ambiguous fields that have the same address due to no_unique_address. Make sure to use a space at the beginning of the line and a period at the end (like the other comments). > diff --git a/gdb/valops.c b/gdb/valops.c > index 9787cdbb513..75b732af62b 100644 > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -1962,6 +1962,32 @@ 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 Two spaces after period. > + different. This is not necessarily complete, but a situation where > + this assumption is incorrect is unlikely*/ Period and two spaces: ... unlikely. */ > + { > + bool ambiguous = false, insert = true; > + for(auto finds: m_fields){ Space before opening parentheses and after closing. Iterate using const-reference, to avoid copying the items as we are iterating. I would also use the type name instead of auto, for clarity, but I am ok with either. Lastly, the variable name "finds" is a bit strange, or I don't understand what you mean by it. > + if(finds.path.back() != m_struct_path.back()) > + { > + /* Same boffset points to members of different classes. > + We have found an ambiguity and should record it*/ > + ambiguous = true; > + } > + else > + { > + /* we don't need to insert this value again, because a > + non-ambiguous path already leads to it */ First letter capital, period + two spaces at the end. There are two lines above that have more than 8 spaces, that should be replaced with a tab. Most editors have a way to configure whitespaces to do that automatically. > + insert = false; Since this path makes it so that we'll definitely not insert, we can break the iteration. Simon