Re: [RFC PATCH 1/1] Fix decl_base comparison function

[Giuliano Procida <gprocida@google.com>]

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 <dodji@seketeli.org> wrote:

> Giuliano Procida <gprocida@google.com> a écrit:
>
> [...]
>
> > 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 = (ln != rn);
> > -  if (decls_are_different
> > +  bool decls_are_same = (ln == 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 = 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 ‘struct <anonymous>’ from
type ‘struct <anonymous>’

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 ‘operator=’ (operand types are ‘A::<unnamed struct>’
and ‘A::<unnamed struct>’)

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 ‘operator=’ (operand types are ‘A::<unnamed struct>’
and ‘B::<unnamed struct>’)

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 = 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 = tools_utils::decl_names_equal(ln, rn);
> > +    decls_are_same = tools_utils::decl_names_equal(ln, rn);
> >
> > -  if (decls_are_different)
> > +  if (!decls_are_same)
> >      {
> >        result = 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.el7.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 <gprocida@google.com>
>
> Applied to master with the change above and after adjusting the commit
> log.
>
> Thanks!
>
> Cheers,
>
>
>
> --
>                 Dodji
>