[applied] comp-filter,ir: Simplify logic of has_harmless_name_change

Message ID 871puk5dyj.fsf@redhat.com
State New
Headers
Series [applied] comp-filter,ir: Simplify logic of has_harmless_name_change |

Commit Message

Dodji Seketeli March 26, 2025, 9:38 a.m. UTC
  Hello,

While looking into something else, I noticed that
has_harmless_name_change was overly complicated because it open-codes
things that should be done by types_are_compatible.

This patch removes the duplicated work from has_harmless_name_change
and augments types_are_compatible as needed.

The end result of this patch is neutral (as expected) on the
non-regression test suite.

	* include/abg-fwd.h (decl_name_changed)
	(integral_type_has_harmless_name_change): Declare new functions.
	* include/abg-ir.h (equals_modulo_cv_qualifier): Declare new
	overloads for array_type_def_pstr and pointer_type_def_sptr.
	* src/abg-comp-filter.cc
	(type_diff_has_typedef_cv_qual_change_only): Rename
	type_diff_has_cv_qual_change_only into this for better legibility.
	Use equals_modulo_cv_qualifier rather than poorly open-coding its
	functionality.
	(has_fn_parm_type_cv_qual_change)
	(has_fn_return_type_cv_qual_change, has_var_type_cv_qual_change):
	Adjust the call to the new
	type_diff_has_typedef_cv_qual_change_only.
	(is_compatible_type_change): Declare new static function.
	(is_non_compatible_distinct_change): Remove
	type_diff_has_cv_qual_change_only and use the more generic
	is_compatible_type_change.
	(has_harmless_name_change): Remove the redundant and too much low
	level messing up with typedefs, CV qualifiers and harmless
	integral name changes.  types_are_compatible should now take care
	of that minutiae.
	(decl_name_changed, integral_type_has_harmless_name_change): Move
	these static function ...
	* src/abg-ir.cc (decl_name_changed)
	(integral_type_has_harmless_name_change): ... here and make them
	non-static.
	(types_are_compatible): Simplify logic.  Look through qualified,
	typedef, pointers, reference and array types.
	(equals_modulo_cv_qualifier): Define new overloads for
	array_type_def_sptr and pointer_type_def_sptr.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to the mainline.
---
 include/abg-fwd.h      |  15 +++
 include/abg-ir.h       |  11 +++
 src/abg-comp-filter.cc | 154 ++++++++---------------------
 src/abg-ir.cc          | 220 ++++++++++++++++++++++++++++++++++++++---
 4 files changed, 270 insertions(+), 130 deletions(-)
  

Patch

diff --git a/include/abg-fwd.h b/include/abg-fwd.h
index 150fe2b0..f65f3b20 100644
--- a/include/abg-fwd.h
+++ b/include/abg-fwd.h
@@ -1647,6 +1647,21 @@  find_first_data_member_matching_regexp(const class_or_union& t,
 var_decl_sptr
 find_last_data_member_matching_regexp(const class_or_union& t,
 				      const regex::regex_t_sptr& regex);
+
+bool
+decl_name_changed(const type_or_decl_base* a1, const type_or_decl_base *a2);
+
+bool
+decl_name_changed(const type_or_decl_base_sptr& d1,
+		  const type_or_decl_base_sptr& d2);
+
+bool
+integral_type_has_harmless_name_change(const decl_base_sptr& f,
+				       const decl_base_sptr& s);
+
+bool
+integral_type_has_harmless_name_change(const type_base_sptr& f,
+				       const type_base_sptr& s);
 } // end namespace ir
 
 using namespace abigail::ir;
diff --git a/include/abg-ir.h b/include/abg-ir.h
index 899f0231..a19a1b80 100644
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -3052,6 +3052,17 @@  var_equals_modulo_types(const var_decl&, const var_decl&, change_kind*);
 bool
 equals_modulo_cv_qualifier(const array_type_def*, const array_type_def*);
 
