abg-ir.cc: Improve types_have_similar_structure.

This function is used to determine whether or not type differences are
"local" and is primarily used in --leaf-changes-only mode. The logic
has some issues which are addressed by this patch:

    - Any number of points-to (*) and refers-to (& and &&) components
      are peeled off the types being compared, rather than checking
      these match in number and kind.
    - This peeling is done with peel_typedef_pointer_or_reference_type
      which also peels any number of CV qualifiers (OK) and
      array-of ([N]) type components (not OK).
    - The function sets a state variable (was_indirect_type) to modify
      the behaviour of downstream comparisons, but this cannot be
      passed through recursive calls.

The effect of the indirect_type flag is to switch to comparisons by
name: identically named structs don't get introspected. Arguably, a
more useful behaviour for --leaf-changes-only mode would be to treat
any change to a named type as non-local, except in the context of the
definition of that type itself. This would be a more significant
change in behaviour.

	* include/abg-fwd.h (types_have_similar_structure): In both
	overloads, add an indirect_type argument, defaulting to
	false.
	* src/abg-ir.cc (reference_type_def constructor): Tabify.
	(types_have_similar_structure): In both overloads, add an
	indirect_type argument and update documentation text. In the
	type_base_sptr overload, pass indirect_type in the tail
	call. In the type_base* overload, replace was_indirect_type
	with indirect_type; peel CV qualifiers and typedefs without
	testing as the peel function does this; replace the
	indiscriminate peeling of qualifier/pointer/reference/array
	type components with code that checks the same
	pointer/reference/array type component is used on each side
	and makes recursive calls with indirect_type set to true; pass
	the indirect_type argument recursively when comparing other
	subtypes; move the typeid check earlier, document its purpose
	and remove unneccessary checks after later dynamic casts;
	remove an always-true conditional; add a TODO for comparing
	array types more accurately.
	* tests/data/Makefile.am: Add new test case files.
	* tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New
	test case.
	* tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto.
	* tests/data/test-abidiff-exit/test-leaf-peeling-report.txt:
	Ditto.
	* tests/test-abidiff-exit.cc: Run new test case.

Signed-off-by: Giuliano Procida <gprocida@google.com>

include/abg-fwd.h

@@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s);
 
 bool
 types_have_similar_structure(const type_base_sptr& first,
-			     const type_base_sptr& second);
+			     const type_base_sptr& second,
+			     bool indirect_type = false);
 
 bool
 types_have_similar_structure(const type_base* first,
-			     const type_base* second);
+			     const type_base* second,
+			     bool indirect_type = false);
 
 } // end namespace ir
 

src/abg-ir.cc

@@ -22563,33 +22563,14 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s)
 /// Test if two types have similar structures, even though they are
 /// (or can be) different.
 ///
-/// Two indirect types (pointers, references, qualified, typedefs)
-/// have similar structure if their underlying types are of the same
-/// kind and have the same name.  In this indirect types case, the
-/// size of the underlying type does not matter.
+/// const and volatile qualifiers are completely ignored.
 ///
-/// Two direct types (i.e, non indirect) have a similar structure if
-/// they have the same kind, name and size.  Two class types have
-/// similar structure if they have the same name, size, and if their
-/// data members have similar types.
+/// typedef are resolved to their definitions; their names are ignored.
 ///
-/// @param first the first type to consider.
-///
-/// @param second the second type to consider.
-///
-/// @return true iff @p first and @p second have similar structures.
-bool
-types_have_similar_structure(const type_base_sptr& first,
-			     const type_base_sptr& second)
-{return types_have_similar_structure(first.get(), second.get());}
-
-/// Test if two types have similar structures, even though they are
-/// (or can be) different.
-///
-/// Two indirect types (pointers, references, qualified, typedefs)
-/// have similar structure if their underlying types are of the same
-/// kind and have the same name.  In this indirect types case, the
-/// size of the underlying type does not matter.
+/// Two indirect types (pointers or references) have similar structure
+/// if their underlying types are of the same kind and have the same
+/// name.  In the indirect types case, the size of the underlying type
+/// does not matter.
 ///
 /// Two direct types (i.e, non indirect) have a similar structure if
 /// they have the same kind, name and size.  Two class types have
