Bug 24731 - Wrongly reporting union members order change

When union data members are re-ordered, abidiff reports the
re-ordering as if it was a meaningful ABI change.

This patch teaches Libabigail to categorize that benign type layout
change as a HARMLESS_UNION_CHANGE_CATEGORY kind of change and ignore it.

	* include/abg-comp-filter.h (union_diff_has_harmless_changes):
	Declare new function and ...
	* src/abg-comp-filter.cc (union_diff_has_harmless_changes):
	... define it here.
	(categorize_harmless_diff_node): Use the new
	union_diff_has_harmless_changes here.
	* include/abg-comparison.h (HARMLESS_UNION_CHANGE_CATEGORY): Add a
	new enumerator to diff_category enum.  Adjust the value of the
	other enumerators.
	* src/abg-comparison.cc (get_default_harmless_categories_bitmap):
	Add the new HARMLESS_UNION_CHANGE_CATEGORY in here.
	(operator<<(ostream& o, diff_category c)): Support the new
	HARMLESS_UNION_CHANGE_CATEGORY.
	* tests/data/test-diff-filter/test-PR24731-report-0.txt: Likewise.
	* tests/data/test-diff-filter/test-PR24731-report-1.txt: Likewise.
	* tests/data/test-diff-filter/test-PR24731-v0.c: Likewise.
	* tests/data/test-diff-filter/test-PR24731-v0.o: Likewise.
	* tests/data/test-diff-filter/test-PR24731-v1.c: Likewise.
	* tests/data/test-diff-filter/test-PR24731-v1.o: Likewise.
	* tests/data/Makefile.am: Add the new test material above to
	source distribution.
	* tests/test-diff-filter.cc (in_out_spec): Add the new test input
	to this test harness.

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

diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h
index 966f6c2..2f7c15c 100644 (file)
--- a/include/abg-comp-filter.h
+++ b/include/abg-comp-filter.h
@@ -42,6 +42,8 @@ namespace filtering
 bool
 has_harmless_name_change(const decl_base_sptr& f, const decl_base_sptr& s);
 
+bool union_diff_has_harmless_changes(const diff *d);
+
 bool
 has_harmful_name_change(const decl_base_sptr& f, const decl_base_sptr& s);
 

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index cf2fa4e..378ce03 100644 (file)
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -372,59 +372,61 @@ enum diff_category
   /// alias change that is harmless.
   HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY = 1 << 6,
 
+  HARMLESS_UNION_CHANGE_CATEGORY = 1 << 7,
+
   /// This means that a diff node was marked as suppressed by a
   /// user-provided suppression specification.
-  SUPPRESSED_CATEGORY = 1 << 7,
+  SUPPRESSED_CATEGORY = 1 << 8,
 
   /// This means that a diff node was warked as being for a private
   /// type.  That is, the diff node is meant to be suppressed by a
   /// suppression specification that was auto-generated to filter out
   /// changes to private types.
-  PRIVATE_TYPE_CATEGORY = 1 << 8,
+  PRIVATE_TYPE_CATEGORY = 1 << 9,
 
   /// This means the diff node (or at least one of its descendant
   /// nodes) carries a change that modifies the size of a type or an
   /// offset of a type member.  Removal or changes of enumerators in a
   /// enum fall in this category too.
-  SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 9,
+  SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 10,
 
   /// This means that a diff node in the sub-tree carries an
   /// incompatible change to a vtable.
-  VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 10,
+  VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 11,
 
   /// A diff node in this category is redundant.  That means it's
   /// present as a child of a other nodes in the diff tree.
-  REDUNDANT_CATEGORY = 1 << 11,
+  REDUNDANT_CATEGORY = 1 << 12,
 
   /// This means that a diff node in the sub-tree carries a class type
   /// that was declaration-only and that is now defined, or vice
   /// versa.
-  CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 12,
+  CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 13,
 
   /// A diff node in this category is a function parameter type which
   /// top cv-qualifiers change.
-  FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 13,
+  FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 14,
 
   /// A diff node in this category has a function parameter type with a
   /// cv-qualifiers change.
-  FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 14,
+  FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 15,
 
   /// A diff node in this category is a function return type with a
   /// cv-qualifier change.
-  FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 15,
+  FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 16,
 
   /// A diff node in this category is for a variable which type holds
   /// a cv-qualifier change.
-  VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 16,
+  VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 17,
 
   /// A diff node in this category carries a change from void pointer
   /// to non-void pointer.
-  VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 17,
+  VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 18,
 
   /// A diff node in this category carries a change in the size of the
   /// array type of a global variable, but the ELF size of the
   /// variable didn't change.
-  BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 18,
+  BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19,
   /// A special enumerator that is the logical 'or' all the
   /// enumerators above.
   ///
@@ -438,6 +440,7 @@ enum diff_category
   | STATIC_DATA_MEMBER_CHANGE_CATEGORY
   | HARMLESS_ENUM_CHANGE_CATEGORY
   | HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY
+  | HARMLESS_UNION_CHANGE_CATEGORY
   | SUPPRESSED_CATEGORY
   | PRIVATE_TYPE_CATEGORY
   | SIZE_OR_OFFSET_CHANGE_CATEGORY

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index ed099c1..3134f5d 100644 (file)
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -1319,6 +1319,25 @@ has_benign_infinite_array_change(const diff* dif)
     }
   return false;
 }
