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

[Matthias Maennich via libabigail]

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