Fix redundancy detection through fn ptr and typedef paths

When analyzing the libfreetype.so binary, it appears that
libabigail's diff node redundancy marking pass is failing to detect a
redundant diff node in cases were the node is recursively referencing
itself through a path that involves function type and typedef diff
nodes.

So it is only at reporting time that we'd detect that the node is
redundant so we emit messages like "this change was reported
earlier".  But When the earlier change in question is suppressed due
to, e.g, a suppression specification resulting from the user providing
abidiff with the --headers-dir{1,2} command line option, then the
change report becomes confusing, at best.

The right behaviour is to detect the node is redundant and mark it as
such, so that the reporting pass can avoid reporting it altogether.

This is what this patch does.

This patch changes the output of the runtestdiffpkg regression test.
To update the reference output, we need an additional patch to handle
a separate (but somewhat related) issue.  That is going to be done in
the subsequent commit which title is:

	    "Filter out changes like type to const type"

	* include/abg-comparison.h
	(is_function_type_diff_with_local_changes)
	(is_reference_or_pointer_diff_to_non_basic_distinct_types)
	(peel_typedef_diff): Declare new functions.
	* src/abg-comparison.cc
	(is_function_type_diff_with_local_changes)
	(is_reference_or_ptr_diff_to_non_basic_nor_distinct_types)
	(peel_typedef_diff): Define new functions.
	(is_reference_or_pointer_diff): Peel typedefs before operating.
	(redundancy_marking_visitor::visit_begin): Only sibbling parameter
	diff node that carry basic type changes (or distinct type changes)
	are *not* marked as redundant.  All other kinds of sibbling
	parameter diff nodes are markes redundant.  Also, rather than
	never marking function type diffs as redundant by fear of missing
	local changes on these, just avoid marking function type diff
	nodes with local changes.  It's possible to be that precise now
	that we can detect that a diff node carries local changes.
	* tests/data/test-diff-suppr/test37-opaque-type-v{0,1}.o: New
	binary tests input.
	* tests/data/test-diff-suppr/test37-opaque-type-v{0,1}.c: Source
	code of the binary tests input above.
	* tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v{0,1}.h:
	Headers of the binary tests input above.
	* tests/data/test-diff-suppr/test37-opaque-type-report-0.txt:
	Reference output for this new test.
	* tests/data/Makefile.am: Add the new test material above to
	source distribution.
	* tests/test-diff-suppr.cc (in_out_specs): Add the new test input
	above to the test harness.

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

include/abg-comparison.h

@@ -2682,6 +2682,9 @@ is_array_diff(const diff* diff);
 const function_type_diff*
 is_function_type_diff(const diff* diff);
 
+const function_type_diff*
+is_function_type_diff_with_local_changes(const diff* diff);
+
 const typedef_diff*
 is_typedef_diff(const diff *diff);
 
@@ -2702,6 +2705,9 @@ is_qualified_type_diff(const diff* diff);
 bool
 is_reference_or_pointer_diff(const diff* diff);
 
+bool
+is_reference_or_pointer_diff_to_non_basic_distinct_types(const diff* diff);
+
 const fn_parm_diff*
 is_fn_parm_diff(const diff* diff);
 
@@ -2720,6 +2726,9 @@ is_child_node_of_base_diff(const diff* diff);
 const corpus_diff*
 is_corpus_diff(const diff* diff);
 
+const diff*
+peel_typedef_diff(const diff* dif);
+
 const diff*
 peel_pointer_diff(const diff* dif);
 

src/abg-comparison.cc

@@ -675,6 +675,23 @@ const function_type_diff*
 is_function_type_diff(const diff* diff)
 {return dynamic_cast<const function_type_diff*>(diff);}
 
+/// Test if a given diff node carries a function type change with
+/// local changes.
+///
+/// @param diff the diff node to consider.
+///
+/// @return a non-nil pointer to a @ref function_type_diff iff @p diff
+/// is a function_type_diff node that carries a local change.
+const function_type_diff*
+is_function_type_diff_with_local_changes(const diff* diff)
+{
+  if (const function_type_diff* d = is_function_type_diff(diff))
+    if (d->has_local_changes())
+      return d;
+
+  return 0;
+}
+
 /// Test if a diff node is about differences between variables.
 ///
 /// @param diff the diff node to test.
@@ -740,7 +757,8 @@ is_qualified_type_diff(const diff* diff)
 {return dynamic_cast<const qualified_type_diff*>(diff);}
 
 /// Test if a diff node is either a reference diff node or a pointer
-/// diff node.
+/// diff node.  Note that this function also works on diffs of
+/// typedefs of reference or pointer.
 ///
 /// @param diff the diff node to test.
 ///
