Re: [Patch] Bug 19272 - abipkgdiff doesn't report arch differences

Re: [Patch] Bug 19272 - abipkgdiff doesn't report arch differences

[Dodji Seketeli]

Hey Ondrej,

Ondrej Oprala <ondrej.oprala@gmail.com> a écrit:

> After a few months of silence, I finally picked out something from the
> BZ ;)

Glad to see you back! :-)

> Subject: [PATCH] Bug 19272 -  abipkgdiff doesn't report arch change
>
> Previously, architecture change wasn't included in the incompatible
> changes check.

Right.

> Basically, this patch adds a call to corpus_diff::architecture_changed()
> wherever corpus_diff::soname_changed() is called.
>
> Also, abipkgdiff seemed to only start reporting differences if a
> compatible ABI change was found.

Hhmmh ...

[...]


> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc

> @@ -11953,7 +11953,7 @@ corpus_diff::has_incompatible_changes() const
>    const diff_stats& stats = const_cast<corpus_diff*>(this)->
>      apply_filters_and_suppressions_before_reporting();
>  
> -  return (soname_changed()
> +  return (soname_changed() || architecture_changed()
>  	  || stats.net_num_func_removed() != 0
>  	  || (stats.num_func_with_virtual_offset_changes() != 0
>  	      // If all reports about functions with sub-type changes

Yes I agree with this change.

> diff --git a/tools/abicompat.cc b/tools/abicompat.cc
> index 9891795..159ad5f 100644
> --- a/tools/abicompat.cc
> +++ b/tools/abicompat.cc
> @@ -374,6 +374,7 @@ perform_compat_check_in_normal_mode(options& opts,
>      changes->apply_filters_and_suppressions_before_reporting();
>  
>    if (changes->soname_changed()
> +      || changes->architecture_changed()
>        || s.num_func_removed() != 0
>        || s.num_vars_removed() != 0
>        || s.num_func_syms_removed() != 0

Here, we are re-coding what corpus_diff::has_net_changes() does.
Basically, we want to know if changes (which is an instance of
corpus_diff) carries any changeset (that wasn't filtered out) at all.
and corpus_diff::has_net_change() is really what should be called.

I think the reason why we are not calling corpus_diff::has_net_change()
here because it was implemented *after* this code snipet in abicompat.cc
was implemented and I forgot to update abicompat.cc.

Incidentally, it seems that corpus_diff::has_net_change() doesn't take
architecture (or even soname) changes into account.  So that function
needs to be updated too.

And this entire "if" statement needs to be replaced by a 

    if (changes->has_net_changes())

> @@ -395,6 +396,7 @@ perform_compat_check_in_normal_mode(options& opts,
>        status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
>  
>        bool abi_broke_for_sure = changes->soname_changed()
> +	|| changes->architecture_changed()
>  	|| s.num_vars_removed()
>  	|| s.num_func_removed()
>  	|| s.num_var_syms_removed()

Here, what we are really looking to do is to detect incompatible abi
changes.

So this entire assignment statement should be replaced by:

    bool abi_broke_for_sure = changes->has_incompatible_changes();

> diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
> index a5c3901..24aa06e 100644
> --- a/tools/abipkgdiff.cc
> +++ b/tools/abipkgdiff.cc
> @@ -1236,7 +1236,8 @@ pthread_routine_compare(vector<compare_args_sptr> *args)
>  			    a->opts, env, diff, ctxt);
>  
>        const string key = a->elf1.path;
> -      if ((s & abigail::tools_utils::ABIDIFF_ABI_CHANGE)
> +      if ((s & (abigail::tools_utils::ABIDIFF_ABI_CHANGE |
> +                abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE))
>  	  || (verbose && diff->has_changes()))

You shouldn't have to add the | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE
because if there is an incompatible change, then the ABIDIFF_ABI_CHANGE
should be set too.

If it's not, then it's the logic that sets the ABIDIFF_ABI_CHANGE bit
that is wrong.

We see that the ABIDIFF_ABI_CHANGE bit is set in the compare() function
if, corpus_diff::has_net_change() is true for the diff we are looking
at.  So I think the fix to corpus_diff::has_net_change() above should
have fixed this as well.  So I think we don't need any change to
tools/abipkgdiff.cc at all.

Also, one thing that is missing from the patch is a regression test,
especially one that would be added to test-diff-pkg.cc.

I have amended the patch to fix the points I have commented on above and
pushed it to master after successfully running "make distcheck".  The
commit is at https://sourceware.org/git/?p=libabigail.git;a=commit;h=125c0c3c7c7d2243ebef759e5c3d0238cd7ccdf7.

Thank you for looking after this issue, it's appreciated.

Cheers,

-- 
		Dodji

[Patch] Bug 19272 - abipkgdiff doesn't report arch differences

[Ondrej Oprala]

[-- Attachment #1: Type: text/plain, Size: 109 bytes --]

Hi Dodji,

After a few months of silence, I finally picked out something from the BZ ;)

Cheers,

   Ondrej


[-- Attachment #2: 0001-Bug-19272-abipkgdiff-doesn-t-report-arch-change.patch --]
[-- Type: text/x-patch, Size: 2965 bytes --]

From 003408102c8875c7de40a6d0322820bf11ece522 Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <ondrej.oprala@gmail.com>
Date: Fri, 9 Dec 2016 23:01:58 +0100
Subject: [PATCH] Bug 19272 -  abipkgdiff doesn't report arch change

Previously, architecture change wasn't included in the incompatible
changes check.

Basically, this patch adds a call to corpus_diff::architecture_changed()
wherever corpus_diff::soname_changed() is called.

Also, abipkgdiff seemed to only start reporting differences if a
compatible ABI change was found.

	* src/abg-comparison.cc (corpus_diff::has_incompatible_changes):
	Check for an architecture change and return true if there was
	one.
	* tools/abicompat.cc (perform_compat_check_in_normal_mode):
	Likewise.
	* tools/abipkgdiff.cc (pthread_routine_compare): Report
	differences for {in,}compatible changes alike.

Signed-off-by: Ondrej Oprala <ondrej.oprala@gmail.com>
---
 src/abg-comparison.cc | 2 +-
 tools/abicompat.cc    | 2 ++
 tools/abipkgdiff.cc   | 3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index b57e7f0..320b74e 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -11953,7 +11953,7 @@ corpus_diff::has_incompatible_changes() const
   const diff_stats& stats = const_cast<corpus_diff*>(this)->
     apply_filters_and_suppressions_before_reporting();
 
-  return (soname_changed()
+  return (soname_changed() || architecture_changed()
 	  || stats.net_num_func_removed() != 0
 	  || (stats.num_func_with_virtual_offset_changes() != 0
 	      // If all reports about functions with sub-type changes
diff --git a/tools/abicompat.cc b/tools/abicompat.cc
index 9891795..159ad5f 100644
--- a/tools/abicompat.cc
+++ b/tools/abicompat.cc
@@ -374,6 +374,7 @@ perform_compat_check_in_normal_mode(options& opts,
     changes->apply_filters_and_suppressions_before_reporting();
 
   if (changes->soname_changed()
+      || changes->architecture_changed()
       || s.num_func_removed() != 0
       || s.num_vars_removed() != 0
       || s.num_func_syms_removed() != 0
@@ -395,6 +396,7 @@ perform_compat_check_in_normal_mode(options& opts,
       status |= abigail::tools_utils::ABIDIFF_ABI_CHANGE;
 
       bool abi_broke_for_sure = changes->soname_changed()
+	|| changes->architecture_changed()
 	|| s.num_vars_removed()
 	|| s.num_func_removed()
 	|| s.num_var_syms_removed()
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index a5c3901..24aa06e 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -1236,7 +1236,8 @@ pthread_routine_compare(vector<compare_args_sptr> *args)
 			    a->opts, env, diff, ctxt);
 
       const string key = a->elf1.path;
-      if ((s & abigail::tools_utils::ABIDIFF_ABI_CHANGE)
+      if ((s & (abigail::tools_utils::ABIDIFF_ABI_CHANGE |
+                abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE))
 	  || (verbose && diff->has_changes()))
 	{
 	  const string prefix = "  ";
-- 
2.7.4