Properly propagate {REDUNDANT, SUPPRESSED}_CATEGORY wrt local changes

* src/abg-comparison.cc
	(suppression_categorization_visitor::visit_end): If a diff node
	carries local changes, then, even if all of its children node have
	been suppressed, this diff node shall not be categorized as
	suppressed by way of propagation.
	(redundancy_marking_visitor::visit_end): If a diff node carries
	local changes, then, even if all of its children nodes are
	redundant, this diff node shall not be categorized as being
	redundant by way of propagation.
	* tests/data/test-diff-suppr/libtest4-local-suppr-v{0,1}.so: New test
	inputs.
	* tests/data/test-diff-suppr/test4-local-suppr-0.suppr: Likewise.
	* tests/data/test-diff-suppr/test4-local-suppr-report-{0,1}.txt:
	Likewise.
	* tests/data/test-diff-suppr/test4-local-suppr-v{0,1}.{c,h}:
	Source code of the new tests inputs.
	* tests/Makefile.am: Add the new test material to the source
	distribution.
	* tests/test-diff-suppr.cc (in_out_spec): Run this test harness
	over the new test input above.

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

src/abg-comparison.cc

@@ -8983,7 +8983,8 @@ struct suppression_categorization_visitor : public diff_node_visitor
     bool has_non_empty_child = false;
     bool has_suppressed_child = false;
 
