From patchwork Wed Jan 1 00:00:00 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aleksei Vetrov via Libabigail X-Patchwork-Id: 39012 X-Patchwork-Original-From: libabigail@sourceware.org (Matthias Maennich via libabigail) From: Aleksei Vetrov via Libabigail Date: Wed, 01 Jan 2020 00:00:00 -0000 Subject: [PATCH v2] abidiff/kmidiff: do not default-suppress added symbols In-Reply-To: <20191121000333.44501-1-maennich@google.com> References: <20191121000333.44501-1-maennich@google.com> Message-ID: <20200113101637.36477-1-maennich@google.com> kmidiff and abidiff do filter out added symbols (vars, functions and symbols without debug info) by default when dealing with kernel binaries. The reason for this is that the ABI could be considered compatible and not broken when adding symbols. In practice, this is confusing as there is no possibility for a symmetric comparison (i.e. a deleted function when comparing left to right is an added function when comparing right to left). Furthermore, there is no option available to actually report these added symbols. I thought of adding an option to report added symbols, but in the end came to the conclusion that we should behave consistent across the various ways you can diff an ABI with abidiff and kmidiff and should not change default behaviour for a particular type of binary. Hence, remove the default behaviour of filtering out added symbols when comparing kernel binaries. To restore the current behaviour, the user needs to parametrize with the tools with --no-added-syms --no-unreferenced-symbols. Adjusted test cases accordingly and add a new test that covers the old behaviour new available with additional flags to abidiff. * 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. * tests/data/test-diff-suppr/test46-PR25128-report-2.txt: Add test case for abidiff with flag --no-added-syms. * tests/data/Makefile.am: add new testcase. Signed-off-by: Matthias Maennich --- tests/data/Makefile.am | 1 + .../test-diff-suppr/test46-PR25128-report-1.txt | 8 ++++++-- .../test-diff-suppr/test46-PR25128-report-2.txt | 14 ++++++++++++++ tests/test-diff-suppr.cc | 10 ++++++++++ tools/abidiff.cc | 4 ---- tools/kmidiff.cc | 4 ---- 6 files changed, 31 insertions(+), 10 deletions(-) create mode 100644 tests/data/test-diff-suppr/test46-PR25128-report-2.txt diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 16026545fff4..27a2a3652c9f 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -1240,6 +1240,7 @@ test-diff-suppr/test45-abi.xml \ test-diff-suppr/test46-PR25128-base.xml \ test-diff-suppr/test46-PR25128-new.xml \ test-diff-suppr/test46-PR25128-report-1.txt \ +test-diff-suppr/test46-PR25128-report-2.txt \ test-diff-suppr/test47-non-reachable-types-report-1.txt \ test-diff-suppr/test47-non-reachable-types-report-2.txt \ test-diff-suppr/test47-non-reachable-types-report-3.txt \ diff --git a/tests/data/test-diff-suppr/test46-PR25128-report-1.txt b/tests/data/test-diff-suppr/test46-PR25128-report-1.txt index 558530e1e7f9..b08460d4799b 100644 --- a/tests/data/test-diff-suppr/test46-PR25128-report-1.txt +++ b/tests/data/test-diff-suppr/test46-PR25128-report-1.txt @@ -1,8 +1,12 @@ -Leaf changes summary: 1 artifact changed +Leaf changes summary: 2 artifacts changed Changed leaf types summary: 1 leaf type changed -Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function (1 filtered out) +Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 1 Added function Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable +1 Added function: + + [A] 'function void arch_set_max_freq_scale(cpumask*, unsigned long int)' + 'struct root_domain at sched.h:732:1' changed: type size changed from 14592 to 14720 (in bits) there are data member changes: diff --git a/tests/data/test-diff-suppr/test46-PR25128-report-2.txt b/tests/data/test-diff-suppr/test46-PR25128-report-2.txt new file mode 100644 index 000000000000..558530e1e7f9 --- /dev/null +++ b/tests/data/test-diff-suppr/test46-PR25128-report-2.txt @@ -0,0 +1,14 @@ +Leaf changes summary: 1 artifact changed +Changed leaf types summary: 1 leaf type changed +Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function (1 filtered out) +Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable + +'struct root_domain at sched.h:732:1' changed: + type size changed from 14592 to 14720 (in bits) + there are data member changes: + type 'unsigned long int' of 'root_domain::max_cpu_capacity' changed: + entity changed from 'unsigned long int' to 'struct max_cpu_capacity' at sched.h:722:1 + type size changed from 64 to 192 (in bits) +, size changed from 64 to 192 (in bits) (by +128 bits) + 'perf_domain* root_domain::pd' offset changed from 14528 to 14656 (in bits) (by +128 bits) + diff --git a/tests/test-diff-suppr.cc b/tests/test-diff-suppr.cc index 5d9528fe0bd9..b513a4b4180b 100644 --- a/tests/test-diff-suppr.cc +++ b/tests/test-diff-suppr.cc @@ -1838,6 +1838,16 @@ InOutSpec in_out_specs[] = "data/test-diff-suppr/test46-PR25128-report-1.txt", "output/test-diff-suppr/test46-PR25128-report-1.txt" }, + { + "data/test-diff-suppr/test46-PR25128-base.xml", + "data/test-diff-suppr/test46-PR25128-new.xml", + "", + "", + "", + "--no-default-suppression --leaf-changes-only --no-added-syms", + "data/test-diff-suppr/test46-PR25128-report-2.txt", + "output/test-diff-suppr/test46-PR25128-report-2.txt" + }, { "data/test-diff-suppr/test47-non-reachable-types-v0.o", "data/test-diff-suppr/test47-non-reachable-types-v1.o", diff --git a/tools/abidiff.cc b/tools/abidiff.cc index 1edb1b401cc4..77efcd167544 100644 --- a/tools/abidiff.cc +++ b/tools/abidiff.cc @@ -837,10 +837,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); } /// 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);