From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd41.google.com (mail-io1-xd41.google.com [IPv6:2607:f8b0:4864:20::d41]) by sourceware.org (Postfix) with ESMTPS id C5CA9385ED4B for ; Tue, 4 Aug 2020 15:47:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C5CA9385ED4B Received: by mail-io1-xd41.google.com with SMTP id l1so42675792ioh.5 for ; Tue, 04 Aug 2020 08:47:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=z/URJPl5PgCP0OXJ8jvzGK5ZGarlQjkQ36sFzcjniqQ=; b=WEfIIXsIg6FLhWlcJ4QRGBaAIv0Jh4KpbGtzOFmo5yhJa7SVyqnvJzzzSL1i91U5+h KxgpbbGhksrH2C8jtoqareRrUOUQS2iWK9EbsHzrk+5GeM40h++z4sF4iNuwaHxM613M tp41vJm4DLwJM16AI3vdx/s71qm7IN910dfZwzTuIANOvWnmRgdnnN0UrMN/dd2qDY/3 H3nn/AWyGcb8UCfhZ0LNFMlogGTo/5vd3KCkDV5W6dxiKd1mnv1SnIFCRUyv04xO2PMS sYE9cK1ieXvvMHUYJvixUJIqhyHtncec9rfkJnIqJsgInBTLZGwMNMB3EnhZ1YCYQ2+o U8tA== X-Gm-Message-State: AOAM530yGLxKKqr8prHyCpU1n39izKATindusoZkmkaxL9iaKjo4FvwQ 8T71mOJIuadR1mQTfEszVQPUAmBCF2qdFL3Y2VO10Q== X-Google-Smtp-Source: ABdhPJwmPQ/GqWeVh1mKAUQai5jUQO8zGyhHN/rNHOO9x8YoFX8/pj8J4dDqYNb8g/cMQHnLohC4fSnzmSMCK+DlVBg= X-Received: by 2002:a02:dc3:: with SMTP id 186mr6333537jax.46.1596556057939; Tue, 04 Aug 2020 08:47:37 -0700 (PDT) MIME-Version: 1.0 References: <20200722110736.2550361-1-gprocida@google.com> <20200722110736.2550361-2-gprocida@google.com> <87pn86e9fw.fsf@seketeli.org> In-Reply-To: <87pn86e9fw.fsf@seketeli.org> From: Giuliano Procida Date: Tue, 4 Aug 2020 16:47:00 +0100 Message-ID: Subject: Re: [RFC PATCH 1/1] Fix decl_base comparison function To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com, =?UTF-8?Q?Matthias_M=C3=A4nnich?= X-Spam-Status: No, score=-28.5 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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:47:40 -0000 Thanks for the review and response. I'd like to check my understanding of something. On Tue, 4 Aug 2020 at 16:07, Dodji Seketeli wrote: > 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, > change_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). > I thought we could rely on their names (at least as a sufficient condition for inequality). Here are 3 examples. 1. In plain C, there's a flat namespace. struct { int x; } a; struct { int x; } b; At a language level the types of a and b are distinct and it's illegal to assign one to the other. error: incompatible types when assigning to type =E2=80=98struct =E2=80=99 from type =E2=80=98struct =E2=80=99 They are structurally the same but if we were treat them as identical then we'd get somewhat misleading diffs when the code gets changed to: struct { int x; } a; struct { long x; } b; [The compiler has to track the identity of these types separately which is probably easy enough, but we have a few choices of how to refer to the type: a) file, line, column (which is a bit fragile), b) with reference to the entities being given a type, which could be a typedef, c) where they appear in their scope, whatever that may mean.] 2. In C++, we get namespace and type name scopes. struct A { struct { int x; } a; struct { int x; } b; }; error: no match for =E2=80=98operator=3D=E2=80=99 (operand types are =E2=80= =98A::=E2=80=99 and =E2=80=98A::=E2=80=99) It's illegal to assign A::a to A::b as their types are distinct. [It's possible to propagate the type's identity to other declarations with decltype (and auto).] 3. struct A { struct { int x; } a; }; struct B { struct { int x; } a; }; Similarly, A::a and B::a have distinct types. error: no match for =E2=80=98operator=3D=E2=80=99 (operand types are =E2=80= =98A::=E2=80=99 and =E2=80=98B::=E2=80=99) This last case is what I thought I was addressing with that TODO. Let me know if we are talking at cross-purposes. Regards, Giuliano. 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; > > + } > > > > - 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); > > > > - 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-ve= rs-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.el7.x= 86_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, > > > > -- > Dodji >