Fix redundancy marking for change of types used directly

If a type T is used directly (i.e, not through a pointer or reference)
as a function parameter or as a base class, a change in T should never
be marked as redundant in that context.  Otherwise, the change in that
context might be filtered out, possibly hiding real ABI incompatible
changes.

This patch implements this policy.

Also, it turned out in some circumstances, we where marking the first
visited diff node of a given class of equivalence of nodes as being
redundant, while we should only mark the *subsequently* visited nodes
of that class of equivalence as visited.  The patch also fixes that.

	* include/abg-comparison.h (pointer_map): Make this be a map of
	{size_t, size_t} pairs, rather than {size_t, bool}, so that each
	pointer in the map can be associated to another one.
	(diff_context::diff_has_been_visited): Return the pointer to the
	first diff node of the equivalence class that has been visited.
	* src/abg-comparison.cc (is_pointer_diff, is_reference_diff)
	(is_reference_or_pointer_diff, is_fn_parm_diff, is_base_diff)
	(is_child_node_of_function_parm_diff, is_child_node_of_base_diff):
	Define new static functions.
	(diff_context::diff_has_been_visited): Return the pointer to the
	first diff node of the equivalence class that has been visited.
	(diff_context::mark_diff_as_visited): Save the pointer to the
	first diff node of a given class of equivalence that has been
	visited.
	(redundancy_marking_visitor::visit_begin): If a diff node is a
	child node of a function parameter diff or base diff node and if
	it's not a pointer or reference diff node, then do not mark it as
	redundant.  Also, make sure to not mark the first diff node of a
	given class of equivalence that has been visited, as redundant;
	only the other subsequent nodes should be marked redundant; we
	were hitting this case because of an optimization that makes
	equivalent class diff nodes to share their private (pimpl) data.
	* tests/data/test-diff-filter/test29-finer-redundancy-marking-v{0,1}.o:
	New test input binaries.
	* tests/data/test-diff-filter/test29-finer-redundancy-marking-v{0,1}.cc:
	Source code of the new test input binaries above.
	* tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt:
	New test input.
	* tests/data/Makefile.am: Add the new test material above to the
	source distribution.
	* tests/test-diff-filter.cc (in_out_specs): Make this test harness
	run over the additional test input above.
	* tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt: Adjust.

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

include/abg-comparison.h

@@ -117,8 +117,9 @@ class class_diff;
 typedef shared_ptr<class_diff> class_diff_sptr;
 
 /// Convenience typedef for a map of pointer values.  The Key is a
-/// pointer value and the value is a boolean.
-typedef unordered_map<size_t, bool> pointer_map;
+/// pointer value and the value is potentially another pointer value
+/// associated to the first one.
+typedef unordered_map<size_t, size_t> pointer_map;
 
 /// Convenience typedef for a map which key is a string and which
 /// value is a @ref decl_base_sptr.
@@ -934,10 +935,10 @@ public:
   void
   initialize_canonical_diff(const diff_sptr diff);
 
-  bool
+  diff*
   diff_has_been_visited(const diff*) const;
 
-  bool
+  diff_sptr
   diff_has_been_visited(const diff_sptr) const;
 
   void

src/abg-comparison.cc

@@ -339,6 +339,82 @@ is_function_decl_diff(const diff* diff)
   return d;
 }
 