+bool
+equals_modulo_cv_qualifier(const array_type_def_sptr& l,
+			   const array_type_def_sptr& r);
+
+bool
+equals_modulo_cv_qualifier(const pointer_type_def*, const pointer_type_def*);
+
+bool
+equals_modulo_cv_qualifier(const pointer_type_def_sptr&,
+			   const pointer_type_def_sptr&);
+
 /// Abstracts a variable declaration.
 class var_decl : public virtual decl_base
 {
diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index ce04d978..6735580d 100644
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -34,11 +34,11 @@  has_offset_changes(const string_decl_base_sptr_map& f_data_members,
 		   const string_decl_base_sptr_map& s_data_members);
 
 static bool
-type_diff_has_cv_qual_change_only(const diff *type_dif);
+type_diff_has_typedef_cv_qual_change_only(const diff *type_dif);
 
 static bool
-type_diff_has_cv_qual_change_only(const type_base_sptr& f,
-				  const type_base_sptr& s);
+type_diff_has_typedef_cv_qual_change_only(const type_base_sptr& f,
+					  const type_base_sptr& s);
 
 static bool
 has_harmful_enum_change(const diff* diff);
@@ -636,6 +636,24 @@  is_compatible_change(const decl_base_sptr& d1, const decl_base_sptr& d2)
   return false;
 }
 
+/// Test if a diff node carries a compatible type change.
+///
+/// @param d the diff node to consider.
+///
+/// @return true iff @p carries a compatible type change.
+static bool
+is_compatible_type_change(const diff* d)
+{
+  if (!d)
+    return false;
+
+  if (type_base_sptr t1 = is_type(d->first_subject()))
+    if (type_base_sptr t2 = is_type(d->second_subject()))
+      return types_are_compatible(t1, t2);
+
+  return false;
+}
+
 /// Test if a diff node carries a non-compatible change between two
 /// types of different kinds.
 ///
@@ -653,7 +671,7 @@  is_non_compatible_distinct_change(const diff *d)
   if (const distinct_diff* dd = is_distinct_diff(d))
     {
       if (dd->compatible_child_diff()
-	  || type_diff_has_cv_qual_change_only(d)
+	  || is_compatible_type_change(d)
 	  || (!dd->first_subject() || !dd->second_subject()))
 	// The distinct diff node carries a compatible or benign
 	// change
@@ -667,46 +685,6 @@  is_non_compatible_distinct_change(const diff *d)
   return false;
 }
 
-/// Test if two decls have different names.
-///
-/// @param d1 the first declaration to consider.
-///
-/// @param d2 the second declaration to consider.
-///
-/// @return true if d1 and d2 have different names.
-static bool
-decl_name_changed(const type_or_decl_base* a1, const type_or_decl_base *a2)
-{
-  string d1_name, d2_name;
-
-  const decl_base *d1 = dynamic_cast<const decl_base*>(a1);
-  if (d1 == 0)
-    return false;
-
-  const decl_base *d2 = dynamic_cast<const decl_base*>(a2);
-  if (d2 == 0)
-    return false;
-
-  if (d1)
-    d1_name = d1->get_qualified_name();
-  if (d2)
-    d2_name = d2->get_qualified_name();
-
-  return d1_name != d2_name;
-}
-
-/// Test if two decls have different names.
-///
-/// @param d1 the first declaration to consider.
-///
-/// @param d2 the second declaration to consider.
-///
-/// @return true if d1 and d2 have different names.
-static bool
-decl_name_changed(const type_or_decl_base_sptr& d1,
-		  const type_or_decl_base_sptr& d2)
-{return decl_name_changed(d1.get(), d2.get());}
-
 /// Test if a diff node carries a changes in which two decls have
 /// different names.
 ///
@@ -718,42 +696,6 @@  static bool
 decl_name_changed(const diff *d)
 {return decl_name_changed(d->first_subject(), d->second_subject());}
 
