]> sourceware.org Git - libabigail.git/commit
comparison: Ensure that fn parms with basic types can't be redundant
authorDodji Seketeli <dodji@redhat.com>
Tue, 19 Jul 2022 14:15:08 +0000 (16:15 +0200)
committerDodji Seketeli <dodji@redhat.com>
Wed, 21 Sep 2022 23:46:20 +0000 (01:46 +0200)
commitaad0e11f5058d287fa44d35f017798f317c6a698
treebfa7d1b2e37f5e0385227a20f331332a3629247b
parent9996947dfb6da7fc82bcdeb62a19cf2a6367c9a6
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>
include/abg-comparison.h
src/abg-comparison.cc
src/abg-default-reporter.cc
tests/data/Makefile.am
tests/data/test-diff-filter/test-PR29387-report.txt [new file with mode: 0644]
tests/data/test-diff-filter/test-PR29387-v0.c [new file with mode: 0644]
tests/data/test-diff-filter/test-PR29387-v0.o [new file with mode: 0644]
tests/data/test-diff-filter/test-PR29387-v1.c [new file with mode: 0644]
tests/data/test-diff-filter/test-PR29387-v1.o [new file with mode: 0644]
tests/test-diff-filter.cc
This page took 0.035085 seconds and 5 git commands to generate.