Re: [RFC] Fix has_net_changes for leaf-changes-only mode.

[Giuliano Procida <gprocida@google.com>]

Hi Dodji.

On Thu, 26 Mar 2020 at 17:12, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > * Which other functions need the same treatment?
> > * Should something else change instead?
> > * What about tracking of changed types (inclusion of these in summary
> >   stats and exit code for leaf mode)?
> >
> > This patch breaks tests and is not suitable for applying in its
> > current form.
> >
> > The issue we saw is that anonymous struct name changes triggered
> > hundreds of diffs
>
> Sorry, but what do you mean by "anonymous struct name changes" ?

These were kernel ABI differences. Freshly produced XML compared
against a recorded XML ABI. The recorded ABI was produced by a
slightly different version (of abidw). 1.8.0-fc4af1c4-android vs
1.8.0-7bd6f34f-android. Those are just libabigail commit hashes.
Current mm-next vs 2 merges ago.

With the --harmless and default reporting mode:1.8.0-7bd6f34f-android

Functions changes summary: 0 Removed, 410 Changed, 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

410 functions with some indirect sub-type change:

[...]

          while looking at anonymous data member 'union {struct
{sk_buff* next; sk_buff* prev; union {net_device* dev; unsigned long
int dev_scratch;};}; rb_node rbnode; list_head list;}':
          the internal name of that anonymous data memberchanged from:
           __anonymous_union__
          to:
           __anonymous_union__16
           This is usually due to an anonymous member type being added
or removed from the containing type

[many more]

With --harmless and leaf reporting mode:

Leaf changes summary: 0 artifact changed
Changed leaf types summary: 0 leaf type changed
Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable

(exit status 4)

> [...]
>
> > diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> > index 46bf9e30..7756c12b 100644
> > --- a/src/abg-comparison.cc
> > +++ b/src/abg-comparison.cc
> > @@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
> >      const diff_stats& stats = const_cast<corpus_diff*>(this)->
> >        apply_filters_and_suppressions_before_reporting();
> >
> > +    bool leaf = context()->show_leaf_changes_only();
> >      return (architecture_changed()
> > -         || soname_changed()
> > -         || stats.net_num_func_changed()
> > -         || stats.net_num_vars_changed()
> > -         || stats.net_num_func_added()
> > -         || stats.net_num_added_func_syms()
> > -         || stats.net_num_func_removed()
> > -         || stats.net_num_removed_func_syms()
> > -         || stats.net_num_vars_added()
> > -         || stats.net_num_added_var_syms()
> > -         || stats.net_num_vars_removed()
> > -         || stats.net_num_removed_var_syms()
> > -         || stats.net_num_added_unreachable_types()
> > -         || stats.net_num_removed_unreachable_types()
> > -         || stats.net_num_changed_unreachable_types());
> > +            || soname_changed()
> > +            || (leaf
> > +                  ? stats.net_num_leaf_func_changes()
> > +                  : stats.net_num_func_changed())
> > +            || (leaf
> > +                  ? stats.net_num_leaf_var_changes()
> > +                  : stats.net_num_vars_changed())
>
> In essence, this is code which behaviour depends on the kind of
> "reporter" we are using.  That is, if we are using the leaf reporter,
> then we want a given behaviour, otherwise we want another behaviour.
>
> So I think a route to make the whole thing maintainable going forward
> would be to put that code in the reporters.
>
> That is, in include/abg-reporter.h, add a pure virtual
> bool reporter_base::diff_has_net_changes(const corpus_diff&) const function.
>
> That interface would then be implemented in both default_reporter and
> leaf_reporter class types.
>
> Then in corpus_diff::has_net_changes(), we'd do something like:
>
>   return context()->get_reporter()->diff_has_net_changes(*this);
>
> That way, the actual choice of what constitute the set of net changes
> would be left to the each reporter.
>
> Does that make any sense?

It does, but I was really overthinking things and had missed that the
leaf report headers are already different and already include the
extra missing bit of information needed to make things work with all
current tests. This is net_num_type_changes.

I'll send an updated patch. I would like to improve test coverage and
look at the other related functions (has_changes etc.), but this issue
is causing pain right now.

Regards,
Giuliano.

> [...]
>
>
> > (mostly but not all filtered out in non-leaf mode which may be its own
> > bug).
>
> Hopefully with this approach, this issue should go away as well.
>
> Thanks a lot for looking into this.
>
> Cheers,
>
> --
>                 Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>