-/// Test if a diff node carries a change whereby two integral types
-/// have different names in a harmless way.
-///
-/// Basically, if the integral type name change is accompanied by a
-/// size change then the change is considered harmful.  If there are
-/// modifiers change, the change is considered harmful.
-static bool
-integral_type_has_harmless_name_change(const decl_base_sptr& f,
-				       const decl_base_sptr& s)
-{
-  if ((is_integral_type(f) || f->get_name().empty())
-      && (is_integral_type(s) || s->get_name().empty())
-      && decl_name_changed(f, s)
-      && (is_type(f)->get_size_in_bits()
-	  == is_type(s)->get_size_in_bits())
-      && (is_type(f)->get_alignment_in_bits()
-	  == is_type(s)->get_alignment_in_bits()))
-    {
-      real_type fi, si;
-      ABG_ASSERT(f->get_name().empty()
-		 || parse_real_type(f->get_name(), fi));
-      ABG_ASSERT(s->get_name().empty()
-		 || parse_real_type(s->get_name(), si));
-
-      if (fi.get_base_type() == si.get_base_type()
-	  && fi.get_modifiers() != si.get_modifiers())
-	// The base type hasn't changed.  That means only modifiers
-	// changed.  This is considered has harmful by default.
-	return false;
-
-      return true;
-    }
-
-  return false;
-}
-
 /// Test if two decls represents a harmless name change.
 ///
 /// For now, a harmless name change is considered only for a typedef,
@@ -784,27 +726,18 @@  has_harmless_name_change(const decl_base_sptr& f,
 		&& s->get_is_anonymous_or_has_anonymous_parent())
 	       && tools_utils::decl_names_equal(f->get_qualified_name(),
 						s->get_qualified_name()))
-	      // ... a typedef name change, without having the
-	      // underlying type changed ...
-	      || (is_typedef(f)
-		  && is_typedef(s)
-		  && (is_typedef(f)->get_underlying_type()
-		   == is_typedef(s)->get_underlying_type()))
-	      // ... Types are compatible (equal modulo a typedef) ...
+	      // ... Types are compatible (equal modulo a typedef or
+	      // cv quals) ...
 	      || (is_type(f)
 		  && is_type(s)
 		  && types_are_compatible(is_type(f), is_type(s)))
-	      // ... Only qualifers changed on the type without having
-	      // the underlying type changed ...
-	      || type_diff_has_cv_qual_change_only(is_type(f), is_type(s))
 	      || has_harmless_enum_change(is_type(f), is_type(s), ctxt)
 	      // ... or a data member name change, without having its
 	      // type changed ...
 	      || (is_data_member(f)
 		  && is_data_member(s)
 		  && (is_var_decl(f)->get_type()
-		      == is_var_decl(s)->get_type()))
-	      || integral_type_has_harmless_name_change(f, s)));
+		      == is_var_decl(s)->get_type()))));
 }
 
 /// Test if two decls represent a harmful name change.
@@ -1951,7 +1884,7 @@  has_fn_parm_type_top_cv_qual_change(const diff* diff)
 ///
 /// @return true iff the type_diff carries a CV qualifier only change.
 static bool
-type_diff_has_cv_qual_change_only(const diff *type_dif)
+type_diff_has_typedef_cv_qual_change_only(const diff *type_dif)
 {
   if (!type_dif)
     return false;
@@ -1959,19 +1892,22 @@  type_diff_has_cv_qual_change_only(const diff *type_dif)
   type_base_sptr f = is_type(type_dif->first_subject());
   type_base_sptr s = is_type(type_dif->second_subject());
 
-  return type_diff_has_cv_qual_change_only(f, s);
+  return type_diff_has_typedef_cv_qual_change_only(f, s);
 }
 
 /// Test if a type only carries a CV qualifier-only change.
 ///
+/// Note that for pointers and array types, the functions look at
+/// pointed-to types for comparison.
+///
 /// @param f the first version of the type.
 ///
 /// @param s the second version of the type.
 ///
 /// @return true iff the change is only a qualifier change.
 static bool