@@ -748,7 +766,42 @@ is_qualified_type_diff(const diff* diff)
 /// pointer diff node.
 bool
 is_reference_or_pointer_diff(const diff* diff)
-{return is_reference_diff(diff) || is_pointer_diff(diff);}
+{
+  diff = peel_typedef_diff(diff);
+  return is_reference_diff(diff) || is_pointer_diff(diff);
+}
+
+/// Test if a diff node is a reference or pointer diff node to a
+/// change that is neither basic type change nor distinct type change.
+///
+/// Note that this function also works on diffs of typedefs of
+/// reference or pointer.
+///
+/// @param diff the diff node to consider.
+///
+/// @return true iff @p diff is a eference or pointer diff node to a
+/// change that is neither basic type change nor distinct type change.
+bool
+is_reference_or_ptr_diff_to_non_basic_nor_distinct_types(const diff* diff)
+{
+  diff = peel_typedef_diff(diff);
+  if (const reference_diff* d = is_reference_diff(diff))
+    {
+      diff = peel_reference_diff(d);
+      if (is_diff_of_basic_type(diff) || is_distinct_diff(diff))
+	return false;
+      return true;
+    }
+  else if (const pointer_diff *d = is_pointer_diff(diff))
+    {
+      diff = peel_pointer_diff(d);
+      if (is_diff_of_basic_type(diff) || is_distinct_diff(diff))
+	return false;
+      return true;
+    }
+
+  return false;
+}
 
 /// Test if a diff node is about differences between two function
 /// parameters.
