diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h index ced3f3e7..9cba4360 100644 --- a/include/abg-comp-filter.h +++ b/include/abg-comp-filter.h @@ -42,6 +42,12 @@ namespace filtering bool has_harmless_name_change(const decl_base_sptr& f, const decl_base_sptr& s); +bool +has_harmful_name_change(const decl_base_sptr& f, const decl_base_sptr& s); + +bool +has_harmful_name_change(const diff* dif); + bool has_virtual_mem_fn_change(const function_decl_diff* diff); diff --git a/include/abg-comparison.h b/include/abg-comparison.h index b4c68349..bc1f221f 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -357,32 +357,38 @@ enum diff_category /// user-provided suppression specification. SUPPRESSED_CATEGORY = 1 << 7, + /// This means that a diff node was warked as being for a private + /// type. That is, the diff node is meant to be suppressed by a + /// suppression specification that was auto-generated to filter out + /// changes to private types. + PRIVATE_TYPE_CATEGORY = 1 << 8, + /// This means the diff node (or at least one of its descendant /// nodes) carries a change that modifies the size of a type or an /// offset of a type member. Removal or changes of enumerators in a /// enum fall in this category too. - SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 8, + SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 9, /// This means that a diff node in the sub-tree carries an /// incompatible change to a vtable. - VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 9, + VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 10, /// A diff node in this category is redundant. That means it's /// present as a child of a other nodes in the diff tree. - REDUNDANT_CATEGORY = 1 << 10, + REDUNDANT_CATEGORY = 1 << 11, /// This means that a diff node in the sub-tree carries a class type /// that was declaration-only and that is now defined, or vice /// versa. - CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 11, + CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 12, /// A diff node in this category is a function parameter type which /// top cv-qualifiers change. - FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 12, + FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 13, /// A diff node in this category is a function parameter type which /// cv-qualifiers change. - FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 13, + FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 14, /// A special enumerator that is the logical 'or' all the /// enumerators above. @@ -398,6 +404,7 @@ enum diff_category | HARMLESS_ENUM_CHANGE_CATEGORY | HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY | SUPPRESSED_CATEGORY + | PRIVATE_TYPE_CATEGORY | SIZE_OR_OFFSET_CHANGE_CATEGORY | VIRTUAL_MEMBER_CHANGE_CATEGORY | REDUNDANT_CATEGORY @@ -936,6 +943,9 @@ public: bool is_suppressed() const; + bool + is_suppressed(bool &is_private_type) const; + bool to_be_reported() const; diff --git a/include/abg-suppression.h b/include/abg-suppression.h index 34b445ec..edf54ef5 100644 --- a/include/abg-suppression.h +++ b/include/abg-suppression.h @@ -800,7 +800,10 @@ const char* get_private_types_suppr_spec_label(); bool -is_private_type_suppr_spec(suppr::type_suppression&); +is_private_type_suppr_spec(const type_suppression&); + +bool +is_private_type_suppr_spec(const suppression_sptr& s); } // end namespace suppr } // end namespace abigail diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index 02631dc7..057acc74 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -441,6 +441,41 @@ has_harmless_name_change(const decl_base_sptr& f, const decl_base_sptr& s) 0)))); } +/// Test if two decls represents a harmful name change. +/// +/// A harmful name change is a name change that is not harmless, so +/// this function uses the function has_harmless_name_change. +/// +/// @param f the first decl to consider in the comparison. +/// +/// @param s the second decl to consider in the comparison. +/// +/// @return true iff decl @p s represents a harmful name change over +/// @p f. +bool +has_harmful_name_change(const decl_base_sptr& f, const decl_base_sptr& s) +{return decl_name_changed(f, s) && ! has_harmless_name_change(f, s);} + +/// Test if a diff node represents a harmful name change. +/// +/// A harmful name change is a name change that is not harmless, so +/// this function uses the function has_harmless_name_change. +/// +/// @param f the first decl to consider in the comparison. +/// +/// @param s the second decl to consider in the comparison. +/// +/// @return true iff decl @p s represents a harmful name change over +/// @p f. +bool +has_harmful_name_change(const diff* dif) +{ + decl_base_sptr f = is_decl(dif->first_subject()), + s = is_decl(dif->second_subject()); + + return has_harmful_name_change(f, s); +} + /// Test if a class_diff node has non-static members added or /// removed. /// diff --git a/src/abg-comparison-priv.h b/src/abg-comparison-priv.h index 5fce8b75..20463554 100644 --- a/src/abg-comparison-priv.h +++ b/src/abg-comparison-priv.h @@ -323,8 +323,9 @@ public: return false; /// We don't want to display nodes suppressed by a user-provided - /// suppression specification. - if (category & SUPPRESSED_CATEGORY) + /// suppression specification or by a "private type" suppression + /// specification. + if (category & (SUPPRESSED_CATEGORY | PRIVATE_TYPE_CATEGORY)) return true; // We don't want to display redundant diff nodes, when the user diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 52a85d14..5919abd9 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -2372,10 +2372,13 @@ bool diff::is_filtered_out() const { 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. + if (canonical->get_category() & SUPPRESSED_CATEGORY + || canonical->get_category() & PRIVATE_TYPE_CATEGORY) + // The canonical type was suppressed either by a user-provided + // suppression specification or by a "private-type" suppression + // specification.. 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()); } @@ -2400,13 +2403,39 @@ diff::is_filtered_out_wrt_non_inherited_categories() const /// user-provided suppression list. bool diff::is_suppressed() const +{ + bool is_private = false; + return is_suppressed(is_private); +} + +/// Test if the current diff node has been suppressed by a +/// user-provided suppression specification or by an auto-generated +/// "private type" suppression specification. +/// +/// Note that private type suppressions are auto-generated from the +/// path to where public headers are, as given by the user. +/// +/// @param is_private_type out parameter if the current diff node was +/// suppressed because it's a private type then this parameter is set +/// to true. +/// +/// @return true if the current diff node has been suppressed by a +/// user-provided suppression list. +bool +diff::is_suppressed(bool &is_private_type) const { const suppressions_type& suppressions = context()->suppressions(); - for (suppressions_type::const_iterator i = suppressions.begin(); + for (suppressions_type::const_iterator i = suppressions.begin(); i != suppressions.end(); ++i) - if ((*i)->suppresses_diff(this)) - return true; + { + if ((*i)->suppresses_diff(this)) + { + if (is_private_type_suppr_spec(*i)) + is_private_type = true; + return true; + } + } return false; } @@ -2965,7 +2994,8 @@ operator<<(ostream& o, diff_category c) o << "STATIC_DATA_MEMBER_CHANGE_CATEGORY"; emitted_a_category |= true; } - else if (c & HARMLESS_ENUM_CHANGE_CATEGORY) + + if (c & HARMLESS_ENUM_CHANGE_CATEGORY) { if (emitted_a_category) o << "|"; @@ -2981,6 +3011,22 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } + if (c & SUPPRESSED_CATEGORY) + { + if (emitted_a_category) + o << "|"; + o << "SUPPRESSED_CATEGORY"; + emitted_a_category |= true; + } + + if (c & PRIVATE_TYPE_CATEGORY) + { + if (emitted_a_category) + o << "|"; + o << "PRIVATE_TYPE_CATEGORY"; + emitted_a_category |= true; + } + if (c & SIZE_OR_OFFSET_CHANGE_CATEGORY) { if (emitted_a_category) @@ -3021,12 +3067,11 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } - - if (c & SUPPRESSED_CATEGORY) + if (c & FN_PARM_TYPE_CV_CHANGE_CATEGORY) { if (emitted_a_category) o << "|"; - o << "SUPPRESSED_CATEGORY"; + o << "FN_PARM_TYPE_CV_CHANGE_CATEGORY"; emitted_a_category |= true; } @@ -10553,7 +10598,17 @@ struct category_propagation_visitor : public diff_node_visitor assert(diff); diff_category c = diff->get_category(); - c &= ~(REDUNDANT_CATEGORY|SUPPRESSED_CATEGORY); + // Do not propagate redundant and suppressed categories. Those + // are propagated in a specific pass elsewhere. + c &= ~(REDUNDANT_CATEGORY + | SUPPRESSED_CATEGORY + | PRIVATE_TYPE_CATEGORY); + // Also, if a (class) type has got a harmful name change, do not + // propagate harmless name changes coming from its sub-types + // (i.e, data members) to the class itself. + if (filtering::has_harmful_name_change(d)) + c &= ~HARMLESS_DECL_NAME_CHANGE_CATEGORY; + d->add_to_category(c); if (!already_visited && canonical) if (update_canonical) @@ -10629,15 +10684,19 @@ struct suppression_categorization_visitor : public diff_node_visitor virtual void visit_begin(diff* d) { - if (d->is_suppressed()) + bool is_private_type = false; + if (d->is_suppressed(is_private_type)) { - d->add_to_local_and_inherited_categories(SUPPRESSED_CATEGORY); + diff_category c = is_private_type + ? PRIVATE_TYPE_CATEGORY + : SUPPRESSED_CATEGORY; + d->add_to_local_and_inherited_categories(c); // 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); + canonical_diff->add_to_category(c); } } @@ -10676,6 +10735,13 @@ struct suppression_categorization_visitor : public diff_node_visitor && (!d->has_local_changes() || is_pointer_diff(d))) { + // Note that we handle private diff nodes differently from + // generally suppressed diff nodes. E.g, it's not because a + // type is private (and suppressed because of that; i.e, in + // the category PRIVATE_TYPE_CATEGORY) that a typedef to that + // type should also be private and so suppressed. Private + // diff nodes thus have different propagation rules than + // generally suppressed rules. for (vector::const_iterator i = d->children_nodes().begin(); i != d->children_nodes().end(); ++i) diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc index 63e1e354..1ce6fe94 100644 --- a/src/abg-default-reporter.cc +++ b/src/abg-default-reporter.cc @@ -279,7 +279,7 @@ default_reporter::report(const typedef_diff& d, // want to just know about the local changes of the typedef, // but also about the changes on the underlying type. diff_category c = dif->get_category(); - if (!(c & SUPPRESSED_CATEGORY)) + if (!(c & (SUPPRESSED_CATEGORY | PRIVATE_TYPE_CATEGORY))) { out << indent << "underlying type '" diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc index 61db764b..28cbc029 100644 --- a/src/abg-suppression.cc +++ b/src/abg-suppression.cc @@ -667,8 +667,16 @@ type_suppression::suppresses_diff(const diff* diff) const if (!suppresses_type(ft, d->context()) && !suppresses_type(st, d->context())) { - ft = peel_typedef_type(ft); - st = peel_typedef_type(st); + // A private type suppression specification considers tha a type + // can be private and yet some typedefs of that type can be + // public -- depending on, e.g, if the typedef is defined in a + // public header or not. So if we are in the context of a + // private type suppression let's *NOT* peel typedefs away. + if (!is_private_type_suppr_spec(*this)) + { + ft = peel_typedef_type(ft); + st = peel_typedef_type(st); + } if (!suppresses_type(ft, d->context()) && !suppresses_type(st, d->context())) @@ -4072,9 +4080,24 @@ get_private_types_suppr_spec_label() /// /// @return true iff @p s is a private type suppr spec. bool -is_private_type_suppr_spec(suppr::type_suppression& s) +is_private_type_suppr_spec(const type_suppression& s) {return s.get_label() == get_private_types_suppr_spec_label();} +/// Test if a type suppression specification represents a private type +/// suppression automatically generated by libabigail from the user +/// telling us where public headers are. +/// +/// @param s the suppression specification we are looking at. +/// +/// @return true iff @p s is a private type suppr spec. +bool +is_private_type_suppr_spec(const suppression_sptr& s) +{ + type_suppression_sptr type_suppr = is_type_suppression(s); + return (type_suppr + && type_suppr->get_label() == get_private_types_suppr_spec_label()); +} + // }// end namespace suppr } // end namespace abigail diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 3709a4de..09740f1d 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -1148,6 +1148,13 @@ test-diff-suppr/test38-char-class-in-ini-v0.o \ test-diff-suppr/test38-char-class-in-ini-v1.c \ test-diff-suppr/test38-char-class-in-ini-v1.o \ test-diff-suppr/test38-char-class-in-ini.abignore \ +test-diff-suppr/test39-opaque-type-report-0.txt \ +test-diff-suppr/test39-opaque-type-v0.c \ +test-diff-suppr/test39-opaque-type-v1.c \ +test-diff-suppr/test39-opaque-type-v0.o \ +test-diff-suppr/test39-opaque-type-v1.o \ +test-diff-suppr/test39-public-headers-dir/test39-header-v0.h \ +test-diff-suppr/test39-public-headers-dir/test39-header-v1.h \ \ 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-filter/test7-report.txt b/tests/data/test-diff-filter/test7-report.txt index 9666a8fd..f71441ee 100644 --- a/tests/data/test-diff-filter/test7-report.txt +++ b/tests/data/test-diff-filter/test7-report.txt @@ -1,3 +1,13 @@ -Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function +Functions changes summary: 0 Removed, 1 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable +1 function with some indirect sub-type change: + + [C]'function return_type foo()' has some indirect sub-type changes: + return type changed: + type name changed from 'return_type' to 'other_return_type' + type size hasn't changed + + no data member change (1 filtered); + + diff --git a/tests/data/test-diff-suppr/test39-opaque-type-report-0.txt b/tests/data/test-diff-suppr/test39-opaque-type-report-0.txt new file mode 100644 index 00000000..21995e0c --- /dev/null +++ b/tests/data/test-diff-suppr/test39-opaque-type-report-0.txt @@ -0,0 +1,12 @@ +Functions changes summary: 0 Removed, 1 Changed (1 filtered out), 0 Added functions +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 function with some indirect sub-type change: + + [C]'function void bar(StructType*)' at test39-opaque-type-v1.c:20:1 has some indirect sub-type changes: + parameter 1 of type 'StructType*' changed: + in pointed to type 'typedef StructType' at test39-header-v1.h:2:1: + typedef name changed from StructType to AnotherStructType at test39-header-v1.h:2:1 + + + diff --git a/tests/data/test-diff-suppr/test39-opaque-type-v0.c b/tests/data/test-diff-suppr/test39-opaque-type-v0.c new file mode 100644 index 00000000..38611d85 --- /dev/null +++ b/tests/data/test-diff-suppr/test39-opaque-type-v0.c @@ -0,0 +1,15 @@ +#include "test39-public-headers-dir/test39-header-v0.h" + +struct _StructType +{ + int m0; +}; + +void +foo(StructType* a __attribute__((unused))) +{ +} + +void bar(StructType* a __attribute__((unused))) +{ +} diff --git a/tests/data/test-diff-suppr/test39-opaque-type-v0.o b/tests/data/test-diff-suppr/test39-opaque-type-v0.o new file mode 100644 index 00000000..cb77ef49 Binary files /dev/null and b/tests/data/test-diff-suppr/test39-opaque-type-v0.o differ diff --git a/tests/data/test-diff-suppr/test39-opaque-type-v1.c b/tests/data/test-diff-suppr/test39-opaque-type-v1.c new file mode 100644 index 00000000..05ec2070 --- /dev/null +++ b/tests/data/test-diff-suppr/test39-opaque-type-v1.c @@ -0,0 +1,22 @@ +#include "test39-public-headers-dir/test39-header-v1.h" + +struct _StructType +{ + int m0; + char m1; +}; + +struct _AnotherStructType +{ + int m1; +}; + + +void +foo(StructType* a __attribute__((unused))) +{ +} + +void bar(AnotherStructType* a __attribute__((unused))) +{ +} diff --git a/tests/data/test-diff-suppr/test39-opaque-type-v1.o b/tests/data/test-diff-suppr/test39-opaque-type-v1.o new file mode 100644 index 00000000..29e4613b Binary files /dev/null and b/tests/data/test-diff-suppr/test39-opaque-type-v1.o differ diff --git a/tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v0.h b/tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v0.h new file mode 100644 index 00000000..0b38eddd --- /dev/null +++ b/tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v0.h @@ -0,0 +1,4 @@ +typedef struct _StructType StructType; + +void foo(StructType*); +void bar(StructType*); diff --git a/tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v1.h b/tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v1.h new file mode 100644 index 00000000..47558c57 --- /dev/null +++ b/tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v1.h @@ -0,0 +1,5 @@ +typedef struct _StructType StructType; +typedef struct _AnotherStructType AnotherStructType; + +void foo(StructType*); +void bar(AnotherStructType*); diff --git a/tests/test-diff-suppr.cc b/tests/test-diff-suppr.cc index 7e343940..e7a5b5cc 100644 --- a/tests/test-diff-suppr.cc +++ b/tests/test-diff-suppr.cc @@ -1718,6 +1718,16 @@ InOutSpec in_out_specs[] = "data/test-diff-suppr/test38-char-class-in-ini-report-0.txt", "output/test-diff-suppr/test38-char-class-in-ini-report-0.txt" }, + { + "data/test-diff-suppr/test39-opaque-type-v0.o", + "data/test-diff-suppr/test39-opaque-type-v1.o", + "data/test-diff-suppr/test39-public-headers-dir", + "data/test-diff-suppr/test39-public-headers-dir", + "", + "--no-default-suppression", + "data/test-diff-suppr/test39-opaque-type-report-0.txt", + "output/test-diff-suppr/test39-opaque-type-report-0.txt" + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL} };