@@ -22600,9 +22581,43 @@ types_have_similar_structure(const type_base_sptr& first,
 ///
 /// @param second the second type to consider.
 ///
+/// @param indirect_type whether to do an indirect comparison
+///
 /// @return true iff @p first and @p second have similar structures.
 bool
-types_have_similar_structure(const type_base* first, const type_base* second)
+types_have_similar_structure(const type_base_sptr& first,
+			     const type_base_sptr& second,
+			     bool indirect_type)
+{return types_have_similar_structure(first.get(), second.get(), indirect_type);}
+
+/// Test if two types have similar structures, even though they are
+/// (or can be) different.
+///
+/// const and volatile qualifiers are completely ignored.
+///
+/// typedef are resolved to their definitions; their names are ignored.
+///
+/// Two indirect types (pointers or references) have similar structure
+/// if their underlying types are of the same kind and have the same
+/// name.  In the indirect types case, the size of the underlying type
+/// does not matter.
+///
+/// Two direct types (i.e, non indirect) have a similar structure if
+/// they have the same kind, name and size.  Two class types have
+/// similar structure if they have the same name, size, and if the
+/// types of their data members have similar types.
+///
+/// @param first the first type to consider.
+///
+/// @param second the second type to consider.
+///
+/// @param indirect_type whether to do an indirect comparison
+///
+/// @return true iff @p first and @p second have similar structures.
+bool
+types_have_similar_structure(const type_base* first,
+			     const type_base* second,
+			     bool indirect_type)
 {
   if (!!first != !!second)
     return false;
@@ -22610,33 +22625,39 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (!first)
     return false;
 
-  if (is_typedef(first) || is_qualified_type(first))
-    first = peel_qualified_or_typedef_type(first);
-
-  if (is_typedef(second) || is_qualified_type(second))
-    second = peel_qualified_or_typedef_type(second);
-
-  bool was_indirect_type = (is_pointer_type(first)
-			    || is_pointer_type(second)
-			    || is_reference_type(first)
-			    || is_reference_type(second));
-
-  if (was_indirect_type)
-    {
-      first = peel_typedef_pointer_or_reference_type(first);
-      second = peel_typedef_pointer_or_reference_type(second);
-    }
+  // Treat typedefs purely as type aliases and ignore CV-qualifiers.
+  first = peel_qualified_or_typedef_type(first);
+  second = peel_qualified_or_typedef_type(second);
 
+  // Eliminate all but N of the N^2 comparison cases. This also guarantees the
+  // various ty2 below cannot be null.
   if (typeid(*first) != typeid(*second))
     return false;
 
+  // Peel off matching pointers.
+  if (const pointer_type_def* ty1 = is_pointer_type(first))
+    {
+      const pointer_type_def* ty2 = is_pointer_type(second);
+      return types_have_similar_structure(ty1->get_pointed_to_type(),
+					  ty2->get_pointed_to_type(),
+					  true);
+    }
+
+  // Peel off matching references.
+  if (const reference_type_def* ty1 = is_reference_type(first))
+    {
+      const reference_type_def* ty2 = is_reference_type(second);
+      if (ty1->is_lvalue() != ty2->is_lvalue())
+	return false;
+      return types_have_similar_structure(ty1->get_pointed_to_type(),
+					  ty2->get_pointed_to_type(),
+					  true);
+    }
+
   if (const type_decl* ty1 = is_type_decl(first))
     {
       const type_decl* ty2 = is_type_decl(second);
-      if (ty2 == 0)
-	return false;
-
-      if (!was_indirect_type)
+      if (!indirect_type)
 	if (ty1->get_size_in_bits() != ty2->get_size_in_bits())
 	  return false;
 
@@ -22646,10 +22667,7 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (const enum_type_decl* ty1 = is_enum_type(first))
     {
       const enum_type_decl* ty2 = is_enum_type(second);
-      if (ty2 == 0)
-	return false;
-
-      if (!was_indirect_type)
+      if (!indirect_type)
 	if (ty1->get_size_in_bits() != ty2->get_size_in_bits())
 	  return false;
 
@@ -22660,14 +22678,11 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (const class_decl* ty1 = is_class_type(first))
     {
       const class_decl* ty2 = is_class_type(second);
-      if (ty2 == 0)
-	return false;
-
       if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous()
 	  && ty1->get_name() != ty2->get_name())
 	return false;
 
-      if (!was_indirect_type)
+      if (!indirect_type)
 	{
 	  if ((ty1->get_size_in_bits() != ty2->get_size_in_bits())
 	      || (ty1->get_non_static_data_members().size()
@@ -22684,7 +22699,8 @@ types_have_similar_structure(const type_base* first, const type_base* second)
 	      var_decl_sptr dm1 = *i;
 	      var_decl_sptr dm2 = *j;
 	      if (!types_have_similar_structure(dm1->get_type().get(),
-						dm2->get_type().get()))
+						dm2->get_type().get(),
+						indirect_type))
 		return false;
 	    }
 	}
@@ -22695,14 +22711,11 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (const union_decl* ty1 = is_union_type(first))
     {
       const union_decl* ty2 = is_union_type(second);
-      if (ty2 == 0)
-	return false;
-
       if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous()
 	  && ty1->get_name() != ty2->get_name())
 	return false;
 
-      if (!was_indirect_type)
+      if (!indirect_type)
 	return ty1->get_size_in_bits() == ty2->get_size_in_bits();
 
       return true;
@@ -22711,13 +22724,13 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (const array_type_def* ty1 = is_array_type(first))
     {
       const array_type_def* ty2 = is_array_type(second);
-
-      if (!was_indirect_type)
-	if (ty1->get_size_in_bits() != ty2->get_size_in_bits()
-	    || ty1->get_dimension_count() != ty2->get_dimension_count()
-	    || !types_have_similar_structure(ty1->get_element_type(),
-					     ty2->get_element_type()))
-	  return false;
+      // TODO: Handle uint32_t[10] vs uint64_t[5] better.
+      if (ty1->get_size_in_bits() != ty2->get_size_in_bits()
+	  || ty1->get_dimension_count() != ty2->get_dimension_count()
+	  || !types_have_similar_structure(ty1->get_element_type(),
+					   ty2->get_element_type(),
+					   indirect_type))
+	return false;
 
       return true;
     }
@@ -22725,14 +22738,12 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (const array_type_def::subrange_type *ty1 = is_subrange_type(first))
     {
       const array_type_def::subrange_type *ty2 = is_subrange_type(second);
-      if (!ty2)
-	return false;
-
       if (ty1->get_upper_bound() != ty2->get_upper_bound()
 	  || ty1->get_lower_bound() != ty2->get_lower_bound()
 	  || ty1->get_language() != ty2->get_language()
 	  || !types_have_similar_structure(ty1->get_underlying_type(),
-					  ty2->get_underlying_type()))
+					   ty2->get_underlying_type(),
+					   indirect_type))
 	return false;
 
       return true;
@@ -22741,11 +22752,9 @@ types_have_similar_structure(const type_base* first, const type_base* second)
   if (const function_type* ty1 = is_function_type(first))
     {
       const function_type* ty2 = is_function_type(second);
-      if (!ty2)
-	return false;
-
       if (!types_have_similar_structure(ty1->get_return_type(),
-					ty2->get_return_type()))
+					ty2->get_return_type(),
+					indirect_type))
 	return false;
 
       if (ty1->get_parameters().size() != ty2->get_parameters().size())
@@ -22758,7 +22767,8 @@ types_have_similar_structure(const type_base* first, const type_base* second)
 	    && j != ty2->get_parameters().end());
 	   ++i, ++j)
 	if (!types_have_similar_structure((*i)->get_type(),
-					  (*j)->get_type()))
+					  (*j)->get_type(),
+					  indirect_type))
 	  return false;
 
       return true;

