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

[Giuliano Procida <gprocida@google.com>]

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