@@ -10913,9 +10966,10 @@ struct redundancy_marking_visitor : public diff_node_visitor
 	     || d->get_canonical_diff()->is_traversing())
 	    && d->has_changes())
 	  {
-	    // But if two diff nodes are redundant sibbling, do not
-	    // mark them as being redundant.  This is to avoid marking
-	    // nodes as redundant in this case:
+	    // But if two diff nodes are redundant sibbling that carry
+	    // changes of base types, do not mark them as being
+	    // redundant.  This is to avoid marking nodes as redundant
+	    // in this case:
 	    //
 	    //     int foo(int a, int b);
 	    // compared with:
@@ -10948,7 +11002,10 @@ struct redundancy_marking_visitor : public diff_node_visitor
 		    sib = f->type_diff().get();
 		  if (sib == d)
 		    continue;
-		  if (sib->get_canonical_diff() == d->get_canonical_diff())
+		  if (sib->get_canonical_diff() == d->get_canonical_diff()
+		      // Sibbling diff nodes that carry base type
+		      // changes ar to be marked as redundant.
+		      && (is_base_diff(sib) || is_distinct_diff(sib)))
 		    {
 		      redundant_with_sibling_node = true;
 		      break;
@@ -10966,7 +11023,7 @@ struct redundancy_marking_visitor : public diff_node_visitor
 		// redundant because otherwise one could miss important
 		// similar local changes that are applied to different
 		// functions.
-		&& !dynamic_cast<function_type_diff*>(d)
+		&& !is_function_type_diff_with_local_changes(d)
 		// Changes involving variadic parameters of functions
 		// should never be marked redundant because we want to see
 		// them all.
@@ -10985,9 +11042,13 @@ struct redundancy_marking_visitor : public diff_node_visitor
 		// that must be marked redundant.
 		&& d->context()->diff_has_been_visited(d) != d
 		// If the diff node is a function parameter and is not
-		// a reference/pointer then do not mark it as
+		// a reference/pointer (to a non basic or a non
+		// distinct type diff) then do not mark it as
 		// redundant.
-		&& (is_reference_or_pointer_diff(d)
+		//
+		// Children nodes of base class diff nodes are never
+		// redundant either, we want to see them all.
+		&& (is_reference_or_ptr_diff_to_non_basic_nor_distinct_types(d)
 		    || (!is_child_node_of_function_parm_diff(d)
 			&& !is_child_node_of_base_diff(d))))
 	      {
@@ -11312,6 +11373,25 @@ is_diff_of_basic_type(const diff* diff, bool allow_indirect_type)
   return is_diff_of_basic_type(diff);
 }
 
+/// If a diff node is about changes between two typedef types, get the
+/// diff node about changes between the underlying types.
+///
+/// Note that this function walks the tree of underlying diff nodes
+/// returns the first diff node about types that are not typedefs.
+///
+/// @param dif the dif node to consider.
+///
+/// @return the underlying diff node of @p dif, or just return @p dif
+/// if it's not a typedef diff node.
+const diff*
+peel_typedef_diff(const diff* dif)
+{
+  const typedef_diff *d = 0;
+  while ((d = is_typedef_diff(dif)))
+    dif = d->underlying_type_diff().get();
+  return dif;
+}
+
 /// If a diff node is about changes between two pointer types, get the
 /// diff node about changes between the underlying (pointed-to) types.
 ///

tests/data/Makefile.am

@@ -1135,6 +1135,13 @@ test-diff-suppr/libtest36-leaf-v1.so \
 test-diff-suppr/test36-leaf-report-0.txt \
 test-diff-suppr/test36-leaf-v0.cc \
 test-diff-suppr/test36-leaf-v1.cc \
+test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h \
+test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h \
+test-diff-suppr/test37-opaque-type-report-0.txt \
+test-diff-suppr/test37-opaque-type-v0.c \
+test-diff-suppr/test37-opaque-type-v0.o \
+test-diff-suppr/test37-opaque-type-v1.c \
+test-diff-suppr/test37-opaque-type-v1.o \
 \
 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-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h

@@ -0,0 +1,24 @@
+typedef struct opaque_struct * opaque_struct_pointer_type;
+
+typedef struct public_struct_type *public_struct_pointer_type;
+typedef struct public_struct_type2 *public_struct_pointer_type2;
+
+typedef void (*FuncPointerType0) (public_struct_pointer_type,
+				  public_struct_pointer_type);
+
+typedef void (*FuncPointerType1) (public_struct_pointer_type, int);
+
+typedef struct public_struct_type2
+{
+  FuncPointerType0 m0;
+  FuncPointerType1 m1;
+} public_struct_type2;
+
+typedef struct public_struct_type
+{
+  opaque_struct_pointer_type m0;
+  public_struct_type2 *m1;
+} public_struct_type;
+
+void
+bar0(public_struct_pointer_type *p);

tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h

@@ -0,0 +1,24 @@
+typedef struct opaque_struct * opaque_struct_pointer_type;
+
+typedef struct public_struct_type *public_struct_pointer_type;
+typedef struct public_struct_type2 *public_struct_pointer_type2;
+
+typedef void (*FuncPointerType0) (public_struct_pointer_type,
+				  public_struct_pointer_type);
+
+typedef void (*FuncPointerType1) (public_struct_pointer_type, int);
+
+typedef struct public_struct_type2
+{
+  FuncPointerType0 m0;
+  FuncPointerType1 m1;
+} public_struct_type2;
+
+typedef struct public_struct_type
+{
+  opaque_struct_pointer_type m0;
+  public_struct_type2 *m1;
+} public_struct_type;
+
+void
+bar0(public_struct_pointer_type *p);

tests/data/test-diff-suppr/test37-opaque-type-report-0.txt

@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+

tests/data/test-diff-suppr/test37-opaque-type-v0.c

@@ -0,0 +1,22 @@
+/*
+ * Compile this test with:
+ *   gcc -c -g test37-opaque-type-v0.c
+ *
+ *   This test exhibits changes that are redundant in a weird way.
+ *   The redundant path through the diff graph involves typedefs,
+ *   function types and function parameter diff nodes.
+ *
+ */
+#include "test37-opaque-type-header-dir/test37-opaque-type-header-v0.h"
+
+struct opaque_struct
+{
+  int m0;
+  int m1;
+  struct public_struct_type *m2;
+};
+
+void
+bar0(public_struct_pointer_type *p  __attribute__((unused)))
+{
+}

tests/data/test-diff-suppr/test37-opaque-type-v1.c

@@ -0,0 +1,24 @@
+/*
+ * Compile this test with:
+ *   gcc -c -g test37-opaque-type-v1.c
+ *
+ *   This test exhibits changes that are redundant in a weird way.
+ *   The redundant path through the diff graph involves typedefs,
+ *   function types and function parameter diff nodes.
+ *
+ */
+
+#include "test37-opaque-type-header-dir/test37-opaque-type-header-v1.h"
+
+struct opaque_struct
+{
+  int m0;
+  int m1;
+  struct public_struct_type *m2;
+  char m3;
+};
+
+void
+bar0(public_struct_pointer_type *p  __attribute__((unused)))
+{
+}

tests/test-diff-suppr.cc

@@ -1698,6 +1698,16 @@ InOutSpec in_out_specs[] =
     "data/test-diff-suppr/test36-leaf-report-0.txt",
     "output/test-diff-suppr/test36-leaf-report-0.txt"
   },
+  {
+    "data/test-diff-suppr/test37-opaque-type-v0.o",
+    "data/test-diff-suppr/test37-opaque-type-v1.o",
+    "data/test-diff-suppr/test37-opaque-type-header-dir",
+    "data/test-diff-suppr/test37-opaque-type-header-dir",
+    "",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test37-opaque-type-report-0.txt",
+    "output/test-diff-suppr/test37-opaque-type-report-0.txt"
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL}
 };