+/// Test if a diff node is about differences between two pointers.
+///
+/// @param diff the diff node to consider.
+///
+/// @return the @p diff converted into an instance of @ref
+/// pointer_diff iff @p diff is about differences between two
+/// pointers.
+static const pointer_diff*
+is_pointer_diff(const diff* diff)
+{return dynamic_cast<const pointer_diff*>(diff);}
+
+/// Test if a diff node is about differences between two references.
+///
+/// @param diff the diff node to consider.
+///
+/// @return the @p diff converted into an instance of @ref
+/// reference_diff iff @p diff is about differences between two
+/// references.
+static const reference_diff*
+is_reference_diff(const diff* diff)
+{return dynamic_cast<const reference_diff*>(diff);}
+
+/// Test if a diff node is either a reference diff node or a pointer
+/// diff node.
+///
+/// @param diff the diff node to test.
+///
+/// @return true iff @p diff is either reference diff node or a
+/// pointer diff node.
+static bool
+is_reference_or_pointer_diff(const diff* diff)
+{return is_reference_diff(diff) || is_pointer_diff(diff);}
+
+/// Test if a diff node is about differences between two function
+/// parameters.
+///
+/// @param diff the diff node to consider.
+///
+/// @return the @p diff converted into an instance of @ref
+/// reference_diff iff @p diff is about differences between two
+/// function parameters.
+static const fn_parm_diff*
+is_fn_parm_diff(const diff* diff)
+{return dynamic_cast<const fn_parm_diff*>(diff);}
+
+/// Test if a diff node is about differences between two base class
+/// specifiers.
+///
+/// @param diff the diff node to consider.
+///
+/// @return the @p diff converted into an instance of @ref base_diff
+/// iff @p diff is about differences between two base class
+/// specifiers.
+static const base_diff*
+is_base_diff(const diff* diff)
+{return dynamic_cast<const base_diff*>(diff);}
+
+/// Test if a diff node is a child node of a function parameter diff node.
+///
+/// @param diff the diff node to test.
+///
+/// @return true iff @p diff is a child node of a function parameter
+/// diff node.
+static bool
+is_child_node_of_function_parm_diff(const diff* diff)
+{return diff && is_fn_parm_diff(diff->parent_node());}
+
+/// Test if a diff node is a child node of a base diff node.
+///
+/// @param diff the diff node to test.
+///
+/// @return true iff @p diff is a child node of a base diff node.
+static bool
+is_child_node_of_base_diff(const diff* diff)
+{return diff && is_base_diff(diff->parent_node());}
+
 /// The default traverse function.
 ///
 /// @return true.
@@ -3160,39 +3236,53 @@ diff_context::initialize_canonical_diff(const diff_sptr diff)
 /// Test if a diff node has been traversed.
 ///
 /// @param d the diff node to consider.
-bool
+///
+/// @return the first diff node against which @p d is redundant.
+diff*
 diff_context::diff_has_been_visited(const diff* d) const
 {
   const diff* canonical = d->get_canonical_diff();
   assert(canonical);
 
   size_t ptr_value = reinterpret_cast<size_t>(canonical);
-  return (priv_->visited_diff_nodes_.find(ptr_value)
-	  != priv_->visited_diff_nodes_.end());
+  pointer_map::iterator it = priv_->visited_diff_nodes_.find(ptr_value);
+  if (it != priv_->visited_diff_nodes_.end())
+    return reinterpret_cast<diff*>(it->second);
+  else
+    return 0;
 }
 
 /// Test if a diff node has been traversed.
 ///
 /// @param d the diff node to consider.
-bool
+///
+/// @return the first diff node against which @p d is redundant.
+diff_sptr
 diff_context::diff_has_been_visited(const diff_sptr d) const
-{return diff_has_been_visited(d.get());}
+{
+  diff_sptr diff(diff_has_been_visited(d.get()));
+  return diff;
+}
 
 /// Mark a diff node as traversed by a traversing algorithm.
 ///
 /// Actually, it's the @ref CanonicalDiff "canonical diff" of this
 /// node that is marked as traversed.
 ///
-/// Subsequent invocations of diff_has_been_visited() on the diff
-/// node will yield true.
+/// Subsequent invocations of diff_has_been_visited() on the diff node
+/// will yield true.
 void
 diff_context::mark_diff_as_visited(const diff* d)
 {
+  if(diff_has_been_visited(d))
+    return;
+
   const diff* canonical = d->get_canonical_diff();
   assert(canonical);
 
-   size_t ptr_value = reinterpret_cast<size_t>(canonical);
-   priv_->visited_diff_nodes_[ptr_value] = true;
+   size_t canonical_ptr_value = reinterpret_cast<size_t>(canonical);
+   size_t diff_ptr_value = reinterpret_cast<size_t>(d);;
+   priv_->visited_diff_nodes_[canonical_ptr_value] = diff_ptr_value;
 }
 
 /// Unmark all the diff nodes that were marked as being traversed.
@@ -13346,7 +13436,19 @@ struct redundancy_marking_visitor : public diff_node_visitor
 		// should never be marked redundant because we want to see
 		// them all.
 		&& !is_diff_of_variadic_parameter(d)