tests/data/Makefile.am

@@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \
 test-abidiff-exit/test-leaf3-v1.c \
 test-abidiff-exit/test-leaf3-v1.o \
 test-abidiff-exit/test-leaf3-report.txt \
+test-abidiff-exit/test-leaf-peeling-v0.cc \
+test-abidiff-exit/test-leaf-peeling-v0.o \
+test-abidiff-exit/test-leaf-peeling-v1.cc \
+test-abidiff-exit/test-leaf-peeling-v1.o \
+test-abidiff-exit/test-leaf-peeling-report.txt \
 \
 test-diff-dwarf/test0-v0.cc		\
 test-diff-dwarf/test0-v0.o			\

tests/data/test-abidiff-exit/test-leaf-peeling-report.txt

@@ -0,0 +1,38 @@
+Leaf changes summary: 4 artifacts changed
+Changed leaf types summary: 4 leaf types changed
+Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
+Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
+
+'struct foo at test-leaf-peeling.0.cc:1:1' changed:
+  type size changed from 32 to 64 (in bits)
+  there are data member changes:
+    type 'int' of 'foo::z' changed:
+      type name changed from 'int' to 'long int'
+      type size changed from 32 to 64 (in bits)
+    and size changed from 32 to 64 (in bits) (by +32 bits)
+
+
+
+'struct ops1 at test-leaf-peeling.0.cc:5:1' changed:
+  type size hasn't changed
+  there are data member changes:
+    type 'int*' of 'ops1::x' changed:
+      pointer type changed from: 'int*' to: 'int**'
+
+
+
+
+'struct ops2 at test-leaf-peeling.0.cc:9:1' changed:
+  type size changed from 320 to 640 (in bits)
+  there are data member changes:
+    'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits)
+
+
+
+'struct ops3 at test-leaf-peeling.0.cc:13:1' changed:
+  type size hasn't changed
+  there are data member changes:
+    type 'void (int&)*' of 'ops3::spong' changed:
+      pointer type changed from: 'void (int&)*' to: 'void (int&&)*'
+
+

tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc

@@ -0,0 +1,24 @@
+struct foo {
+  int z;
+};
+
+struct ops1 {
+  int * x;
+};
+
+struct ops2 {
+  foo y[10];
+};
+
+struct ops3 {
+  void (*spong)(int & wibble);
+};
+
+void register_ops1(ops1*) {
+}
+
+void register_ops2(ops2*) {
+}
+
+void register_ops3(ops3*) {
+}

tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc

@@ -0,0 +1,24 @@
+struct foo {
+  long z;
+};
+
+struct ops1 {
+  int ** x;
+};
+
+struct ops2 {
+  foo y[10];
+};
+
+struct ops3 {
+  void (*spong)(int && wibble);
+};
+
+void register_ops1(ops1*) {
+}
+
+void register_ops2(ops2*) {
+}
+
+void register_ops3(ops3*) {
+}

tests/test-abidiff-exit.cc

@@ -158,6 +158,15 @@ InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/test-leaf3-report.txt",
     "output/test-abidiff-exit/test-leaf3-report.txt"
   },
+  {
+    "data/test-abidiff-exit/test-leaf-peeling-v0.o",
+    "data/test-abidiff-exit/test-leaf-peeling-v1.o",
+    "",
+    "--leaf-changes-only",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+    "data/test-abidiff-exit/test-leaf-peeling-report.txt",
+    "output/test-abidiff-exit/test-leaf-peeling-report.txt"
+  },
   {0, 0, 0 ,0,  abigail::tools_utils::ABIDIFF_OK, 0, 0}
 };