From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by sourceware.org (Postfix) with ESMTPS id AC2283861838 for ; Tue, 4 Aug 2020 15:07:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AC2283861838 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dodji@seketeli.org X-Originating-IP: 91.166.131.130 Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodji@seketeli.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 4C6051C000B; Tue, 4 Aug 2020 15:07:32 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 00F4D180088E; Tue, 4 Aug 2020 17:07:31 +0200 (CEST) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com, maennich@google.com Subject: Re: [RFC PATCH 1/1] Fix decl_base comparison function Organization: Me, myself and I References: <20200722110736.2550361-1-gprocida@google.com> <20200722110736.2550361-2-gprocida@google.com> X-Operating-System: Red Hat Enterprise Linux Workstation 7.8 Beta X-URL: http://www.seketeli.net/~dodji Date: Tue, 04 Aug 2020 17:07:31 +0200 In-Reply-To: <20200722110736.2550361-2-gprocida@google.com> (Giuliano Procida's message of "Wed, 22 Jul 2020 12:07:36 +0100") Message-ID: <87pn86e9fw.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Aug 2020 15:07:36 -0000 Giuliano Procida a =C3=A9crit: [...] > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > index 1fe6f499..aa2a56fa 100644 > --- a/src/abg-ir.cc > +++ b/src/abg-ir.cc > @@ -4067,26 +4067,33 @@ equals(const decl_base& l, const decl_base& r, ch= ange_kind* k) > /// Otherwise, let's just compare their name, the obvious way. > /// That's the fast path because in that case the names are > /// interned_string and comparing them is much faster. > - bool decls_are_different =3D (ln !=3D rn); > - if (decls_are_different > + bool decls_are_same =3D (ln =3D=3D rn); > + if (!decls_are_same > && l.get_is_anonymous() > && !l.get_has_anonymous_parent() > && r.get_is_anonymous() > && !r.get_has_anonymous_parent()) > - // Both decls are anonymous and their scope are *NOT* anonymous. > - // So we consider the decls to have equivalent names (both > - // anonymous, remember). We are still in the fast path here. > - decls_are_different =3D false; > + { > + // Both decls are anonymous and their scope are *NOT* anonymous. > + // So we consider the decls to have equivalent names (both > + // anonymous, remember). We are still in the fast path here. > + // > + // TODO: Don't conflate anonymous structs, unions and enums? Yes, we want to compare decls here, irrespective (as much as possible) of the kind of types they are. That is precisely the point of this function. The specifics of structs, unions and enums are dealt with in the overloads of the equals functions that are specific to those types. Those specific overloads use this one, precisely to compare the generic "decl-related-part" of those types. That generic part has to do with the "naming" of those entities. > + // > + // TODO: Should we really be conflating all foo1::..::fooM::anon > + // with all bar1::..::barN::anon? The reasoning here is that two declarations entities that are anonymous can not be named, by definition. And that is irrespective of their (naming) context. So, for naming purposes, we can't say for sure that foo1::..::fooM::anon and bar1::..::barN::anon are different just based on their name (or the name of their context for that matter). To tell if these two entities are different, we'd have to look at something else but their name. So for now, we assume they are equal. later down in the function, other properties (related to declarations in the generic sense) are looked at to try to determine if they are equal. So I am removing these two TODO comments. > + decls_are_same =3D true; > + } >=20=20 > - if (decls_are_different > + if (!decls_are_same > && l.get_has_anonymous_parent() > && r.get_has_anonymous_parent()) > // This is the slow path as we are comparing the decl qualified > // names component by component, properly handling anonymous > // scopes. > - decls_are_different =3D tools_utils::decl_names_equal(ln, rn); > + decls_are_same =3D tools_utils::decl_names_equal(ln, rn); >=20=20 > - if (decls_are_different) > + if (!decls_are_same) > { > result =3D false; > if (k) [...] > > * src/abg-ir.cc (equals): In the decl_base overload, note that > the value returned by decl_names_equal should be negated and > replace decls_are_different with decls_are_same, negating all > occurrences. > * tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt: > Update tests, removing some spurious anonymous union name change. > * tests/data/test-diff-filter/test33-report-0.txt: Diff now > completely empty. > * tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym= -vers-report-0.txt: > 3 functions previously considered to have harmless changes are > now deemed to have no changes. > * tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el= 7.x86_64-report-2.txt: > 1 struct RedStore data member previously considered to have > harmless changes is now deemed to have no changes. > * tests/data/test-read-dwarf/test9-pr18818-clang.so.abi: > One instance of an anonymous struct removed and a typedef > repointed at another existing instance; many type ids > renumbered. > > Signed-off-by: Giuliano Procida Applied to master with the change above and after adjusting the commit log. Thanks! Cheers, --=20 Dodji