diff mbox series

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

Message ID 20200324171344.258865-1-gprocida@google.com
State Superseded
Headers
Series [RFC] Fix has_net_changes for leaf-changes-only mode. |

Commit Message

Giuliano Procida March 24, 2020, 5:13 p.m. UTC 
  * 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 (mostly but not all filtered out in non-leaf mode
which may be its own bug).

	* src/abg-comparison.cc (corpus_diff::has_net_changes): In
        leaf-changes-only mode, check leaf changes to functions and
        variables to avoid returning non-zero exit code with empty
        output.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-comparison.cc | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

Giuliano Procida March 24, 2020, 5:17 p.m. UTC | #1 
Dodji, Matthias.

The test breakage I mentioned is returning exit code 0 despite type
changes impacting functions.

Comments welcome.

Giuliano.

On Tue, 24 Mar 2020 at 17:13, Giuliano Procida <gprocida@google.com> wrote:
>
> * 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 (mostly but not all filtered out in non-leaf mode
> which may be its own bug).
>
>         * src/abg-comparison.cc (corpus_diff::has_net_changes): In
>         leaf-changes-only mode, check leaf changes to functions and
>         variables to avoid returning non-zero exit code with empty
>         output.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  src/abg-comparison.cc | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> 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())
> +            || 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());
>  }
>
>  /// Apply the different filters that are registered to be applied to
> --
> 2.25.1.696.g5e7596f4ac-goog
>
Matthias Männich March 26, 2020, 10:37 a.m. UTC | #2 
On Tue, Mar 24, 2020 at 05:13:44PM +0000, Giuliano Procida wrote:
>* 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 (mostly but not all filtered out in non-leaf mode
>which may be its own bug).
>
>	* src/abg-comparison.cc (corpus_diff::has_net_changes): In
>        leaf-changes-only mode, check leaf changes to functions and
>        variables to avoid returning non-zero exit code with empty
>        output.
>

That patch looks generally good to me. Do you have an idea how to
address the test breakages?

>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-comparison.cc | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
>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();

const

Cheers,
Matthias

>     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())
>+            || 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());
> }
>
> /// Apply the different filters that are registered to be applied to
>-- 
>2.25.1.696.g5e7596f4ac-goog
>
Giuliano Procida March 26, 2020, 12:01 p.m. UTC | #3 
Hi Matthias.

On Thu, 26 Mar 2020 at 10:37, Matthias Maennich <maennich@google.com> wrote:
>
> On Tue, Mar 24, 2020 at 05:13:44PM +0000, Giuliano Procida wrote:
> >* 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 (mostly but not all filtered out in non-leaf mode
> >which may be its own bug).
> >
> >       * src/abg-comparison.cc (corpus_diff::has_net_changes): In
> >        leaf-changes-only mode, check leaf changes to functions and
> >        variables to avoid returning non-zero exit code with empty
> >        output.
> >
>
> That patch looks generally good to me. Do you have an idea how to
> address the test breakages?

Here's an example (the same without --impacted-interfaces):

$ build/tools/abidiff --impacted-interfaces --leaf-changes-only
tests/data/test-abidiff-exit/test-leaf2-v?.o
Leaf changes summary: 1 artifact changed
Changed leaf types summary: 1 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

'struct ops at test-leaf2-v0.cc:1:1' changed:
  type size hasn't changed
  there are data member changes:
    type 'void (int)*' of 'ops::munge' changed:
      pointer type changed from: 'void (int)*' to: 'char (long int, bool)*'

  one impacted interface:
    function void register_ops(const ops&)
$ echo $?
0

Exit code 0 is wrong. We should be counting and reporting changed
types and using the count as part of the exit code.

> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-comparison.cc | 33 +++++++++++++++++++--------------
> > 1 file changed, 19 insertions(+), 14 deletions(-)
> >
> >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();
>
> const

I think it's OK as-is. There are no instances of local "const bool" in
the code. This bool is only used in the next expression.

> Cheers,
> Matthias

Regards,
Giuliano.

> >     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())
> >+            || 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());
> > }
> >
> > /// Apply the different filters that are registered to be applied to
> >--
> >2.25.1.696.g5e7596f4ac-goog
> >
Dodji Seketeli March 26, 2020, 5:12 p.m. UTC | #4 
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" ?

[...]

> 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?

[...]


> (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,
Giuliano Procida March 27, 2020, 6:42 p.m. UTC | #5 
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.
>
diff mbox series

Patch

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())
+            || 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());
 }
 
 /// Apply the different filters that are registered to be applied to