comparison: Ensure that fn parms with basic types can't be redundant

When comparing the two libc binaries from
"https://the.owo.foundation/Ac1Ksw8.tgz", abidiff crashes.

It appears to be due to an assert that is hit because the overload of
default_reporter::report for fn_parm_diff momentarily removes the
redundant-ness from the categorization of the diff node of the type of
a function parameter.  The problem is that the sole child diff node of
that type diff node is itself redundant.  So the function parameter
type diff node should really be redundant too.  Oops, there is a logic
violation there, hence the assert violation.

I commented out the line that removes the redundant-ness.  After all,
if function parameter types shouldn't be redundant, that should have
been taken care of by the redundancy_marking_visitor code in
abg-comparison.cc as well as its associated category propagation code.

But then consider what happens with a reproducer of the libc binaries
above:

	$ cat test-PR29387-v0.c
	typedef int Integer;

	void
	f0(Integer i, char c)
	{
	  i + c;
	}

	void
	f1(Integer i, unsigned char c)
	{
	  i + c;
	}

	$
	$ cat test-PR29387-v1.c
	typedef long int Integer;

	void
	f0(Integer i, char c)
	{
	  i + c;
	}

	void
	f1(Integer i, unsigned char c)
	{
	  i + c;
	}

	$ gcc -g test-PR29387-v0.c
	$ gcc -g test-PR29387-v1.c
	$
	$ abidiff test-PR29387-v0.o test-PR29387-v1.o
	Functions changes summary: 0 Removed, 2 Changed, 0 Added functions
	Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

	2 functions with some indirect sub-type change:

	  [C] 'function void f0(Integer, char)' at PR29387-test-v1.c:4:1 has some indirect sub-type changes:
	    parameter 1 of type 'typedef Integer' changed:
	      underlying type 'int' changed:
		type name changed from 'int' to 'long int'
		type size changed from 32 to 64 (in bits)

	  [C] 'function void f1(Integer, unsigned char)' at PR29387-test-v1.c:10:1 has some indirect sub-type changes:
	$
	$

So, the problem is this peace of report:

	  [C] 'function void f1(Integer, unsigned char)' at PR29387-test-v1.c:10:1 has some indirect sub-type changes:

You see that the report is empty; the reporter could not say what changed.

What changed is the typedef "Integer" that changed from "int" to "long
int".  The redundancy_marking_visitor pass marked the change of the
underlying type of the Integer typedef as being redundant.  This is
because that typedef change has already been reported on the f0
function interface.

The problem is that by design, the 'int' to 'long int' change should
not have been marked as being redundant by the
redundancy_marking_visitor pass.  This is because, we want to see all
the "basic type changes" on function parameters types.  They are
deemed "local changes" of the function types, and we want to see all
local changes to functions because it is almost 100% sure these are
non-compatible changes.

The root cause of the problem is that the function
has_basic_type_change_only in abg-comparison.cc fails to detect that
the parameter change carries a typedef-to-basic-type change, so the
function parameter is wrongly marked as being redundant even though it
ultimately carries a basic type change.

This patch thus teaches has_basic_type_change_only to look through
parameter changes to better detect basic type changes.

	* include/abg-comparison.h (peel_fn_parm_diff)
	(peel_typedef_qualified_type_or_parameter_diff): Declare ...
	* src/abg-comparison.cc (peel_fn_parm_diff)
	(peel_typedef_qualified_type_or_parameter_diff): ... new
	functions.
	(has_basic_type_change_only): Look through function parameters,
	typedefs and qualified types.
	* src/abg-default-reporter.cc (default_reporter::report): Remove
	the temporary removal of the redundant categorisation.
	Redundant-ness should have been handled by the
	redundancy_marking_visitor pass.
	* tests/data/test-diff-filter/test-PR29387-report.txt: Reference
	test output.
	* tests/data/test-diff-filter/test-PR29387-v{0,1}.c: Source of the
	input tests.
	* tests/data/test-diff-filter/test-PR29387-v{0,1}.o: Input test
	binaries.
	* tests/data/Makefile.am: Add the new test material above to the
	source distribution.
	* tests/test-diff-filter.cc (in_out_specs): Add the test binaries
	above 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 e51a5021d76573ec83deaf7a984b1676c6d20fba..a3c197b9199c10d62b636e60d62d1cb02b72fd7a 100644 (file)
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -2860,11 +2860,17 @@ peel_reference_diff(const diff* dif);
 const diff*
 peel_qualified_diff(const diff* dif);
 
+const diff*
+peel_fn_parm_diff(const diff* dif);
+
 const diff*
 peel_pointer_or_qualified_type_diff(const diff* dif);
 
 const diff*
 peel_typedef_or_qualified_type_diff(const diff* dif);
+
+const diff*
+peel_typedef_qualified_type_or_parameter_diff(const diff *dif);
 }// end namespace comparison
 
 }// end namespace abigail

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 279821129cdaca68072d1a515ff93897a97953db..c536f66ffbd1a53416b6413977c07dcd3f89a120 100644 (file)
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -12424,6 +12424,21 @@ peel_qualified_diff(const diff* dif)
   return dif;
 }
 
+/// If a diff node is about changes between two function parameters
+/// get the diff node about changes between the types of the parameters.
+///
+/// @param dif the dif node to consider.
+///
+/// @return the diff of the types of the parameters.
+const diff*
+peel_fn_parm_diff(const diff* dif)
+{
+  const fn_parm_diff *d = 0;
+  while ((d = is_fn_parm_diff(dif)))
+    dif = d->type_diff().get();
+  return dif;
+}
+
 /// If a diff node is about changes between two pointer, reference or
 /// qualified types, get the diff node about changes between the
 /// underlying types.