-		&& !is_diff_of_variadic_parameter_type(d))
+		&& !is_diff_of_variadic_parameter_type(d)
+		// If the *same* diff node (not one that is merely
+		// equivalent to this one) has already been visited
+		// the do not mark it as beind redundant.  It's only
+		// the other nodes that are equivalent to this one
+		// 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
+		// redundant.
+		&& (is_reference_or_pointer_diff(d)
+		    || (!is_child_node_of_function_parm_diff(d)
+			&& !is_child_node_of_base_diff(d))))
 	      {
 		d->add_to_category(REDUNDANT_CATEGORY);
 		// As we said in preamble, as this node is marked as

tests/data/Makefile.am

@@ -403,6 +403,11 @@ test-diff-filter/test28-redundant-and-filtered-children-nodes-report-0.txt \
 test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt \
 test-diff-filter/test28-redundant-and-filtered-children-nodes-v0.cc \
 test-diff-filter/test28-redundant-and-filtered-children-nodes-v1.cc \
+test-diff-filter/test29-finer-redundancy-marking-report-0.txt \
+test-diff-filter/test29-finer-redundancy-marking-v0.o  \
+test-diff-filter/test29-finer-redundancy-marking-v1.cc \
+test-diff-filter/test29-finer-redundancy-marking-v0.cc \
+test-diff-filter/test29-finer-redundancy-marking-v1.o \
 \
 test-diff-suppr/test0-type-suppr-v0.cc	\
 test-diff-suppr/test0-type-suppr-v1.cc	\

tests/data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt

@@ -0,0 +1,28 @@
+Functions changes summary: 0 Removed, 3 Changed, 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+3 functions with some indirect sub-type change:
+
+  [C]'function void bar(base&)' has some indirect sub-type changes:
+    parameter 1 of type 'base&' has sub-type changes:
+      in referenced type 'struct base':
+        type size changed from 32 to 64 bits
+        1 data member insertion:
+          'char base::m1', at offset 32 (in bits)
+
+  [C]'function void baz(base)' has some indirect sub-type changes:
+    parameter 1 of type 'struct base' has sub-type changes:
+      details were reported earlier
+
+  [C]'function void foo(inherited*)' has some indirect sub-type changes:
+    parameter 1 of type 'inherited*' has sub-type changes:
+      in pointed to type 'struct inherited':
+        type size changed from 64 to 96 bits
+        1 base class change:
+          'struct base' changed:
+            details were reported earlier
+
+        1 data member change:
+         'int inherited::m0' offset changed from 32 to 64 (in bits)
+
+

tests/data/test-diff-filter/test29-finer-redundancy-marking-v0.cc

@@ -0,0 +1,24 @@
+// Compile this with:
+//   g++ -g -Wall -c test29-finer-redundancy-marking-v0.cc
+
+struct base
+{
+  int m0;
+};
+
+struct inherited : public base
+{
+  int m0;
+};
+
+void
+bar(base&)
+{}
+
+void
+baz(base)
+{}
+
+void
+foo(inherited*)
+{}

tests/data/test-diff-filter/test29-finer-redundancy-marking-v1.cc

@@ -0,0 +1,25 @@
+// Compile this with:
+//   g++ -g -Wall -c test29-finer-redundancy-marking-v1.cc
+
+struct base
+{
+  int m0;
+  char m1;
+};
+
+struct inherited : public base
+{
+  int m0;
+};
+
+void
+bar(base&)
+{}
+
+void
+baz(base)
+{}
+
+void
+foo(inherited*)
+{}

tests/data/test-diff-suppr/test5-fn-suppr-report-0.txt

@@ -1,7 +1,7 @@
-Functions changes summary: 0 Removed, 1 Changed (1 filtered out), 0 Added function
+Functions changes summary: 0 Removed, 2 Changed, 0 Added functions
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 
-1 function with some indirect sub-type change:
+2 functions with some indirect sub-type change:
 
   [C]'function void bar(S*)' has some indirect sub-type changes:
     parameter 1 of type 'S*' has sub-type changes:
@@ -12,4 +12,8 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
         1 data member change:
          'int S::m1' offset changed from 0 to 32 (in bits)
 
+  [C]'function void bar(int, S)' has some indirect sub-type changes:
+    parameter 2 of type 'struct S' has sub-type changes:
+      details were reported earlier
+
 

tests/test-diff-filter.cc

@@ -362,6 +362,13 @@ InOutSpec in_out_specs[] =
    "data/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt",
     "output/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt",
   },
+  {
+    "data/test-diff-filter/test29-finer-redundancy-marking-v0.o",
+    "data/test-diff-filter/test29-finer-redundancy-marking-v1.o",
+    "--no-linkage-name --no-redundant",
+    "data/test-diff-filter/test29-finer-redundancy-marking-report-0.txt",
+    "output/test-diff-filter/test29-finer-redundancy-marking-report-0.txt",
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL}
 };