Bug 24787 - Filter out enum changes into compatible integer types

Libabigail's filtering engine fails to recognize an enum changing into
a compatible integer (or vice versa) as a harmless change.

This patch fixes that.

	* include/abg-comparison.h (peel_typedef_or_qualified_type_diff):
	Declare new function.
	(peel_pointer_or_qualified_type_diff): Rename
	peel_pointer_or_qualified_type into this.
	* include/abg-fwd.h (is_enum_type): Declare a new overload for
	type_or_decl_base*.
	* src/abg-comp-filter.cc (has_harmless_enum_to_int_change): Define
	new static function.
	* src/abg-comparison.cc (categorize_harmless_diff_node): Use the
	new has_harmless_enum_to_int_change here.
	(peel_pointer_or_qualified_type_diff): Renamed
	peel_pointer_or_qualified_type into this.
	(is_diff_of_basic_type): Adjust.
	(peel_typedef_or_qualified_type_diff): Define new function.
	* test-diff-filter/PR24787-lib{one, two}.so: New test input
	binaries.
	* test-diff-filter/PR24787-{one, two}.c: Source files of the test
	input binaries above.
	* test-diff-filter/PR24787-report-0.txt: Test output reference.
	* tests/data/Makefile.am: Add the new testing material to source
	distribution.
	* tests/test-diff-filter.cc (in_out_specs): Add the new test to
	the test harness.

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

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index 66392b97dce5577142174682e35e989a408215cc..8537a43590344fb9b07105f3f0b49a3752a50a56 100644 (file)
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -2799,7 +2799,10 @@ const diff*
 peel_qualified_diff(const diff* dif);
 
 const diff*
-peel_pointer_or_qualified_type(const diff*dif);
+peel_pointer_or_qualified_type_diff(const diff* dif);
+
+const diff*
+peel_typedef_or_qualified_type_diff(const diff* dif);
 }// end namespace comparison
 
 }// end namespace abigail

diff --git a/include/abg-fwd.h b/include/abg-fwd.h
index 3a717436616e563567f6ad473f7148e26e7f135f..ec37af62246da6c03ddb80aaf3fd0661ec90e3b3 100644 (file)
--- a/include/abg-fwd.h
+++ b/include/abg-fwd.h
@@ -423,6 +423,9 @@ is_typedef(type_base*);
 enum_type_decl_sptr
 is_enum_type(const type_or_decl_base_sptr&);
 
+const enum_type_decl*
+is_enum_type(const type_or_decl_base*);
+
 bool
 is_class_type(const type_or_decl_base&);
 

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index 5362b53664150efd04d405ff49c0abff36d25cf9..d412bca76e77140726cb157884abdf10cacd5cbc 100644 (file)
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -1035,6 +1035,52 @@ has_harmful_enum_change(const diff* diff)
   return false;
 }
 
+/// Test if a diff node carries a harmless change of an enum into an
+/// integer (or vice-versa).
+///
+/// The test takes into account the fact change we care about might be
+/// wrapped into a typedef or qualified type diff.
+///
+/// @param diff the diff node to consider.
+///
+/// @return true if @p diff is a harmless enum to integer change.
+static bool
+has_harmless_enum_to_int_change(const diff* diff)
+{
+  if (!diff)
+    return false;
+
+  diff = peel_typedef_or_qualified_type_diff(diff);
+
+  if (const distinct_diff *d = is_distinct_diff(diff))
+    {
+      const enum_type_decl *enum_type = 0;
+      const type_base *integer_type = 0;
+
+      type_base *first_type =
+       peel_qualified_or_typedef_type(is_type(d->first().get()));
+      type_base *second_type =
+       peel_qualified_or_typedef_type(is_type(d->second().get()));
+
+      if (const enum_type_decl *e = is_enum_type(first_type))
+       enum_type = e;
+      else if (const enum_type_decl *e = is_enum_type(second_type))
+       enum_type = e;
+
+      if (const type_base * i = is_type_decl(first_type))
+       integer_type = i;
+      else if (const type_base *i = is_type_decl(second_type))
+       integer_type = i;
+
+      if (enum_type
+         && integer_type
+         && enum_type->get_size_in_bits() == integer_type->get_size_in_bits())
+       return true;
+    }
+
+  return false;
+}
+
 /// Test if an @ref fn_parm_diff node has a top cv qualifier change on
 /// the type of the function parameter.
 ///
@@ -1385,8 +1431,9 @@ categorize_harmless_diff_node(diff *d, bool pre)
          || static_data_member_type_size_changed(f, s))
        category |= STATIC_DATA_MEMBER_CHANGE_CATEGORY;
 
-      if (has_enumerator_insertion(d)
-         && !has_harmful_enum_change(d))
+      if ((has_enumerator_insertion(d)
+          && !has_harmful_enum_change(d))
+         || has_harmless_enum_to_int_change(d))
        category |= HARMLESS_ENUM_CHANGE_CATEGORY;
 
       if (function_name_changed_but_not_symbol(d))

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index feae5f96eb9628d139a67fabc2f8447e3c573fc8..55033b09677b843bf5efd2c0ec12fc3172f4e19e 100644 (file)
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -11551,7 +11551,7 @@ const type_decl_diff*
 is_diff_of_basic_type(const diff* diff, bool allow_indirect_type)
 {
   if (allow_indirect_type)
-      diff = peel_pointer_or_qualified_type(diff);
+      diff = peel_pointer_or_qualified_type_diff(diff);
   return is_diff_of_basic_type(diff);
 }
 