@@ -12480,6 +12495,34 @@ peel_typedef_or_qualified_type_diff(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 neither typedef,
+/// qualified type nor parameters.
+///
+/// @param dif the dif node to consider.
+///
+/// @return the diff node about changes between the underlying types.
+const diff*
+peel_typedef_qualified_type_or_parameter_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 if (const fn_parm_diff *d = is_fn_parm_diff(dif))
+       dif = peel_fn_parm_diff(d);
+      else
+       break;
+    }
+  return dif;
+}
+
 /// Test if a diff node represents a diff between two class or union
 /// types.
 ///
@@ -12522,6 +12565,8 @@ has_local_type_change_only(const diff *d)
 bool
 has_basic_type_change_only(const diff *d)
 {
+  d = peel_typedef_qualified_type_or_parameter_diff(d);
+
   if (is_diff_of_basic_type(d, true) && d->has_changes())
     return true;
   else if (const var_diff * v = dynamic_cast<const var_diff*>(d))

diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index 36af26bf988c98bdfbfe6406ec00c1c09b0feb57..8c705060ce69d6c0aea83163a7f8231a9255b81c 100644 (file)
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -518,9 +518,7 @@ default_reporter::report(const fn_parm_diff& d, ostream& out,
     {
       diff_sptr type_diff = d.type_diff();
       ABG_ASSERT(type_diff->has_changes());
-      diff_category saved_category = type_diff->get_category();
-      // Parameter type changes are never redundants.
-      type_diff->set_category(saved_category & ~REDUNDANT_CATEGORY);
+
       out << indent;
       if (f->get_is_artificial())
        out << "implicit ";
@@ -535,7 +533,6 @@ default_reporter::report(const fn_parm_diff& d, ostream& out,
        out << "' changed:\n";
 
       type_diff->report(out, indent + "  ");
-      type_diff->set_category(saved_category);
     }
 }
 

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 401d4c8cb6b4d445d182596f041d5f42a8547acb..7bddd6460014fae3077cd6cc1b4308e4ccfbd0c0 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -1099,6 +1099,11 @@ test-diff-filter/test-PR27995.abi             \
 test-diff-filter/test-PR28013-fn-variadic.c.report.txt   \
 test-diff-filter/test-PR28013-fn-variadic.c.0.abi        \
 test-diff-filter/test-PR28013-fn-variadic.c.1.abi        \
+test-diff-filter/test-PR29387-v1.c      \
+test-diff-filter/test-PR29387-v0.c      \
+test-diff-filter/test-PR29387-v1.o      \
+test-diff-filter/test-PR29387-v0.o      \
+test-diff-filter/test-PR29387-report.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/test-PR29387-report.txt b/tests/data/test-diff-filter/test-PR29387-report.txt
new file mode 100644 (file)
index 0000000..d6541d4
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR29387-report.txt
@@ -0,0 +1,17 @@
+Functions changes summary: 0 Removed, 2 Changed, 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+2 functions with some indirect sub-type change:
+
+  [C] 'function void f0(Integer, char)' at PR29387-test-v1.c:4:1 has some indirect sub-type changes:
+    parameter 1 of type 'typedef Integer' changed:
+      underlying type 'int' changed:
+        type name changed from 'int' to 'long int'
+        type size changed from 32 to 64 (in bits)
+
+  [C] 'function void f1(Integer, unsigned char)' at PR29387-test-v1.c:10:1 has some indirect sub-type changes:
+    parameter 1 of type 'typedef Integer' changed:
+      underlying type 'int' changed:
+        type name changed from 'int' to 'long int'
+        type size changed from 32 to 64 (in bits)
+

diff --git a/tests/data/test-diff-filter/test-PR29387-v0.c b/tests/data/test-diff-filter/test-PR29387-v0.c
new file mode 100644 (file)
index 0000000..cf0ee4c
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR29387-v0.c
@@ -0,0 +1,13 @@
+typedef int Integer;
+
+void
+f0(Integer i, char c)
+{
+  i + c;
+}
+
+void
+f1(Integer i, unsigned char c)
+{
+  i + c;
+}

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

diff --git a/tests/data/test-diff-filter/test-PR29387-v1.c b/tests/data/test-diff-filter/test-PR29387-v1.c
new file mode 100644 (file)
index 0000000..295e6a4
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR29387-v1.c
@@ -0,0 +1,13 @@
+typedef long int Integer;
+
+void
+f0(Integer i, char c)
+{
+  i + c;
+}
+
+void
+f1(Integer i, unsigned char c)
+{
+  i + c;
+}

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

diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
index 861fc48d366a10d28eba5670e4f1e9379c1aebb2..7f9355bd9d99adc0c212abc5826f25d7c726a306 100644 (file)
--- a/tests/test-diff-filter.cc
+++ b/tests/test-diff-filter.cc
@@ -815,6 +815,13 @@ InOutSpec in_out_specs[] =
    "data/test-diff-filter/test-PR28013-fn-variadic.c.report.txt",
    "output/test-diff-filter/test-PR28013-fn-variadic.c.report.txt",
   },
+  {
+   "data/test-diff-filter/test-PR29387-v0.o",
+   "data/test-diff-filter/test-PR29387-v1.o",
+   "--no-default-suppression",
+   "data/test-diff-filter/test-PR29387-report.txt",
+   "output/test-diff-filter/test-PR29387-report.txt",
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL}
 };