Re: [PATCH 4/7] abidiff: Remove member function diff blank lines.

[Giuliano Procida <gprocida@google.com>]

Hi.

On Mon, 30 Mar 2020 at 15:13, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > Currently, lists of member function deletions, additions and changes
> > are each followed by a blank line in abidiff output.
> >
> > While this formatting works reasonably well in leaf-changes-only mode
> > for top-level changes to types, it is inconsistent with everthing
> > else. In particular, when reporting member function diffs in default
> > mode, or more deeply nested in leaf mode, the blank lines are
> > jarring.
> >
> > There was also no test coverage for leaf-changes-only mode.
> >
> > This change removes these blank lines and associated state variables
> > and logic. It adds a test case for leaf-changes-only mode.
> >
> > Note that the patch also modifies default mode reporting of member
> > types, template member functions and template member classes
> > analogously, but I wasn't able to exercise those code paths.
>
> The template handling code can be ignored for now.  I initially started
> to work on it but it needed huge adaptations on the compiler side to
> have it emit debug info for non-instantiated templates.  I eventually
> gave up on doing that because of more pressing matters.  So until we
> have a compiler that emits debug information for non instantiated
> templates, I don't think that code will be exercised.

Good to know. It's worth adding some comments to the code to this
effect, so I'll do this.

> [...]
>
>
> > diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
> > new file mode 100644
> > index 00000000..0437584e
> > --- /dev/null
> > +++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
> > @@ -0,0 +1,14 @@
> > +struct ops {
> > +  // TODO: type, name and size are differnent from deleted_var, but this is
> > +  // still reported as a change rather than a deletion and an addition.
> > +  long added_var;
> > +  virtual long added_fn() { return 0; }
>
> This is by design.  If a member variable S::foo_member at a given
> offset/ is replaced by another variable S::bar_member (foo_member and
> bar_member have the same offset), we chose to detect that foo_member was
> renamed into bar_member.
>
> So I think we can remove that TODO above.
>
> Would you agree?

I think the TODO has served its purpose which was to attract attention
(now or in the future). I'll remove it.

As to whether I agree... It was a little surprising. I think in most
large structs, if there are changes, the thing I'd mostly to discount
is offset, or perhaps name, next size (for arrays and perhaps integer
types on little endian architectures - but that is very risky) and
finally types. This doesn't really answer the question.

I think my mental model of comparing structs would be: list all the
names in offset order, compute a minimal edit based on names. Pair up
names that have been both removed and added and reclassify them as
moves (note that there may be more than one solution A,B -> B,A could
be A moving or B moving). Finally, perhaps, look at all (remaining)
possible matching removed/added pairs at the same position and see if
the type (including size) remains the same and reclassify these as
renames (this is also ambiguous and is more likely to be misleading
(bool flag1, bool flag2, -> bool flag3)).

If your structs correspond to network (wire) or hardware (DMA) data
structures, you're going to care about offsets an awful lot more.

Here are a couple more worth thinking about:

struct {
  char padding1[16];
  long foo;
  char padding2[8];
};

struct {
  char padding1[8];
  // did foo move and get renamed?
  long bar;
  char padding2[16];
};

and

struct {
  bool flag1;
  bool flag2;
};

struct {
  // flag1 deleted or flag1 renamed and flag2 deleted?
  bool flag2;
};

I'm happy for the code to be working as intended.

Regards.
Giuliano.

> Cheers,
>
> --
>                 Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>