-type_diff_has_cv_qual_change_only(const type_base_sptr& f,
-				  const type_base_sptr& s)
+type_diff_has_typedef_cv_qual_change_only(const type_base_sptr& f,
+					  const type_base_sptr& s)
 {
   type_base_sptr a = f;
   type_base_sptr b = s;
@@ -1983,29 +1919,17 @@  type_diff_has_cv_qual_change_only(const type_base_sptr& f,
     return true;
 
   if (is_pointer_type(a) && is_pointer_type(b))
-    {
-      a = peel_pointer_type(a);
-      b = peel_pointer_type(b);
-    }
-
-  if (a && b && *a == *b)
-    return true;
-
-  a = peel_qualified_or_typedef_type(a);
-  b = peel_qualified_or_typedef_type(b);
-
-  if (a && b && *a == *b)
-    return true;
+    return equals_modulo_cv_qualifier(is_pointer_type(a), is_pointer_type(b));
 
   // If f and s are arrays, note that they can differ only by the cv
   // qualifier of the array element type.  That cv qualifier is not
   // removed by peel_qualified_type.  So we need to test this case
   // specifically.
-  if (array_type_def *f_a = is_array_type(a.get()))
-    if (array_type_def *s_a = is_array_type(b.get()))
+  if (array_type_def_sptr f_a = is_array_type(a))
+    if (array_type_def_sptr s_a = is_array_type(b))
       return equals_modulo_cv_qualifier(f_a, s_a);
 
-  return (a && b && *a == *b);
+  return false;
 }
 
 /// Test if an @ref fn_parm_diff node has a cv qualifier change on the
@@ -2029,7 +1953,7 @@  has_fn_parm_type_cv_qual_change(const diff* dif)
     return false;
 
   const diff *type_dif = parm_diff->type_diff().get();
-  return type_diff_has_cv_qual_change_only(type_dif);
+  return type_diff_has_typedef_cv_qual_change_only(type_dif);
 }
 
 /// Test if a function type or decl diff node carries a CV
@@ -2053,7 +1977,7 @@  has_fn_return_type_cv_qual_change(const diff* dif)
     return false;
 
   const diff* return_type_diff = fn_type_diff->return_type_diff().get();
-  return type_diff_has_cv_qual_change_only(return_type_diff);
+  return type_diff_has_typedef_cv_qual_change_only(return_type_diff);
 }
 
 /// Test if a function type or decl diff node carries a function
@@ -2101,7 +2025,7 @@  has_var_type_cv_qual_change(const diff* dif)
   if (!type_dif)
     return false;
 
-  return type_diff_has_cv_qual_change_only(type_dif);
+  return type_diff_has_typedef_cv_qual_change_only(type_dif);
 }
 
 /// Test if a type change is a "void pointer to pointer" change.
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 51d2847d..b86eb9a6 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -10227,7 +10227,11 @@  get_type_declaration(const type_base_sptr t)
 /// Type A and B are compatible if
 ///
 ///	- A and B are equal
+///	- or A and B are integral types with harmless name change
 ///	- or if one type is a typedef of the other one.
+///	- or if one type is the CV qualified version of the other
+///	- or if A and B are pointers, references or arrays of
+///	  compatible types
 ///
 /// @param type1 the first type to consider.
 ///
@@ -10235,29 +10239,55 @@  get_type_declaration(const type_base_sptr t)
 ///
 /// @return true iff @p type1 and @p type2 are compatible.
 bool
