[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH] abidiff/kmidiff: do not default-suppress added symbols

Hi Dodji!

On Tue, Jan 07, 2020 at 03:09:02PM +0100, Dodji Seketeli wrote:

Hello,

Matthias Maennich <maennich@google.com> a écrit:

[...]


diff --git a/tools/abidiff.cc b/tools/abidiff.cc
index a8b782898094..fda8d2f98565 100644
--- a/tools/abidiff.cc
+++ b/tools/abidiff.cc
@@ -815,10 +815,6 @@ static void
 adjust_diff_context_for_kmidiff(diff_context &ctxt)
 {
   ctxt.show_linkage_names(false);
-  ctxt.show_added_fns(false);
-  ctxt.show_added_vars(false);
-  ctxt.show_added_symbols_unreferenced_by_debug_info
-    (false);
 }


Please note that abidiff does have a --no-added-syms option which
suppresses added vars, functions and symbols for non-kernel binaries.  I
agree with you that abidiff should behave consistently regarding kernel
and non-kernel binaries, so by default, it should display added vars,
functions and symbols for kernel binaries.

But then, by the same token, I'd argue that the --no-added-syms option
should prevent those added vars, functions and symbols from being
reported for kernel binaries as well.

So I think this patch can go into master, as is.  And then I'll later
amend adjust_diff_context_for_kmidiff to make it honour the
--no-added-syms option, if you think that makes sense.


You are right. That is a regression. I will check whether I can directly
address this in a v2 of this patch.

Cheers,
Matthias




 /// Convert options::di_root_paths{1,2} into
diff --git a/tools/kmidiff.cc b/tools/kmidiff.cc
index 842381f8917e..9a1645284a01 100644
--- a/tools/kmidiff.cc
+++ b/tools/kmidiff.cc
@@ -322,10 +322,6 @@ set_diff_context(diff_context_sptr ctxt, const options& opts)
   ctxt->show_redundant_changes(false);
   ctxt->show_locs(true);
   ctxt->show_linkage_names(false);
-  ctxt->show_added_fns(false);
-  ctxt->show_added_vars(false);
-  ctxt->show_added_symbols_unreferenced_by_debug_info
-    (false);
   ctxt->show_symbols_unreferenced_by_debug_info
     (true);
   ctxt->show_leaf_changes_only(opts.leaf_changes_only);


As kmidiff.cc doesn't support the --no-added-syms option (that abidiff
supports), I agree with this hunk.

[...]



	* tools/abidiff.cc (adjust_diff_context_for_kmidiff): Drop
	default suppression of added symbols.
	* tools/kmidiff.cc (set_diff_context): Likewise.
	* tests/data/test-diff-suppr/test46-PR25128-report-1.txt: Adjust
	test expectation.


To sum it up, I'd say that the whole patch is OK to commit into master.

Thanks!

--
		Dodji