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

[Dodji Seketeli <dodji@seketeli.org>]

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