-types_are_compatible(const type_base_sptr type1,
-		     const type_base_sptr type2)
+types_are_compatible(const type_base_sptr type1, const type_base_sptr type2)
 {
   if (!type1 || !type2)
     return false;
 
-  if (type1 == type2)
+  if (type1 == type2 || *type1 == *type2)
     return true;
 
-  // Normally we should strip typedefs entirely, but this is
-  // potentially costly, especially on binaries with huge changesets
-  // like the Linux Kernel.  So we just get the leaf types for now.
-  //
-  // Maybe there should be an option by which users accepts to pay the
-  // CPU usage toll in exchange for finer filtering?
+  type_base_sptr t1 = peel_qualified_or_typedef_type(type1);
+  type_base_sptr t2 = peel_qualified_or_typedef_type(type2);
 
-  // type_base_sptr t1 = strip_typedef(type1);
-  // type_base_sptr t2 = strip_typedef(type2);
+  if (t1 && t2 && *t1 == *t2)
+    return true;
 
-  type_base_sptr t1 = peel_typedef_type(type1);
-  type_base_sptr t2 = peel_typedef_type(type2);
+  if (integral_type_has_harmless_name_change(t1, t2))
+    return true;
+
+  if (is_pointer_type(t1) && is_pointer_type(t2))
+    {
+      t1 = is_pointer_type(t1)->get_pointed_to_type();
+      t2 = is_pointer_type(t2)->get_pointed_to_type();
+      return types_are_compatible(t1, t2);
+    }
 
-  return t1 == t2;
+  if (is_reference_type(t1) && is_reference_type(t2))
+    {
+      t1 = is_reference_type(t1)->get_pointed_to_type();
+      t2 = is_reference_type(t2)->get_pointed_to_type();
+      return types_are_compatible(t1, t2);
+    }
+
+  if (is_array_type(t1) && is_array_type(t2))
+    {
+      array_type_def_sptr a1 = is_array_type(t1);
+      array_type_def_sptr a2 =is_array_type(t2);
+      type_base_sptr e1 = a1->get_element_type();
+      type_base_sptr e2 = a2->get_element_type();
+      e1 = peel_qualified_or_typedef_type(e1);
+      e2 = peel_qualified_or_typedef_type(e2);
+
+      if ((a1->get_size_in_bits() != a2->get_size_in_bits())
+	  || (a1->get_dimension_count() != a2->get_dimension_count())
+	  || !types_are_compatible(e1, e2))
+	return false;
+
+      return true;
+    }
+
+  return false;
 }
 
 /// Test if two types are equal modulo a typedef.
@@ -19664,7 +19694,7 @@  equals(const array_type_def& l, const array_type_def& r, change_kind* k)
   ABG_RETURN(result);
 }
 
-/// Test if two variables are equals modulo CV qualifiers.
+/// Test if two array types are equals modulo CV qualifiers.
 ///
 /// @param l the first array of the comparison.
 ///
@@ -19708,6 +19738,72 @@  equals_modulo_cv_qualifier(const array_type_def* l, const array_type_def* r)
   return true;
 }
 
+/// Test if two array types are equals modulo CV qualifiers.
+///
+/// @param l the first array of the comparison.
+///
+/// @param r the second array of the comparison.
+///
+/// @return true iff @p l equals @p r or, if they are different, the
+/// difference between the too is just a matter of CV qualifiers.
+bool
+equals_modulo_cv_qualifier(const array_type_def_sptr& l,
+			   const array_type_def_sptr& r)
+{return equals_modulo_cv_qualifier(l.get(), r.get());}
+
+/// Test if two pointer types are equals modulo CV qualifiers.
+///
+/// @param l the first pointer of the comparison.
+///
+/// @param r the second pointer of the comparison.
+///
+/// @return true iff @p l equals @p r or, if they are different, the
+/// difference between the too is just a matter of CV qualifiers.
+bool
+equals_modulo_cv_qualifier(const pointer_type_def* l, const pointer_type_def* r)
+{
+  if (l == r)
+    return true;
+
+  if (!l || !r)
+    ABG_RETURN_FALSE;
+
+  type_base_sptr l_ptt = l->get_pointed_to_type(),
+    r_ptt = r->get_pointed_to_type();
+
+  do
+    {
+      l_ptt = peel_qualified_or_typedef_type(l_ptt);
+      r_ptt = peel_qualified_or_typedef_type(r_ptt);
+
+      l_ptt = is_pointer_type(l_ptt)
+	? is_pointer_type(l_ptt)->get_pointed_to_type()
+	: l_ptt;
+
+      r_ptt = is_pointer_type(r_ptt)
+	? is_pointer_type(r_ptt)->get_pointed_to_type()
+	: r_ptt;
+    } while (is_pointer_type(l_ptt) && is_pointer_type(r_ptt));
+
+  l_ptt = peel_qualified_or_typedef_type(l_ptt);
+  r_ptt = peel_qualified_or_typedef_type(r_ptt);
+
+  return *l_ptt == *r_ptt;
+}
+
+/// Test if two pointer types are equals modulo CV qualifiers.
+///
+/// @param l the first pointer of the comparison.
+///
+/// @param r the second pointer of the comparison.
+///
+/// @return true iff @p l equals @p r or, if they are different, the
+/// difference between the too is just a matter of CV qualifiers.
+bool
+equals_modulo_cv_qualifier(const pointer_type_def_sptr& l,
+			   const pointer_type_def_sptr& r)
+{return equals_modulo_cv_qualifier(l.get(), r.get());}
+
 /// Get the language of the array.
 ///
 /// @return the language of the array.
