Don't consider changes to basic types as being redundant

Suppose we have two types struct A and struct B.  Suppose the two
types have members of integer type.  Suppose there are members of
integer type in struct A that are modified to become members of char
type.  Suppose, at last, that we also have members of integer type in
struct B that are modified to become members of char type.

In that case, we want to see all the changes of members which types
got changed from integer type to char type.  None of these changes
should be considered redundant.

Today, unfortunately, only the first change is reported by default.
The subsequent changes are considered to be redundant.

This patch fixes that by considering that changes that modifies the type of a
decl from a basic type into another are never considered redundant.

	* include/abg-comparison.h (is_diff_of_basic_type)
	(has_basic_type_change_only): Declare these functions ...
	* src/abg-comparison.cc (is_diff_of_basic_type)
	(has_basic_type_change_only): ... and define them.
	(redundancy_marking_visitor::visit_begin): Use the new
	has_basic_type_change_only.
	* tests/data/test-diff-filter/libtest37-v0.so: New binary test input.
	* tests/data/test-diff-filter/libtest37-v1.so: Likewise.
	* tests/data/test-diff-filter/test37-report-0.txt: New test
	reference output.
	* tests/data/test-diff-filter/test37-v0.cc: Source code of the new
	binary test input.
	* tests/data/test-diff-filter/test37-v1.cc: Likewise.
	* tests/data/Makefile.am: Update to add the new test material to
	the 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-comparison.h b/include/abg-comparison.h
index 70774231568507219c2f7825df3310b546dfcc84..97cc1d9e6457124d6a754cd94d0a9408ba7696bd 100644 (file)
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -2457,6 +2457,12 @@ is_type_diff(const diff* diff);
 const decl_diff_base*
 is_decl_diff(const diff* diff);
 
+bool
+is_diff_of_basic_type(const diff* diff);
+
+bool
+has_basic_type_change_only(const diff* diff);
+
 const class_diff*
 is_class_diff(const diff* diff);
 

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 30e903aa6c581dccb944aaae51ee88f8fbeb7804..bd61cb38acf2a906fbae5c25a7d848b9a8548e2d 100644 (file)
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -13363,6 +13363,11 @@ struct redundancy_marking_visitor : public diff_node_visitor
                    }
                }
            if (!redundant_with_sibling_node
+               // Changes to basic types should never be considered
+               // redundant.  For instance, if a member of integer
+               // type is changed into a char type in both a struct A
+               // and a struct B, we want to see both changes.
+               && !has_basic_type_change_only(d)
                // Functions with similar *local* changes are never marked
                // redundant because otherwise one could miss important
                // similar local changes that are applied to different
@@ -13675,5 +13680,40 @@ is_diff_of_variadic_parameter(const diff* d)
 bool
 is_diff_of_variadic_parameter(const diff_sptr& d)
 {return is_diff_of_variadic_parameter(d.get());}
+
+/// Test if a diff node represents a diff between two basic types.
+///
+/// @param d the diff node to consider.
+///
+/// @return true iff @p d is a diff between two basic types.
+bool
+is_diff_of_basic_type(const diff *d)
+{return dynamic_cast<const type_decl_diff*>(d);}
+
+/// Test if a diff node is a decl diff that only carries a basic type
+/// change on its type diff sub-node.
+///
+/// @param d the diff node to consider.
+///
+/// @return true iff @p d is a decl diff that only carries a basic
+/// type change on its type diff sub-node.
+bool
+has_basic_type_change_only(const diff *d)
+{
+  if (is_diff_of_basic_type(d) && d->has_changes())
+    return true;
+  else if (const var_diff * v = dynamic_cast<const var_diff*>(d))
+    return (!v->has_local_changes()
+           && is_diff_of_basic_type(v->type_diff().get()));
+  else if (const fn_parm_diff * p = dynamic_cast<const fn_parm_diff*>(d))
+    return (!p->has_local_changes()
+           && is_diff_of_basic_type(p->type_diff().get()));
+  else if (const function_decl_diff* f =
+          dynamic_cast<const function_decl_diff*>(d))
+    return (!f->has_local_changes()
+           && f->type_diff()
+           && is_diff_of_basic_type(f->type_diff()->return_type_diff().get()));
+  return false;
+}
 }// end namespace comparison
 } // end namespace abigail

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 9fd822a4ed7f747594d997c0d76d11e2ca343dcc..47c91a6abc11f11e3c3345ecc66ed65dc444b311 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -589,6 +589,11 @@ test-diff-filter/test34-libjemalloc.so.2-intel-16.0.3 \
 test-diff-filter/test34-report-0.txt \
 test-diff-filter/test35-pr18754-no-added-syms-report-0.txt \
 test-diff-filter/test35-pr18754-no-added-syms-report-1.txt \
+test-diff-filter/libtest37-v0.so \
+test-diff-filter/libtest37-v1.so \
+test-diff-filter/test37-report-0.txt \
+test-diff-filter/test37-v0.cc \
+test-diff-filter/test37-v1.cc \
 \
 test-diff-suppr/test0-type-suppr-v0.cc \
 test-diff-suppr/test0-type-suppr-v1.cc \

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

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

diff --git a/tests/data/test-diff-filter/test37-report-0.txt b/tests/data/test-diff-filter/test37-report-0.txt
new file mode 100644 (file)
index 0000000..b2579eb
--- /dev/null
+++ b/tests/data/test-diff-filter/test37-report-0.txt
@@ -0,0 +1,36 @@
+Functions changes summary: 0 Removed, 3 Changed, 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+3 functions with some indirect sub-type change:
+
+  [C]'function void f1(A&)' at test37-v1.cc:19:1 has some indirect sub-type changes:
+    parameter 1 of type 'A&' has sub-type changes:
+      in referenced type 'struct A' at test37-v1.cc:3:1:
+        type size changed from 32 to 8 bits
+        1 data member change:
+         type of 'int A::m0' changed:
+           type name changed from 'int' to 'char'
+           type size changed from 32 to 8 bits
+
+
+  [C]'function void f2(B&)' at test37-v1.cc:23:1 has some indirect sub-type changes:
+    parameter 1 of type 'B&' has sub-type changes:
+      in referenced type 'struct B' at test37-v1.cc:8:1:
+        type size changed from 32 to 8 bits
+        1 data member change:
+         type of 'int B::m0' changed:
+           type name changed from 'int' to 'char'
+           type size changed from 32 to 8 bits
+
+
+  [C]'function void f3(C&)' at test37-v1.cc:27:1 has some indirect sub-type changes:
+    parameter 1 of type 'C&' has sub-type changes:
+      in referenced type 'struct C' at test37-v1.cc:13:1:
+        type size changed from 32 to 8 bits
+        1 data member change:
+         type of 'int C::m0' changed:
+           type name changed from 'int' to 'char'
+           type size changed from 32 to 8 bits
+
+
+

diff --git a/tests/data/test-diff-filter/test37-v0.cc b/tests/data/test-diff-filter/test37-v0.cc
new file mode 100644 (file)
index 0000000..a14dff6
--- /dev/null
+++ b/tests/data/test-diff-filter/test37-v0.cc
@@ -0,0 +1,28 @@
+// g++ -g -Wall -shared -o libtest37-v0.so test37-v0.cc
+
+struct A
+{
+  int m0;
+};
+
+struct B
+{
+  int m0;
+};
+
+struct C
+{
+  int m0;
+};
+
+void
+f1(A&)
+{}
+
+void
+f2(B&)
+{}
+
+void
+f3(C&)
+{}

diff --git a/tests/data/test-diff-filter/test37-v1.cc b/tests/data/test-diff-filter/test37-v1.cc
new file mode 100644 (file)
index 0000000..ffcbf9c
--- /dev/null
+++ b/tests/data/test-diff-filter/test37-v1.cc
@@ -0,0 +1,28 @@
+// g++ -g -Wall -shared -o libtest37-v1.so test37-v1.cc
+
+struct A
+{
+  char m0;
+};
+
+struct B
+{
+  char m0;
+};
+
+struct C
+{
+  char m0;
+};
+
+void
+f1(A&)
+{}
+
+void
+f2(B&)
+{}
+
+void
+f3(C&)
+{}

diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
index 94eb88fc46b29794e90758c434dfe61ae42aa997..ba2ce782beac2a38f735f50741c37b3a8cc065aa 100644 (file)
--- a/tests/test-diff-filter.cc
+++ b/tests/test-diff-filter.cc
@@ -436,6 +436,13 @@ InOutSpec in_out_specs[] =
     "data/test-diff-filter/test35-pr18754-no-added-syms-report-1.txt",
     "output/test-diff-filter/test35-pr18754-no-added-syms-report-1.txt",
   },
+  {
+    "data/test-diff-filter/libtest37-v0.so",
+    "data/test-diff-filter/libtest37-v1.so",
+    "--no-default-suppression --no-linkage-name",
+    "data/test-diff-filter/test37-report-0.txt",
+    "output/test-diff-filter/test37-report-0.txt",
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL}
 };