+
+/// Test if a union diff node does have changes that don't impact its
+/// size.
+///
+/// @param d the union diff node to consider.
+///
+/// @return true iff @p d is a diff node which has changes that don't
+/// impact its size.
+bool
+union_diff_has_harmless_changes(const diff *d)
+{
+  if (is_union_diff(d)
+      && d->has_changes()
+      && !has_type_size_change(d))
+    return true;
+
+  return false;
+}
+
 /// Detect if the changes carried by a given diff node are deemed
 /// harmless and do categorize the diff node accordingly.
 ///
@@ -1355,6 +1374,9 @@ categorize_harmless_diff_node(diff *d, bool pre)
          || class_diff_has_harmless_odr_violation_change(d))
        category |= HARMLESS_DECL_NAME_CHANGE_CATEGORY;
 
+      if (union_diff_has_harmless_changes(d))
+       category |= HARMLESS_UNION_CHANGE_CATEGORY;
+
       if (has_non_virtual_mem_fn_change(d))
        category |= NON_VIRT_MEM_FUN_CHANGE_CATEGORY;
 

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 2cc3c69..feae5f9 100644 (file)
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -2921,6 +2921,7 @@ get_default_harmless_categories_bitmap()
          | abigail::comparison::STATIC_DATA_MEMBER_CHANGE_CATEGORY
          | abigail::comparison::HARMLESS_ENUM_CHANGE_CATEGORY
          | abigail::comparison::HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY
+         | abigail::comparison::HARMLESS_UNION_CHANGE_CATEGORY
          | abigail::comparison::CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY
          | abigail::comparison::FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY
          | abigail::comparison::FN_PARM_TYPE_CV_CHANGE_CATEGORY
@@ -3016,6 +3017,14 @@ operator<<(ostream& o, diff_category c)
       emitted_a_category |= true;
     }
 
+  if (c & HARMLESS_UNION_CHANGE_CATEGORY)
+    {
+      if (emitted_a_category)
+       o << "|";
+      o << "HARMLESS_UNION_CHANGE_CATEORY";
+      emitted_a_category |= true;
+    }
+
   if (c & SUPPRESSED_CATEGORY)
     {
       if (emitted_a_category)

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 3ad2bc7..982d32e 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -698,6 +698,12 @@ test-diff-filter/test47-filter-void-ptr-change-v1.o \
 test-diff-filter/PR24430-fold-qualified-array-clang \
 test-diff-filter/PR24430-fold-qualified-array-gcc \
 test-diff-filter/PR24430-fold-qualified-array-report-0.txt \
+test-diff-filter/test-PR24731-report-0.txt \
+test-diff-filter/test-PR24731-report-1.txt \
+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-suppr/test0-type-suppr-v0.cc \
 test-diff-suppr/test0-type-suppr-v1.cc \

diff --git a/tests/data/test-diff-filter/test-PR24731-report-0.txt b/tests/data/test-diff-filter/test-PR24731-report-0.txt
new file mode 100644 (file)
index 0000000..9666a8f
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR24731-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/test-PR24731-report-1.txt b/tests/data/test-diff-filter/test-PR24731-report-1.txt
new file mode 100644 (file)
index 0000000..d33e565
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR24731-report-1.txt
@@ -0,0 +1,13 @@
+Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+1 function with some indirect sub-type change:
+
+  [C]'function void test_func(u)' at test-PR24731-v1.c:3:1 has some indirect sub-type changes:
+    parameter 1 of type 'union u' has sub-type changes:
+      type size hasn't changed
+      type changed from:
+        union u{int a; char c; short int s;}
+      to:
+        union u{int a; short int s; char c;}
+

diff --git a/tests/data/test-diff-filter/test-PR24731-v0.c b/tests/data/test-diff-filter/test-PR24731-v0.c
new file mode 100644 (file)
index 0000000..c1f5761
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR24731-v0.c
@@ -0,0 +1,5 @@
+ union u { int a; char c; short s; };
+
+ void test_func(union u var) {
+  (void) var;
+ }

diff --git a/tests/data/test-diff-filter/test-PR24731-v0.o b/tests/data/test-diff-filter/test-PR24731-v0.o
new file mode 100644 (file)
index 0000000..c5cd811
Binary files /dev/null and b/tests/data/test-diff-filter/test-PR24731-v0.o differ

diff --git a/tests/data/test-diff-filter/test-PR24731-v1.c b/tests/data/test-diff-filter/test-PR24731-v1.c
new file mode 100644 (file)
index 0000000..9a73f99
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR24731-v1.c
@@ -0,0 +1,5 @@
+ union u { int a; short s; char c; };
+
+ void test_func(union u var) {
+  (void) var;
+ }

diff --git a/tests/data/test-diff-filter/test-PR24731-v1.o b/tests/data/test-diff-filter/test-PR24731-v1.o
new file mode 100644 (file)
index 0000000..3eeea29
Binary files /dev/null and b/tests/data/test-diff-filter/test-PR24731-v1.o differ

diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
index c05928f..74b8c06 100644 (file)
--- a/tests/test-diff-filter.cc
+++ b/tests/test-diff-filter.cc
@@ -549,6 +549,13 @@ InOutSpec in_out_specs[] =
     "data/test-diff-filter/PR24430-fold-qualified-array-report-0.txt",
     "output/test-diff-filter/PR24430-fold-qualified-array-report-0.txt",
   },
+  {
+    "data/test-diff-filter/test-PR24731-v0.o ",
+    "data/test-diff-filter/test-PR24731-v1.o ",
+    "--no-default-suppression",
+    "data/test-diff-filter/test-PR24731-report-0.txt",
+    "output/test-diff-filter/test-PR24731-report-0.txt",
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL}
 };