@@ -11646,7 +11646,7 @@ peel_qualified_diff(const diff* dif)
 /// @return the underlying diff node of @p dif, or just return @p dif
 /// if it's not a pointer, reference or qualified diff node.
 const diff*
-peel_pointer_or_qualified_type(const diff*dif)
+peel_pointer_or_qualified_type_diff(const diff*dif)
 {
   while (true)
     {
@@ -11662,6 +11662,33 @@ peel_pointer_or_qualified_type(const diff*dif)
   return dif;
 }
 
+/// If a diff node is about changes between two typedefs or qualified
+/// 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 typedef or
+/// qualified types.
+///
+/// @param dif the dif node to consider.
+///
+/// @return the underlying diff node of @p dif, or just return @p dif
+/// if it's not typedef or qualified diff node.
+const diff*
+peel_typedef_or_qualified_type_diff(const diff *dif)
+{
+  while (true)
+    {
+      if (const typedef_diff *d = is_typedef_diff(dif))
+       dif = peel_typedef_diff(d);
+      else if (const qualified_type_diff *d = is_qualified_type_diff(dif))
+       dif = peel_qualified_diff(d);
+      else
+       break;
+    }
+  return dif;
+}
+
 /// Test if a diff node represents a diff between two class or union
 /// types.
 ///

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 982d32e5ddd1f5002817db29cc04e60991027c0f..3474b2eb083b3446f39032c34d003509ba7c3eb6 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -704,6 +704,11 @@ test-diff-filter/test-PR24731-v0.c \
 test-diff-filter/test-PR24731-v0.o \
 test-diff-filter/test-PR24731-v1.c \
 test-diff-filter/test-PR24731-v1.o \
+test-diff-filter/PR24787-libone.so \
+test-diff-filter/PR24787-libtwo.so \
+test-diff-filter/PR24787-one.c \
+test-diff-filter/PR24787-two.c \
+test-diff-filter/PR24787-report-0.txt \
 \
 test-diff-suppr/test0-type-suppr-v0.cc \
 test-diff-suppr/test0-type-suppr-v1.cc \

diff --git a/tests/data/test-diff-filter/PR24787-libone.so b/tests/data/test-diff-filter/PR24787-libone.so
new file mode 100755 (executable)
index 0000000..d86352b
Binary files /dev/null and b/tests/data/test-diff-filter/PR24787-libone.so differ

diff --git a/tests/data/test-diff-filter/PR24787-libtwo.so b/tests/data/test-diff-filter/PR24787-libtwo.so
new file mode 100755 (executable)
index 0000000..c18d26a
Binary files /dev/null and b/tests/data/test-diff-filter/PR24787-libtwo.so differ

diff --git a/tests/data/test-diff-filter/PR24787-one.c b/tests/data/test-diff-filter/PR24787-one.c
new file mode 100644 (file)
index 0000000..b37712d
--- /dev/null
+++ b/tests/data/test-diff-filter/PR24787-one.c
@@ -0,0 +1,13 @@
+#include <stdio.h>
+
+
+typedef enum FooFilter {
+    FOO_FILTER_ONE   = 1 << 0,
+    FOO_FILTER_TWO   = 1 << 1,
+    FOO_FILTER_THREE = 1 << 2,
+} FooFilter;
+
+
+void do_something(FooFilter filter_flags) {
+  printf("%d\n", filter_flags);
+}

diff --git a/tests/data/test-diff-filter/PR24787-report-0.txt b/tests/data/test-diff-filter/PR24787-report-0.txt
new file mode 100644 (file)
index 0000000..9666a8f
--- /dev/null
+++ b/tests/data/test-diff-filter/PR24787-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
+

diff --git a/tests/data/test-diff-filter/PR24787-two.c b/tests/data/test-diff-filter/PR24787-two.c
new file mode 100644 (file)
index 0000000..4308181
--- /dev/null
+++ b/tests/data/test-diff-filter/PR24787-two.c
@@ -0,0 +1,13 @@
+#include <stdio.h>
+
+
+typedef enum FooFilter {
+    FOO_FILTER_ONE   = 1 << 0,
+    FOO_FILTER_TWO   = 1 << 1,
+    FOO_FILTER_THREE = 1 << 2,
+} FooFilter;
+
+
+void do_something(unsigned int filter_flags) {
+  printf("%d\n", filter_flags);
+}

diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
index 04a1f677e55d6847a2b61c1623d702a2810fd289..f8879d3c7fd86aada6690fc4597f678061e4075a 100644 (file)
--- a/tests/test-diff-filter.cc
+++ b/tests/test-diff-filter.cc
@@ -563,6 +563,13 @@ InOutSpec in_out_specs[] =
     "data/test-diff-filter/test-PR24731-report-1.txt",
     "output/test-diff-filter/test-PR24731-report-1.txt",
   },
+  {
+    "data/test-diff-filter/PR24787-libone.so",
+    "data/test-diff-filter/PR24787-libtwo.so",
+    "--no-default-suppression",
+    "data/test-diff-filter/PR24787-report-0.txt",
+    "output/test-diff-filter/PR24787-report-0.txt",
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL}
 };