@@ -29336,6 +29432,100 @@  add_outer_ptr_to_mbr_type_expr(const ptr_to_mbr_type* p,
   return result;
 }
 
+/// Test if two decls have different names.
+///
+/// Note that this function takes into account decls whose names are
+/// relevant from an ABI standpoint.  For instance, function parameter
+/// names are not relevant in that context.
+///
+/// @param d1 the first declaration to consider.
+///
+/// @param d2 the second declaration to consider.
+///
+/// @return true if d1 and d2 have different names.
+bool
+decl_name_changed(const type_or_decl_base* a1, const type_or_decl_base *a2)
+{
+  string d1_name, d2_name;
+
+  const decl_base *d1 = dynamic_cast<const decl_base*>(a1);
+  if (d1 == 0)
+    return false;
+
+  const decl_base *d2 = dynamic_cast<const decl_base*>(a2);
+  if (d2 == 0)
+    return false;
+
+  if (is_function_parameter(d1) || is_function_parameter(d2))
+    // Name changes for fn parms are irrelevant.
+    return false;
+
+  d1_name = d1->get_qualified_name();
+  d2_name = d2->get_qualified_name();
+
+  return d1_name != d2_name;
+}
+
+/// Test if two decls have different names.
+///
+/// @param d1 the first declaration to consider.
+///
+/// @param d2 the second declaration to consider.
+///
+/// @return true if d1 and d2 have different names.
+bool
+decl_name_changed(const type_or_decl_base_sptr& d1,
+		  const type_or_decl_base_sptr& d2)
+{return decl_name_changed(d1.get(), d2.get());}
+
+/// Test if a diff node carries a change whereby two integral types
+/// have different names in a harmless way.
+///
+/// Basically, if the integral type name change is accompanied by a
+/// size change then the change is considered harmful.  If there are
+/// modifiers change, the change is considered harmful.
+bool
+integral_type_has_harmless_name_change(const type_base_sptr& f,
+				       const type_base_sptr& s)
+{
+  if (is_decl(f)
+      && is_decl(s)
+      && (is_integral_type(f) || is_decl(f)->get_name().empty())
+      && (is_integral_type(s) || is_decl(s)->get_name().empty())
+      && decl_name_changed(is_decl(f), is_decl(s))
+      && (f->get_size_in_bits() == s->get_size_in_bits())
+      && (f->get_alignment_in_bits() == s->get_alignment_in_bits()))
+    {
+      real_type fi, si;
+      ABG_ASSERT(is_decl(f)->get_name().empty()
+		 || parse_real_type(is_decl(f)->get_name(), fi));
+      ABG_ASSERT(is_decl(s)->get_name().empty()
+		 || parse_real_type(is_decl(s)->get_name(), si));
+
+      if (fi.get_base_type() == si.get_base_type()
+	  && fi.get_modifiers() != si.get_modifiers())
+	// The base type hasn't changed.  That means only modifiers
+	// changed.  This is considered has harmful by default.
+	return false;
+
+      return true;
+    }
+
+  return false;
+}
+
+/// Test if a diff node carries a change whereby two integral types
+/// have different names in a harmless way.
+///
+/// Basically, if the integral type name change is accompanied by a
+/// size change then the change is considered harmful.  If there are
+/// modifiers change, the change is considered harmful.
+bool
+integral_type_has_harmless_name_change(const decl_base_sptr& f,
+				       const decl_base_sptr& s)
+{return integral_type_has_harmless_name_change(is_type(f), is_type(s));}
+
+
 /// When constructing the name of a pointer to mebmer type, add the
 /// return type to the left of the existing type identifier, and the
 /// parameters declarator to the right.