From ddc21ed73e4ce5ea2e78b800ce086712074c94f1 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Thu, 3 Mar 2016 18:48:26 +0100 Subject: [PATCH] Bug 19596 - Suppressed removed symbol changes still considered incompatible Libabigail considers a removed function (or global variable) as being an incompatible change, whether it has been suppressed or not. Likewise, even if all function sub-type changes have been suppressed, changed virtual offset on functions are still considered incompatible. Thus, abidiff still returns an exit code that reflects an incompatible change even if the removed symbol change was suppressed. The rule should rather be that if incompatible changes have been suppressed, abidiff (and the other tools) should take that into account and not return an exit code that reflects incompatible changes. This patch implements that rule, at least for the incompatible changes that are detected so far. * src/abg-comparison.cc (corpus_diff::has_incompatible_changes): Consider the *net* number of removed function and variable symbols. Also, if all function sub-type changes have been suppressed, then no virtual offset change should be considered incompatible. * tests/data/test-abidiff-exit/test1-voffset-change-report1.txt * tests/data/test-abidiff-exit/test1-voffset-change.abignore * tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt * tests/data/test-abidiff-exit/test2-filtered-removed-fns-report1.txt * tests/data/test-abidiff-exit/test2-filtered-removed-fns-v0.c * tests/data/test-abidiff-exit/test2-filtered-removed-fns-v1.c * tests/data/test-abidiff-exit/test2-filtered-removed-fns.abignore * tests/data/Makefile.am: Add the new test material above to source distribution. * tests/test-abidiff-exit.cc (InOutSpec::in_suppr_path): New data member. (in_out_specs): Adjust. Add new test inputs. (main): Adjust. Signed-off-by: Dodji Seketeli --- src/abg-comparison.cc | 15 ++++--- tests/data/Makefile.am | 9 ++++ .../test1-voffset-change-report1.txt | 3 ++ .../test1-voffset-change.abignore | 9 ++++ .../test2-filtered-removed-fns-report0.txt | 8 ++++ .../test2-filtered-removed-fns-report1.txt | 3 ++ .../test2-filtered-removed-fns-v0.c | 11 +++++ .../test2-filtered-removed-fns-v1.c | 6 +++ .../test2-filtered-removed-fns.abignore | 3 ++ tests/test-abidiff-exit.cc | 42 ++++++++++++++++++- 10 files changed, 102 insertions(+), 7 deletions(-) create mode 100644 tests/data/test-abidiff-exit/test1-voffset-change-report1.txt create mode 100644 tests/data/test-abidiff-exit/test1-voffset-change.abignore create mode 100644 tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt create mode 100644 tests/data/test-abidiff-exit/test2-filtered-removed-fns-report1.txt create mode 100644 tests/data/test-abidiff-exit/test2-filtered-removed-fns-v0.c create mode 100644 tests/data/test-abidiff-exit/test2-filtered-removed-fns-v1.c create mode 100644 tests/data/test-abidiff-exit/test2-filtered-removed-fns.abignore diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index a54851de..f7088cea 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -14624,11 +14624,16 @@ corpus_diff::has_incompatible_changes() const apply_filters_and_suppressions_before_reporting(); return (soname_changed() - || stats.num_func_removed() != 0 - || stats.num_func_with_virtual_offset_changes() != 0 - || stats.num_vars_removed() != 0 - || stats.num_func_syms_removed() != 0 - || stats.num_var_syms_removed() != 0); + || stats.net_num_func_removed() != 0 + || (stats.num_func_with_virtual_offset_changes() != 0 + // If all reports about functions with sub-type changes + // have been suppressd, then even those about functions + // that are virtual don't matter anymore because the + // user willingly requested to shut them down + && stats.net_num_func_changed() != 0) + || stats.net_num_vars_removed() != 0 + || stats.net_num_removed_func_syms() != 0 + || stats.net_num_removed_var_syms() != 0); } /// Test if the current instance of @ref corpus_diff carries subtype diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index f5bce293..fcdd0e9f 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -76,10 +76,19 @@ test-abidiff/test-PR18791-v0.so.abi \ test-abidiff/test-PR18791-v1.so.abi \ \ test-abidiff-exit/test1-voffset-change-report0.txt \ +test-abidiff-exit/test1-voffset-change-report1.txt \ +test-abidiff-exit/test1-voffset-change.abignore \ test-abidiff-exit/test1-voffset-change-v0.cc \ test-abidiff-exit/test1-voffset-change-v0.o \ test-abidiff-exit/test1-voffset-change-v1.cc \ test-abidiff-exit/test1-voffset-change-v1.o \ +test-abidiff-exit/test2-filtered-removed-fns-report0.txt \ +test-abidiff-exit/test2-filtered-removed-fns-report1.txt \ +test-abidiff-exit/test2-filtered-removed-fns-v0.c \ +test-abidiff-exit/test2-filtered-removed-fns-v1.c \ +test-abidiff-exit/test2-filtered-removed-fns-v0.o \ +test-abidiff-exit/test2-filtered-removed-fns-v1.o \ +test-abidiff-exit/test2-filtered-removed-fns.abignore \ \ test-diff-dwarf/test0-v0.cc \ test-diff-dwarf/test0-v0.o \ diff --git a/tests/data/test-abidiff-exit/test1-voffset-change-report1.txt b/tests/data/test-abidiff-exit/test1-voffset-change-report1.txt new file mode 100644 index 00000000..2bc7341e --- /dev/null +++ b/tests/data/test-abidiff-exit/test1-voffset-change-report1.txt @@ -0,0 +1,3 @@ +Functions changes summary: 0 Removed, 0 Changed (2 filtered out), 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + diff --git a/tests/data/test-abidiff-exit/test1-voffset-change.abignore b/tests/data/test-abidiff-exit/test1-voffset-change.abignore new file mode 100644 index 00000000..c79be6f5 --- /dev/null +++ b/tests/data/test-abidiff-exit/test1-voffset-change.abignore @@ -0,0 +1,9 @@ +[suppress_function] + name = C::virtual_func0 + change_kind = function-subtype-change + +[suppress_function] + name = C::virtual_func1 + change_kind = function-subtype-change + + diff --git a/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt new file mode 100644 index 00000000..9ae258c9 --- /dev/null +++ b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt @@ -0,0 +1,8 @@ +Functions changes summary: 1 Removed, 0 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 Removed function: + + 'function void to_erase()' {to_erase} + + diff --git a/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report1.txt b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report1.txt new file mode 100644 index 00000000..841801d4 --- /dev/null +++ b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-report1.txt @@ -0,0 +1,3 @@ +Functions changes summary: 0 Removed (1 filtered out), 0 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + diff --git a/tests/data/test-abidiff-exit/test2-filtered-removed-fns-v0.c b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-v0.c new file mode 100644 index 00000000..72efb009 --- /dev/null +++ b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-v0.c @@ -0,0 +1,11 @@ +/* Compile with: gcc -g -c -Wall foo-v0.c */ + +void +to_erase() +{ +} + +void +to_keep() +{ +} diff --git a/tests/data/test-abidiff-exit/test2-filtered-removed-fns-v1.c b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-v1.c new file mode 100644 index 00000000..6d31bf24 --- /dev/null +++ b/tests/data/test-abidiff-exit/test2-filtered-removed-fns-v1.c @@ -0,0 +1,6 @@ +/* Compile with: gcc -g -c -Wall foo-v1.c */ + +void +to_keep() +{ +} diff --git a/tests/data/test-abidiff-exit/test2-filtered-removed-fns.abignore b/tests/data/test-abidiff-exit/test2-filtered-removed-fns.abignore new file mode 100644 index 00000000..bde09f80 --- /dev/null +++ b/tests/data/test-abidiff-exit/test2-filtered-removed-fns.abignore @@ -0,0 +1,3 @@ +[suppress_function] + name = to_erase + change_kind = deleted-function diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc index 9fec3dc5..dd8cc247 100644 --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -46,6 +46,7 @@ struct InOutSpec { const char* in_elfv0_path; const char* in_elfv1_path; + const char* in_suppr_path; const char* abidiff_options; abidiff_status status; const char* in_report_path; @@ -57,13 +58,42 @@ InOutSpec in_out_specs[] = { "data/test-abidiff-exit/test1-voffset-change-v0.o", "data/test-abidiff-exit/test1-voffset-change-v1.o", + "", "--no-show-locs", abigail::tools_utils::ABIDIFF_ABI_CHANGE | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE, "data/test-abidiff-exit/test1-voffset-change-report0.txt", "output/test-abidiff-exit/test1-voffset-change-report0.txt" }, - {0, 0, 0 , abigail::tools_utils::ABIDIFF_OK, 0, 0} + { + "data/test-abidiff-exit/test1-voffset-change-v0.o", + "data/test-abidiff-exit/test1-voffset-change-v1.o", + "data/test-abidiff-exit/test1-voffset-change.abignore", + "--no-show-locs", + abigail::tools_utils::ABIDIFF_OK, + "data/test-abidiff-exit/test1-voffset-change-report1.txt", + "output/test-abidiff-exit/test1-voffset-change-report1.txt" + }, + { + "data/test-abidiff-exit/test2-filtered-removed-fns-v0.o", + "data/test-abidiff-exit/test2-filtered-removed-fns-v1.o", + "", + "--no-show-locs", + abigail::tools_utils::ABIDIFF_ABI_CHANGE + | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE, + "data/test-abidiff-exit/test2-filtered-removed-fns-report0.txt", + "output/test-abidiff-exit/test2-filtered-removed-fns-report0.txt" + }, + { + "data/test-abidiff-exit/test2-filtered-removed-fns-v0.o", + "data/test-abidiff-exit/test2-filtered-removed-fns-v1.o", + "data/test-abidiff-exit/test2-filtered-removed-fns.abignore", + "--no-show-locs", + abigail::tools_utils::ABIDIFF_OK, + "data/test-abidiff-exit/test2-filtered-removed-fns-report1.txt", + "output/test-abidiff-exit/test2-filtered-removed-fns-report1.txt" + }, + {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} }; int @@ -78,13 +108,18 @@ main() bool is_ok = true; string in_elfv0_path, in_elfv1_path, - abidiff_options, abidiff, cmd, + in_suppression_path, abidiff_options, abidiff, cmd, ref_diff_report_path, out_diff_report_path; for (InOutSpec* s = in_out_specs; s->in_elfv0_path; ++s) { in_elfv0_path = string(get_src_dir()) + "/tests/" + s->in_elfv0_path; in_elfv1_path = string(get_src_dir()) + "/tests/" + s->in_elfv1_path; + if (s->in_suppr_path && strcmp(s->in_suppr_path, "")) + in_suppression_path = + string(get_src_dir()) + "/tests/" + s->in_suppr_path; + else + in_suppression_path.clear(); abidiff_options = s->abidiff_options; ref_diff_report_path = @@ -104,6 +139,9 @@ main() if (!abidiff_options.empty()) abidiff += " " + abidiff_options; + if (!in_suppression_path.empty()) + abidiff += " --suppressions " + in_suppression_path; + cmd = abidiff + " " + in_elfv0_path + " " + in_elfv1_path; cmd += " > " + out_diff_report_path;