-    if (!(d->get_category() & SUPPRESSED_CATEGORY))
+    if (!(d->get_category() & SUPPRESSED_CATEGORY)
+	&& !d->has_local_changes())
       {
 	for (vector<diff*>::const_iterator i = d->children_nodes().begin();
 	     i != d->children_nodes().end();
@@ -9205,32 +9206,35 @@ struct redundancy_marking_visitor : public diff_node_visitor
   virtual void
   visit_end(diff* d)
   {
-    bool has_non_redundant_child = false;
-    bool has_non_empty_child = false;
-    for (vector<diff*>::const_iterator i = d->children_nodes().begin();
-	 i != d->children_nodes().end();
-	 ++i)
+    // Propagate the redundancy categorization of the children nodes
+    // to this node.  But if this node has local changes, then it
+    // doesn't inherit redundancy from its children nodes.
+    if (!(d->get_category() & REDUNDANT_CATEGORY)
+	&& !d->has_local_changes())
       {
-	if ((*i)->length())
+	bool has_non_redundant_child = false;
+	bool has_non_empty_child = false;
+	for (vector<diff*>::const_iterator i = d->children_nodes().begin();
+	     i != d->children_nodes().end();
+	     ++i)
 	  {
-	    has_non_empty_child = true;
-	    if (((*i)->get_category() & REDUNDANT_CATEGORY) == 0)
-	      has_non_redundant_child = true;
+	    if ((*i)->length())
+	      {
+		has_non_empty_child = true;
+		if (((*i)->get_category() & REDUNDANT_CATEGORY) == 0)
+		  has_non_redundant_child = true;
+	      }
+	    if (has_non_redundant_child)
+	      break;
 	  }
-	if (has_non_redundant_child)
-	  break;
-      }
 
-    // A diff node for which at least child node carries a change, and
-    // for which all the children are redundant is deemed redundant too.
-    //
-    // TODO: xxx when, for a given node, we can tell the difference
-    // between local changes and changes coming from children nodes,
-    // we'll have to alter the condition above: if the current node
-    // has local changes, even if all its children are redundant,
-    // won't inherit redundancy from its children.
-    if (has_non_empty_child && !has_non_redundant_child)
-      d->add_to_category(REDUNDANT_CATEGORY);
+	// A diff node for which at least a child node carries a
+	// change, and for which all the children are redundant is
+	// deemed redundant too, unless it has local changes.
+	if (has_non_empty_child
+	    && !has_non_redundant_child)
+	  d->add_to_category(REDUNDANT_CATEGORY);
+      }
   }
 
   virtual void

tests/Makefile.am

@@ -388,6 +388,15 @@ tests/data/test-diff-suppr/test3-struct-suppr-v0.cc \
 tests/data/test-diff-suppr/test3-struct-suppr-v0.o \
 tests/data/test-diff-suppr/test3-struct-suppr-v1.cc \
 tests/data/test-diff-suppr/test3-struct-suppr-v1.o \
+tests/data/test-diff-suppr/libtest4-local-suppr-v0.so \
+tests/data/test-diff-suppr/libtest4-local-suppr-v1.so \
+tests/data/test-diff-suppr/test4-local-suppr-0.suppr \
+tests/data/test-diff-suppr/test4-local-suppr-report-0.txt \
+tests/data/test-diff-suppr/test4-local-suppr-report-1.txt \
+tests/data/test-diff-suppr/test4-local-suppr-v0.c \
+tests/data/test-diff-suppr/test4-local-suppr-v0.h \
+tests/data/test-diff-suppr/test4-local-suppr-v1.c \
+tests/data/test-diff-suppr/test4-local-suppr-v1.h \
 \
 data/test-lookup-syms/test0.cc			\
 data/test-lookup-syms/test0.o			\

tests/data/test-diff-suppr/test4-local-suppr-0.suppr

@@ -0,0 +1,3 @@
+[suppress_type]
+  # Types whose name start with "private" should not be flagged
+  name_regexp = ^private.*

tests/data/test-diff-suppr/test4-local-suppr-report-0.txt

@@ -0,0 +1,25 @@
+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 void foo(public_type*, a_not_private_type*)' has some indirect sub-type changes:
+    parameter 0 of type 'public_type*' has sub-type changes:
+      in pointed to type 'struct public_type':
+        size changed from 64 to 128 bits
+        1 data member insertion:
+          'unsigned int public_type::oops', at offset 0 (in bits)
+        1 data member change:
+         'private_data* public_type::priv_' offset changed from 0 to 64
+         and its type 'private_data*' changed:
+           in pointed to type 'struct private_data':
+             size changed from 32 to 64 bits
+             1 data member insertion:
+               'char private_data::private_data1', at offset 32 (in bits)
+
+    parameter 1 of type 'a_not_private_type*' has sub-type changes:
+      in pointed to type 'struct a_not_private_type':
+        size changed from 32 to 64 bits
+        1 data member insertion:
+          'char a_not_private_type::j', at offset 32 (in bits)
+

tests/data/test-diff-suppr/test4-local-suppr-report-1.txt

@@ -0,0 +1,20 @@
+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 void foo(public_type*, a_not_private_type*)' has some indirect sub-type changes:
+    parameter 0 of type 'public_type*' has sub-type changes:
+      in pointed to type 'struct public_type':
+        size changed from 64 to 128 bits
+        1 data member insertion:
+          'unsigned int public_type::oops', at offset 0 (in bits)
+        1 data member change:
+         'private_data* public_type::priv_' offset changed from 0 to 64
+
+    parameter 1 of type 'a_not_private_type*' has sub-type changes:
+      in pointed to type 'struct a_not_private_type':
+        size changed from 32 to 64 bits
+        1 data member insertion:
+          'char a_not_private_type::j', at offset 32 (in bits)
+

tests/data/test-diff-suppr/test4-local-suppr-v0.c

@@ -0,0 +1,16 @@
+// To compile this, type:
+// gcc -shared -g -Wall -o libtest4-local-suppr-v0.so test4-local-suppr-v0.c
+
+#include "test4-local-suppr-v0.h"
+
+struct private_data
+{
+  int private_data0;
+};
+
+void
+foo(struct public_type* p __attribute__((unused)),
+    struct a_not_private_type* t __attribute__((unused)))
+{
+  /* Do something with p */
+}

tests/data/test-diff-suppr/test4-local-suppr-v0.h

@@ -0,0 +1,13 @@
+struct private_opaque_data;
+struct public_type
+{
+  struct private_data* priv_;
+};
+
+struct a_not_private_type
+{
+  int i;
+};
+
+void
+foo(struct public_type* p, struct a_not_private_type* t);

tests/data/test-diff-suppr/test4-local-suppr-v1.c

@@ -0,0 +1,18 @@
+// To compile this, type:
+// gcc -shared -g -Wall -o libtest4-local-suppr-v0.so test4-local-suppr-v0.c
+
+#include "test4-local-suppr-v1.h"
+
+struct private_data
+{
+  int private_data0;
+  char private_data1; // This new member should not be flagged when
+		      // using the suppression list.
+};
+
+void
+foo(struct public_type* p __attribute__((unused)),
+    struct a_not_private_type* t __attribute__((unused)))
+{
+  /* Do something with p */
+}

tests/data/test-diff-suppr/test4-local-suppr-v1.h

@@ -0,0 +1,16 @@
+struct private_opaque_data;
+struct public_type
+{
+  unsigned oops; // <--- we accidentally added a member here.  This
+		 // breaks ABI.
+  struct private_data* priv_;
+};
+
+struct a_not_private_type
+{
+  int i;
+  char j; // <-- This added member should be flagged too.
+};
+
+void
+foo(struct public_type* p, struct a_not_private_type* t);

tests/test-diff-suppr.cc

@@ -151,6 +151,22 @@ InOutSpec in_out_specs[] =
     "data/test-diff-suppr/test3-struct-suppr-report-2.txt",
     "output/test-diff-suppr/test3-struct-suppr-report-2.txt",
   },
+  {
+    "data/test-diff-suppr/libtest4-local-suppr-v0.so",
+    "data/test-diff-suppr/libtest4-local-suppr-v1.so",
+    "data/test-diff-suppr/test4-local-suppr-0.suppr",
+    "",
+    "data/test-diff-suppr/test4-local-suppr-report-1.txt",
+    "output/test-diff-suppr/test4-local-suppr-report-1.txt",
+  },
+  {
+    "data/test-diff-suppr/libtest4-local-suppr-v0.so",
+    "data/test-diff-suppr/libtest4-local-suppr-v1.so",
+    "",
+    "",
+    "data/test-diff-suppr/test4-local-suppr-report-0.txt",
+    "output/test-diff-suppr/test4-local-suppr-report-0.txt",
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL, NULL}
 };