From: Dodji Seketeli Date: Sat, 26 Nov 2016 10:01:45 +0000 (+0100) Subject: A suppressed diff node implies suppressing all equivalent nodes too X-Git-Tag: upstream/1.0~215 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f27520d7673f342a4fea88f869841d202d795d67;p=platform%2Fupstream%2Flibabigail.git A suppressed diff node implies suppressing all equivalent nodes too When a diff node N is suppressed (for instance, using the --headers-dir{1,2} option of abidiff}, it's only that diff node that is suppressed. We forget to suppress diff nodes that are equivalent to N. Here is why we forget to suppress diff ndoes that are equivalent. apply_suppressions walks the diff node graph to mark diff nodes as suppressed. But it does the walking by making sure each diff node's *class of equivalence* is visited once. This is not only a way to prevent infinite loops while visiting the graph, but also an optimization as it avoids walking two equivalent diff nodes. But then it can happen that we forget to categorize some diff nodes inside a given class of equivalence, even though we categorized some others. This patch makes it so that when a diff node inside a class of equivalence is categorized as SUPPRESSED, the canonical diff node of that class of equivalence is categorized as SUPPRESSED too. That way, to know if a diff node is suppressed, we just need to look at its canonical diff node. While doing this, I noticed that abidiff and abipkgdiff are not dropping private types from libabigail's internal representation, even though the Library now has that capability. The patch fixes that. But then the patch adds a --dont-drop-private-types option to abidiff to avoid dropping those private types from the IR, so that regression tests can make sure that a suppressed diff node implies suppression all equivalent nodes too. * doc/manuals/abidiff.rst b/doc/manuals/abidiff.rst: Document the new --dont-drop-private-types option. * src/abg-comparison.cc (diff::is_filtered_out): If the canonical type was suppressed then the current diff node is filtered out. (suppression_categorization_visitor::visit_{begin,end}): Categorized the canonical node as SUPPRESSED if the current node is suppressed. * tools/abidiff.cc (options::drop_private_types): New data member. (options::options): Initialize it. (display_usage): Add new help string for the new --dont-drop-private-types option. (parse_command_line): Parse the new --dont-drop-private-types option. (set_suppressions): Generate suppression specification from header directories given in parameter and stick them to the read context. * tools/abipkgdiff.cc (compare): Likewise. * tests/data/test-diff-suppr/libtest34-v0.so: New test input. * tests/data/test-diff-suppr/libtest34-v1.so: Likewise. * tests/data/test-diff-suppr/test34-report-0.txt: New reference report. * tests/data/test-diff-suppr/test34-v0.c: Source code for the new test input. * tests/data/test-diff-suppr/test34-v1.c: Likewise. * tests/data/test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h: Likewise. * tests/data/test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h: Likewise. * tests/data/test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h: Likewise. * tests/data/test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h: Likewise. * tests/data/Makefile.am: Add new test input material above to source distribution. * tests/test-diff-suppr.cc (in_out_spec): Compare the two new test library provided. Add --dont-drop-private-types to test30*. signed-off-by: Dodji Seketeli Signed-off-by: Dodji Seketeli --- diff --git a/doc/manuals/abidiff.rst b/doc/manuals/abidiff.rst index 8b86d92e..319c4729 100644 --- a/doc/manuals/abidiff.rst +++ b/doc/manuals/abidiff.rst @@ -96,6 +96,23 @@ Options library that the tool has to consider. The tool will thus filter out ABI changes on types that are not defined in public headers. + * ``--dont-drop-private-types`` + + This option is to be used with the ``--headers-dir1`` and + ``--headers-dir2`` options. Without this option, types that are + *NOT* defined in the headers are entirely dropped from the + internal representation build by Libabigail to represent the ABI. + They thus don't have to be filtered out from the final ABI change + report because they are not even present in Libabigail's + representation. + + With this option however, those private types are kept in the + internal representation and later filtered out from the report. + + This options thus potentially makes Libabigail to potentially + consume more memory. It's meant to be mainly used for debugging + purposes. + * ``--stat`` Rather than displaying the detailed ABI differences between diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index b33b1432..d6e0fa20 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -1944,7 +1944,15 @@ diff::set_local_category(diff_category c) /// @return true iff the current diff node should NOT be reported. bool diff::is_filtered_out() const -{return priv_->is_filtered_out(get_category());} +{ + if (diff * canonical = get_canonical_diff()) + if (canonical->get_category() & SUPPRESSED_CATEGORY) + // The canonical type was suppressed. This means all the class + // of equivalence of that canonical type was suppressed. So + // this node should be suppressed too. + return true; + return priv_->is_filtered_out(get_category()); +} /// Test if this diff tree node is to be filtered out for reporting /// purposes, but by considering only the categories that were *NOT* @@ -13116,7 +13124,15 @@ struct suppression_categorization_visitor : public diff_node_visitor visit_begin(diff* d) { if (d->is_suppressed()) - d->add_to_local_and_inherited_categories(SUPPRESSED_CATEGORY); + { + d->add_to_local_and_inherited_categories(SUPPRESSED_CATEGORY); + + // If a node was suppressed, all the other nodes of its class + // of equivalence are suppressed too. + diff *canonical_diff = d->get_canonical_diff(); + if (canonical_diff != d) + canonical_diff->add_to_category(SUPPRESSED_CATEGORY); + } } /// After visiting the children nodes of a given diff node, @@ -13161,7 +13177,14 @@ struct suppression_categorization_visitor : public diff_node_visitor if (has_non_empty_child && has_suppressed_child && !has_non_suppressed_child) - d->add_to_category(SUPPRESSED_CATEGORY); + { + d->add_to_category(SUPPRESSED_CATEGORY); + // If a node was suppressed, all the other nodes of its class + // of equivalence are suppressed too. + diff *canonical_diff = d->get_canonical_diff(); + if (canonical_diff != d) + canonical_diff->add_to_category(SUPPRESSED_CATEGORY); + } } } }; //end struct suppression_categorization_visitor diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 324da7d5..7e9fd2aa 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -999,6 +999,15 @@ test-diff-suppr/test33-v0.cc \ test-diff-suppr/test33-v0.h \ test-diff-suppr/test33-v1.cc \ test-diff-suppr/test33-v1.h \ +test-diff-suppr/libtest34-v0.so \ +test-diff-suppr/libtest34-v1.so \ +test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h \ +test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h \ +test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h \ +test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h \ +test-diff-suppr/test34-report-0.txt \ +test-diff-suppr/test34-v0.c \ +test-diff-suppr/test34-v1.c \ \ test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1 \ test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1.abi \ diff --git a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt index 2e39308f..ffd285c4 100644 --- a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt +++ b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt @@ -1,5 +1,5 @@ ================ changes of 'libtbb.so.2'=============== - Functions changes summary: 0 Removed, 7 Changed (17 filtered out), 17 Added functions + Functions changes summary: 0 Removed, 7 Changed (9 filtered out), 17 Added functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable Function symbols changes summary: 0 Removed, 0 Added function symbol not referenced by debug info Variable symbols changes summary: 3 Removed, 0 Added variable symbols not referenced by debug info @@ -46,7 +46,7 @@ in referenced type 'class tbb::task_group_context' at task.h:302:1: 1 data member insertion: 'tbb::internal::cpu_ctl_env_space tbb::task_group_context::my_cpu_ctl_env', at offset 896 (in bits) at task.h:380:1 - 1 data member changes (2 filtered): + 1 data member changes (1 filtered): type of 'char tbb::task_group_context::_leading_padding[80]' changed: type name changed from 'char[80]' to 'char[72]' array type size changed from 640 to 576 bits: diff --git a/tests/data/test-diff-suppr/libtest34-v0.so b/tests/data/test-diff-suppr/libtest34-v0.so new file mode 100755 index 00000000..a55e0b22 Binary files /dev/null and b/tests/data/test-diff-suppr/libtest34-v0.so differ diff --git a/tests/data/test-diff-suppr/libtest34-v1.so b/tests/data/test-diff-suppr/libtest34-v1.so new file mode 100755 index 00000000..e54f8476 Binary files /dev/null and b/tests/data/test-diff-suppr/libtest34-v1.so differ diff --git a/tests/data/test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h b/tests/data/test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h new file mode 100644 index 00000000..30ecc9dc --- /dev/null +++ b/tests/data/test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h @@ -0,0 +1,17 @@ +struct priv +{ + int m0; +}; + +void +init1(struct priv* p) +{ + p->m0 = 0; +} + +struct priv* +init2(struct priv* p) +{ + p->m0 = 0xCAFEBABE; + return p; +} diff --git a/tests/data/test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h b/tests/data/test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h new file mode 100644 index 00000000..4cbc4d70 --- /dev/null +++ b/tests/data/test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h @@ -0,0 +1,20 @@ +struct priv +{ + unsigned m0; + char m1; +}; + +void +init1 (struct priv* p) +{ + p->m0 = 0; + p->m1 = 0; +} + +struct priv* +init2(struct priv* p) +{ + p->m0 = 0xCAFEBABE; + p->m1 = 0xCA; + return p; +} diff --git a/tests/data/test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h b/tests/data/test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h new file mode 100644 index 00000000..2e41cc92 --- /dev/null +++ b/tests/data/test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h @@ -0,0 +1,20 @@ +struct priv; +typedef struct priv* priv_ptr; + +#define NULL (void*)0 + +struct S +{ + priv_ptr p; +}; + +typedef struct S* s_ptr; + +void +foo(s_ptr, int a); + +s_ptr +bar(s_ptr, char a); + +void +baz(s_ptr); diff --git a/tests/data/test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h b/tests/data/test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h new file mode 100644 index 00000000..2e41cc92 --- /dev/null +++ b/tests/data/test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h @@ -0,0 +1,20 @@ +struct priv; +typedef struct priv* priv_ptr; + +#define NULL (void*)0 + +struct S +{ + priv_ptr p; +}; + +typedef struct S* s_ptr; + +void +foo(s_ptr, int a); + +s_ptr +bar(s_ptr, char a); + +void +baz(s_ptr); diff --git a/tests/data/test-diff-suppr/test34-report-0.txt b/tests/data/test-diff-suppr/test34-report-0.txt new file mode 100644 index 00000000..2a093094 --- /dev/null +++ b/tests/data/test-diff-suppr/test34-report-0.txt @@ -0,0 +1,3 @@ +Functions changes summary: 0 Removed, 0 Changed (5 filtered out), 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + diff --git a/tests/data/test-diff-suppr/test34-v0.c b/tests/data/test-diff-suppr/test34-v0.c new file mode 100644 index 00000000..25761562 --- /dev/null +++ b/tests/data/test-diff-suppr/test34-v0.c @@ -0,0 +1,28 @@ +/* + * Compile with: + * gcc -shared -g -Wall -o libtest33-v0.so test33-v0.c + */ + +#include "test33-pub-include-dir-v0/test33-pub-include-v0.h" +#include "test33-priv-include-dir-v0/test33-priv-include-v0.h" + +void +foo(s_ptr ptr, int a) +{ + ptr->p = NULL; + ++a; +} + +s_ptr +bar(s_ptr ptr, char a) +{ + ptr->p = NULL; + ++a; + return ptr; +} + +void +baz(s_ptr ptr) +{ + ptr->p = NULL; +} diff --git a/tests/data/test-diff-suppr/test34-v1.c b/tests/data/test-diff-suppr/test34-v1.c new file mode 100644 index 00000000..6d3cca24 --- /dev/null +++ b/tests/data/test-diff-suppr/test34-v1.c @@ -0,0 +1,28 @@ +/* + * Compile with: + * gcc -shared -g -Wall -o libtest33-v1.so test33-v1.c + */ + +#include "test33-pub-include-dir-v1/test33-pub-include-v1.h" +#include "test33-priv-include-dir-v1/test33-priv-include-v1.h" + +void +foo(s_ptr ptr, int a) +{ + ptr->p = NULL; + a++; +} + +s_ptr +bar(s_ptr ptr, char a) +{ + ptr->p = NULL; + a++; + return ptr; +} + +void +baz(s_ptr ptr) +{ + ptr->p = NULL; +} diff --git a/tests/test-diff-suppr.cc b/tests/test-diff-suppr.cc index a356eaba..09876635 100644 --- a/tests/test-diff-suppr.cc +++ b/tests/test-diff-suppr.cc @@ -1604,7 +1604,7 @@ InOutSpec in_out_specs[] = "", "", "", - "--no-default-suppression", + "--no-default-suppression --dont-drop-private-types", "data/test-diff-suppr/test30-report-0.txt", "output/test-diff-suppr/test30-report-0.txt" }, @@ -1614,7 +1614,7 @@ InOutSpec in_out_specs[] = "data/test-diff-suppr/test30-include-dir-v0", "data/test-diff-suppr/test30-include-dir-v1", "", - "--no-default-suppression", + "--no-default-suppression --dont-drop-private-types", "data/test-diff-suppr/test30-report-1.txt", "output/test-diff-suppr/test30-report-1.txt" }, @@ -1663,11 +1663,21 @@ InOutSpec in_out_specs[] = "data/test-diff-suppr/libtest33-v1.so", "", "", - "data/test-diff-suppr/libtest33-1.suppr", + "data/test-diff-suppr/test33-suppr-1.txt", "--no-default-suppression --no-show-locs --no-redundant", "data/test-diff-suppr/test33-report-0.txt", "output/test-diff-suppr/test33-report-0.txt" }, + { + "data/test-diff-suppr/libtest34-v0.so", + "data/test-diff-suppr/libtest34-v1.so", + "data/test-diff-suppr/test34-pub-include-dir-v0", + "data/test-diff-suppr/test34-pub-include-dir-v1", + "", + "--no-default-suppression --dont-drop-private-types", + "data/test-diff-suppr/test34-report-0.txt", + "output/test-diff-suppr/test34-report-0.txt" + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL} }; diff --git a/tools/abidiff.cc b/tools/abidiff.cc index e2f165a5..4d344d6c 100644 --- a/tools/abidiff.cc +++ b/tools/abidiff.cc @@ -75,6 +75,7 @@ struct options vector keep_var_regex_patterns; string headers_dir1; string headers_dir2; + bool drop_private_types; bool no_default_supprs; bool no_arch; bool show_stats_only; @@ -103,6 +104,7 @@ struct options : display_usage(), display_version(), missing_operand(), + drop_private_types(true), no_default_supprs(), no_arch(), show_stats_only(), @@ -139,6 +141,8 @@ display_usage(const string& prog_name, ostream& out) << " --debug-info-dir2|--d2 the root for the debug info of file2\n" << " --headers-dir1|--hd1 the path to headers of file1\n" << " --headers-dir2|--hd2 the path to headers of file2\n" + << " --dont-drop-private-types\n keep private types in " + "internal representation\n" << " --stat only display the diff stats\n" << " --symtabs only display the symbol tables of the corpora\n" << " --no-default-suppression don't load any " @@ -275,6 +279,8 @@ parse_command_line(int argc, char* argv[], options& opts) opts.display_usage = true; return true; } + else if (!strcmp(argv[i], "--dont-drop-private-types")) + opts.drop_private_types = false; else if (!strcmp(argv[i], "--no-default-suppression")) opts.no_default_supprs = true; else if (!strcmp(argv[i], "--no-architecture")) @@ -591,7 +597,39 @@ set_suppressions(ReadContextType& read_ctxt, const options& opts) i != opts.suppression_paths.end(); ++i) read_suppressions(*i, supprs); - add_read_context_suppressions(read_ctxt, supprs); + + if (opts.drop_private_types) + { + if (!opts.headers_dir1.empty()) + { + // Generate suppression specification to avoid showing ABI + // changes on types that are not defined in public headers. + // + // As these suppression specifications are applied during the + // corpus loading, they are going to be dropped from the + // internal representation altogether. + suppression_sptr suppr = + gen_suppr_spec_from_headers(opts.headers_dir1); + if (suppr) + supprs.push_back(suppr); + } + + if (!opts.headers_dir2.empty()) + { + // Generate suppression specification to avoid showing ABI + // changes on types that are not defined in public headers. + // + // As these suppression specifications are applied during the + // corpus loading, they are going to be dropped from the + // internal representation altogether. + suppression_sptr suppr = + gen_suppr_spec_from_headers(opts.headers_dir2); + if (suppr) + supprs.push_back(suppr); + } + } + + add_read_context_suppressions(read_ctxt, supprs); } /// Set the regex patterns describing the functions to drop from the diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc index 14da2121..6200c1a8 100644 --- a/tools/abipkgdiff.cc +++ b/tools/abipkgdiff.cc @@ -113,6 +113,8 @@ using abigail::comparison::corpus_diff_sptr; using abigail::suppr::suppression_sptr; using abigail::suppr::suppressions_type; using abigail::suppr::read_suppressions; +using abigail::dwarf_reader::read_context_sptr; +using abigail::dwarf_reader::create_read_context; using abigail::dwarf_reader::get_soname_of_elf_file; using abigail::dwarf_reader::get_type_of_elf_file; using abigail::dwarf_reader::read_corpus_from_elf; @@ -1083,18 +1085,23 @@ compare(const elf_file& elf1, << elf1.path << " ...\n"; - corpus_sptr corpus1 = read_corpus_from_elf(elf1.path, &di_dir1, env.get(), - /*load_all_types=*/false, - c1_status); - if (!(c1_status & abigail::dwarf_reader::STATUS_OK)) - { - if (verbose) - emit_prefix("abipkgdiff", cerr) - << "Could not read file '" - << elf1.path - << "' properly\n"; - return abigail::tools_utils::ABIDIFF_ERROR; - } + corpus_sptr corpus1; + { + read_context_sptr c = create_read_context(elf1.path, &di_dir1, env.get(), + /*load_all_types=*/false); + add_read_context_suppressions(*c, priv_types_supprs1); + corpus1 = read_corpus_from_elf(*c, c1_status); + + if (!(c1_status & abigail::dwarf_reader::STATUS_OK)) + { + if (verbose) + emit_prefix("abipkgdiff", cerr) + << "Could not read file '" + << elf1.path + << "' properly\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } + } if (opts.fail_if_no_debug_info && (c1_status & abigail::dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND)) @@ -1120,18 +1127,23 @@ compare(const elf_file& elf1, << elf2.path << " ...\n"; - corpus_sptr corpus2 = read_corpus_from_elf(elf2.path, &di_dir2, env.get(), - /*load_all_types=*/false, - c2_status); - if (!(c2_status & abigail::dwarf_reader::STATUS_OK)) - { - if (verbose) - emit_prefix("abipkgdiff", cerr) - << "Could not find the read file '" - << elf2.path - << "' properly\n"; - return abigail::tools_utils::ABIDIFF_ERROR; - } + corpus_sptr corpus2; + { + read_context_sptr c = create_read_context(elf2.path, &di_dir2, env.get(), + /*load_all_types=*/false); + add_read_context_suppressions(*c, priv_types_supprs2); + corpus2 = read_corpus_from_elf(*c, c2_status); + + if (!(c2_status & abigail::dwarf_reader::STATUS_OK)) + { + if (verbose) + emit_prefix("abipkgdiff", cerr) + << "Could not find the read file '" + << elf2.path + << "' properly\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } + } if (opts.fail_if_no_debug_info && (c2_status & abigail::dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND))