public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
From: Matthias Maennich <maennich@google.com>
To: Giuliano Procida <gprocida@google.com>
Cc: libabigail@sourceware.org, dodji@seketeli.org, kernel-team@android.com
Subject: Re: [PATCH v2] abg-ir.cc: Improve types_have_similar_structure.
Date: Thu, 26 Mar 2020 12:38:28 +0100	[thread overview]
Message-ID: <20200326113828.GD153882@google.com> (raw)
In-Reply-To: <20200325141859.93896-1-gprocida@google.com>

Hi!

On Wed, Mar 25, 2020 at 02:18:59PM +0000, Android Kernel Team wrote:
>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                             |   6 +-
> src/abg-ir.cc                                 | 147 ++++++++++--------
> tests/data/Makefile.am                        |   5 +
> .../test-leaf-peeling-report.txt              |  38 +++++
> .../test-abidiff-exit/test-leaf-peeling-v0.cc |  24 +++
> .../test-abidiff-exit/test-leaf-peeling-v0.o  | Bin 0 -> 3704 bytes
> .../test-abidiff-exit/test-leaf-peeling-v1.cc |  24 +++
> .../test-abidiff-exit/test-leaf-peeling-v1.o  | Bin 0 -> 3752 bytes
> tests/test-abidiff-exit.cc                    |   9 ++
> 9 files changed, 182 insertions(+), 71 deletions(-)
> create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
> create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
> create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o
> create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
> create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o
>
>diff --git a/include/abg-fwd.h b/include/abg-fwd.h
>index 6f2a862c..1aab70a6 100644
>--- a/include/abg-fwd.h
>+++ b/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
>
>diff --git a/src/abg-ir.cc b/src/abg-ir.cc
>index 2853fe6c..a10b0bb7 100644
>--- a/src/abg-ir.cc
>+++ b/src/abg-ir.cc
>@@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr	pointed_to,
>       decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to);
>       string name;
>       if (pto)
>-        {
>-          set_visibility(pto->get_visibility());
>-          name = string(pto->get_name()) + "&";
>-        }
>+	{
>+	  set_visibility(pto->get_visibility());
>+	  name = string(pto->get_name()) + "&";
>+	}

Correct, but unrelated. No complaints. :-)

>       else
> 	name = string(get_type_name(is_function_type(pointed_to),
> 				    /*qualified_name=*/true)) + "&";
>@@ -22563,33 +22563,44 @@ 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.
>+///
>+/// 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 their
>-/// data members have similar types.
>+/// 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_sptr& first,
>-			     const type_base_sptr& second)
>-{return types_have_similar_structure(first.get(), second.get());}
>+			     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.
> ///
>-/// 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.
>+///
>+/// 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
>@@ -22600,9 +22611,13 @@ 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* 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);
>+  // Treat typedefs purely as type aliases and ignore CV-qualifiers.
>+  first = peel_qualified_or_typedef_type(first);
>+  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));
>+  // 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;

Just curious (because I think it is correct): Does it make a measurable
difference?

>
>-  if (was_indirect_type)
>+  // Peel off matching pointers.
>+  if (const pointer_type_def* ty1 = is_pointer_type(first))

A picky compiler might ask you to add some parentheses here.  [-Wparentheses]

>     {
>-      first = peel_typedef_pointer_or_reference_type(first);
>-      second = peel_typedef_pointer_or_reference_type(second);
>+      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);
>     }
>
>-  if (typeid(*first) != typeid(*second))
>-    return false;
>+  // Peel off matching references.
>+  if (const reference_type_def* ty1 = is_reference_type(first))

As above.

>+    {
>+      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,20 +22678,16 @@ 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 (!was_indirect_type)
>-	    if ((ty1->get_size_in_bits() != ty2->get_size_in_bits())
>-		|| (ty1->get_non_static_data_members().size()
>-		    != ty2->get_non_static_data_members().size()))
>-	      return false;
>+	  if ((ty1->get_size_in_bits() != ty2->get_size_in_bits())
>+	      || (ty1->get_non_static_data_members().size()
>+		  != ty2->get_non_static_data_members().size()))
>+	    return false;
>
> 	  for (class_or_union::data_members::const_iterator
> 		 i = ty1->get_non_static_data_members().begin(),
>@@ -22685,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;
> 	    }
> 	}
>@@ -22696,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;
>@@ -22712,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;
>     }
>@@ -22726,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;
>@@ -22742,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())
>@@ -22759,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;
>diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
>index cf7792f1..6d4e2faa 100644
>--- a/tests/data/Makefile.am
>+++ b/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			\
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
>new file mode 100644
>index 00000000..54a26869
>--- /dev/null
>+++ b/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&&)*'
>+
>+
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
>new file mode 100644
>index 00000000..b927d15e
>--- /dev/null
>+++ b/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*) {
>+}
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o
>new file mode 100644
>index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638
>GIT binary patch
>literal 3704
>zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y
>z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm>
>zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S
>zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf
>zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p
>z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi
>z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL
>z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ
>z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR
>z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k
>zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h
>zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%*
>z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld#
>zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81)
>z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d
>zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A)
>z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv
>zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d<
>zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q;
>zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?<
>zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me
>z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U
>z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H
>z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w
>zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m-
>V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va
>
>literal 0
>HcmV?d00001
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
>new file mode 100644
>index 00000000..7c88df9f
>--- /dev/null
>+++ b/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];
>+};

There is no apparent difference in ops2 between the test files. What are
we testing?

>+
>+struct ops3 {
>+  void (*spong)(int && wibble);
>+};
>+
>+void register_ops1(ops1*) {
>+}
>+
>+void register_ops2(ops2*) {
>+}
>+
>+void register_ops3(ops3*) {
>+}

Maybe we can add some more test cases:

 - compare reference types and pointer types
 - compare int*** and int* (two levels of peeling)
 - compare uint32_t[10] and uint64_t[5] (with the same TODO as in the code)
 - nested types, e.g. struct ops1_v0* vs. struct ops1_v1*

Nice work. I like it!

Cheers,
Matthias
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o
>new file mode 100644
>index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640
>GIT binary patch
>literal 3752
>zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l
>zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C*
>z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8
>zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra
>zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y
>zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X|
>z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ
>z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD
>z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg
>zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR(
>z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz
>z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT
>zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f
>zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2
>z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8
>zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP
>zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e#
>z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N
>zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$
>zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%}
>z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1
>zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L`
>z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW
>zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI
>zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA
>e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z
>
>literal 0
>HcmV?d00001
>
>diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
>index 6ecbfd24..ebac7a00 100644
>--- a/tests/test-abidiff-exit.cc
>+++ b/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}
> };
>
>-- 
>2.25.1.696.g5e7596f4ac-goog
>
>

  reply	other threads:[~2020-03-26 11:38 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 13:29 [PATCH] " Giuliano Procida
2020-03-25 14:13 ` Giuliano Procida
2020-03-25 14:18 ` [PATCH v2] " Giuliano Procida
2020-03-26 11:38   ` Matthias Maennich [this message]
2020-03-26 12:14     ` Giuliano Procida
2020-03-26 12:46       ` Matthias Maennich
2020-03-26 13:36   ` Dodji Seketeli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200326113828.GD153882@google.com \
    --to=maennich@google.com \
    --cc=dodji@seketeli.org \
    --cc=gprocida@google.com \
    --cc=kernel-team@android.com \
    --cc=libabigail@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).