Fix propagation of private type suppression category

This is a partial fix of PR23700.

Conceptually, there are two kinds of type suppression specifications:

1/ a generic user-provided suppression specification that is meant to
suppress changes on types specified by the user

2/ a private type suppression specification that is automatically
generated from the path to public header files provided by the user.

Technically, one difference between 1 and 2 lays in the way we
propagate categories of changes matched by those suppression
specifications.

If a class type change of category SUPPRESSED_CATEGORY is referenced
in a typedef change, then the typedef change is also considered to be
of category SUPPRESSED_CATEGORY.  In other words, the
SUPPRESSED_CATEGORY category is propagated to the typedef change.
That means that if a change to a class type is suppressed, a (changed)
typedef to that class is considered to be suppressed too.

But then that is not true if the class type was changed because it's
private.  In that, a typedef to that class can be *public*, because
the said typedef is defined in a public header.  In that case the
typedef change should *NOT* be considered suppressed just because the
class type change was suppressed.

The problem we have here is that we don't make any difference between
1/ and 2/.  So we need to introduce different propagation rules for 1/
and 2/.

So this patch introduces a new PRIVATE_TYPE_CATEGORY category for
types suppression specification that are automatically generated for
private types.  That new category has its own propagation rule which
is basically "no propagation"; every type must be matched by the
private type suppression specification to be considered as private.

	* include/abg-comp-filter.h (has_harmful_name_change): Declare new
	function overloads.
	* include/abg-comparison.h (PRIVATE_TYPE_CATEGORY): New enumerator
	for diff_category;
	(EVERYTHING_CATEGORY): Adjust this enumerator in diff_category;
	(is_suppressed): Take an output parameter to say if the
	suppression is a private type suppression.
	* include/abg-suppression.h (is_private_type_suppr_spec): Take a
	const reference parameter and add an overload for a shared
	pointer.
	* src/abg-comp-filter.cc (has_harmful_name_change): Define new
	function.
	* src/abg-comparison-priv.h (diff::priv::is_filtered_out): Diffs
	of category PRIVATE_TYPE_CATEGORY are also considered filtered
	out.
	* src/abg-comparison.cc (diff::is_filtered_out): Adjust to account
	for canonical diffs of category PRIVATE_TYPE_CATEGORY.
	(diff::is_suppressed): Add an overload that takes a
	is_private_type output parameter.  Re-write the old overload in
	terms of the new one.
	(operator<<(ostream& o, diff_category c)): Handle
	PRIVATE_TYPE_CATEGORY.
	(category_propagation_visitor::visit_end):  Do not propagate
	PRIVATE_TYPE_CATEGORY here. Do not propagate
	HARMLESS_DECL_NAME_CHANGE_CATEGORY either, when the class does
	have a harmful decl name change.
	(suppression_categorization_visitor::visit_begin): Set the new
	PRIVATE_TYPE_CATEGORY category but do not propagate it.
	(suppression_categorization_visitor::visit_end): Add some
	comments.
	* src/abg-default-reporter.cc (default_reporter::report): Avoid
	reporting typedef underlying types that are in the
	PRIVATE_TYPE_CATEGORY category.
	* src/abg-suppression.cc (type_suppression::suppresses_diff): Do
	not peel typedefs if we are a private type suppression.
	(is_private_type_suppr_spec): Take a const reference.
	* tests/data/Makefile.am: Add the new test material below to
	source distribution.
	* tests/test-diff-suppr.cc: Use new test binary input.
	* tests/data/test-diff-filter/test7-report.txt: Adjust.
	* tests/data/test-diff-suppr/test39-opaque-type-report-0.txt: New
	test reference output.
	* tests/data/test-diff-suppr/test39-opaque-type-v{0,1}.c: Source
	code of new test binary input.
	* tests/data/test-diff-suppr/test39-opaque-type-v{0,1}.o: New test
	binary input.
	* tests/data/test-diff-suppr/test39-public-headers-dir/test39-header-v{0,1}.h:
	Source code of new test binary input.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

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);
 

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;
 

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

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.
 ///

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

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<diff*>::const_iterator i = d->children_nodes().begin();
 	     i != d->children_nodes().end();
 	     ++i)

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 '"

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());
+}
+
 // </file_suppression stuff>
 }// end namespace suppr
 } // end namespace abigail

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 \

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);
+
+

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
+
+
+

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)))
+{
+}

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)))
+{
+}

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*);

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*);

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}
 };