* [Patch] Bug 19272 - abipkgdiff doesn't report arch differences
@ 2016-01-01 0:00 Ondrej Oprala
2016-01-01 0:00 ` Dodji Seketeli
0 siblings, 1 reply; 2+ messages in thread
From: Ondrej Oprala @ 2016-01-01 0:00 UTC (permalink / raw)
To: libabigail
[-- 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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Patch] Bug 19272 - abipkgdiff doesn't report arch differences
2016-01-01 0:00 [Patch] Bug 19272 - abipkgdiff doesn't report arch differences Ondrej Oprala
@ 2016-01-01 0:00 ` Dodji Seketeli
0 siblings, 0 replies; 2+ messages in thread
From: Dodji Seketeli @ 2016-01-01 0:00 UTC (permalink / raw)
To: Ondrej Oprala; +Cc: libabigail
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-12-12 19:20 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-01 0:00 [Patch] Bug 19272 - abipkgdiff doesn't report arch differences Ondrej Oprala
2016-01-01 0:00 ` Dodji Seketeli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).