[Bug default/27569] New: abidiff misses a function parameter addition

[Bug default/27569] New: abidiff misses a function parameter addition

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

            Bug ID: 27569
           Summary: abidiff misses a function parameter addition
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida+abigail at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

Created attachment 13304
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13304&action=edit
two XML files to compare

Hi Dodji.

I'm attaching two reduced (manually edited, pruned) ABIs. Alas, they are still
quite large. I've tried and failed to reproduce the issue with a small test
case.

abidiff reports nothing, but should report this:

  [C] 'function void device_add_disk(gendisk*)' at genhd.h:420:1 has some
indirect sub-type changes:
    parameter 2 of type 'int' was added

Note that adding --redundant makes no difference.

Changing the type of the first argument in common to int made the problem go
away.

I believe I've narrowed the problem down to diff filtering.

Looking at apply_filters_and_compute_diff_stats, is_filtered_out for the single
function diff initially returns true, but this changes to false after
filtering.

The category of the diff changes from NO_CHANGE_CATEGORY before the filtering
and to HARMLESS_UNION_CHANGE_CATEGORY|FN_RETURN_TYPE_CV_CHANGE_CATEGORY
afterwards. I believe these values come from deep within inconsequential
changes to the first argument's type. Exactly one filter is applied:
harmless_harmful_filter.

My best guess is that there is a hole in the category classification for
"incompatible function type/decl changes", though it seems unlikely that this
wasn't noticed before now.

Regards,
Giuliano.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #1 from Giuliano Procida <gprocida+abigail at google dot com> ---
I've done a little more investigation.

HARMLESS_UNION_CHANGE_CATEGORY|FN_RETURN_TYPE_CV_CHANGE_CATEGORY are indeed
inherited/propagated categories.

The function decl diff has is_filtered_out returning false before categories
are propagated to returning true afterwards.

So I think the problem is that NO_CHANGE_CATEGORY is the incorrect local
category to start off with.

I also think the only reason problems have not been noticed before is that
is_filtered_out contains the suspicious-looking:

  if (category == NO_CHANGE_CATEGORY)
    return false;

This catches the common case of no special categories being propagated to a
node containing a real diff. If it's removed, abidiff stops finding a lot of
diffs.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #2 from Giuliano Procida <gprocida+abigail at google dot com> ---
The original commit that introduced the odd return for NO_CHANGE_CATEGORY was
to handle the case where categorisation hadn't yet happened.

commit 5bce089f2384ea83562a981ae3adafc835094d4e
Author: Dodji Seketeli <dodji@redhat.com>
Date:   Tue Sep 16 15:14:35 2014 +0200

    Emit reports about not-yet categorized diff nodes

            * include/abg-comparison.h (NO_CHANGE_CATEGORY): Better comment
            this enumerator.
            * src/abg-comparison.cc (diff::is_filtered_out): Not-yet
            categorized changes are not filtered out anymore.
            (distinct_diff::report): Avoid extraneous new line here.

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

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-03-24

--- Comment #3 from dodji at redhat dot com ---
using --harmless gives a huge output, including the information about parameter
2 being being added.

Is it possible to have the two offending binaries?
When dealing with this type of issue, I'd rather work on binaries than abixml
as the issue might be in the process of emitting the abixml.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #4 from Giuliano Procida <gprocida+abigail at google dot com> ---
I can (probably) find the binaries. I may need to make them available elsewhere
as they are going to be very large.

A couple of things though:

* our kernel workflow compares XML, not binaries, and the bug could
theoretically only manifest with XML comparisons (which, btw, doesn't mean the
XML writer is at fault; it could be the XML reader or some combination of
components)
* the function type change is easily visible in the XML attached

Let me know if you are going to hold out for the binaries as I have a few
things stacked up.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #5 from Giuliano Procida <gprocida+abigail at google dot com> ---
Created attachment 13327
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13327&action=edit
original XML files

I can supply pristine (larger) XML files (see attached). However, I cannot
supply a binary for the "old" file as it wasn't preserved. The "new" binary is
317MiB and we'd need some bespoke means to get it to you.

The "old" XML file was generated by an abidw built from our internal git branch
(the major difference from upstream at the time being the symtab reader); it
was a (stale) user-generated kernel ABI for an AOSP kernel, pending commit. The
"new" XML file was generated by an abidw having one extra commit (relating to
an ELF symbol issue); it was generated by CI infrastructure to capture the
kernel ABI as of the pending commit.

In short, reproducing the issue from binaries isn't feasible due the difficulty
/ impossibility of reproducing the build environment of the "old" kernel ABI.
However, the debugging I did earlier really points to this being an issue with
diff categorisation.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #6 from dodji at redhat dot com ---

> In short, reproducing the issue from binaries isn't feasible due the
> difficulty / impossibility of reproducing the build environment of the "old"
> kernel ABI. However, the debugging I did earlier really points to this being
> an issue with diff categorisation.

Fair enough.

Looking into this a bit more, this is a real issue regardless.

Thanks for raising it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #7 from dodji at redhat dot com ---
I have posted this tentative patch
https://sourceware.org/pipermail/libabigail/2021q1/003301.html.

Could you please try it in your environment to see if it addresses the issue
and review it?

Thanks in advance.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [Bug default/27569] abidiff misses a function parameter addition

[Giuliano Procida]

Hi.

On Thu, 25 Mar 2021 at 11:22, dodji at redhat dot com via Libabigail <
libabigail@sourceware.org> wrote:

> https://sourceware.org/bugzilla/show_bug.cgi?id=27569
>
> --- Comment #7 from dodji at redhat dot com ---
> I have posted this tentative patch
> https://sourceware.org/pipermail/libabigail/2021q1/003301.html.
>
> Could you please try it in your environment to see if it addresses the
> issue
> and review it?
>
>
I had a quick look. It seems to do the trick.

I don't think you need to include new test files as at least one existing
test is affected by the change.

A couple of questions:

Can any other function level changes be missed? I'm guessing not, from
looking at the XML attributes on a function-decl.

In the bug, I wrote:

I also think the only reason problems have not been noticed before is that
is_filtered_out contains the suspicious-looking:

  if (category == NO_CHANGE_CATEGORY)
    return false;

If I remove this line and re-run tests, some other diffs disappear, such as:

---
/usr/local/google/home/gprocida/android/libabigail/build/../tests/data/test-diff-suppr/test10-changed-parm-c-report-0.txt
  2021-02-03 10:29:34.846116830 +0000
+++
/usr/local/google/home/gprocida/android/libabigail/build/tests/output/test-diff-suppr/test10-changed-parm-c-report-0.txt
   2021-03-25 16:28:56.472979039 +0000
@@ -1,16 +1,3 @@
-Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
function
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

-1 function with some indirect sub-type change:
-
-  [C] 'function int foo(int, int)' has some indirect sub-type changes:
-    return type changed:
-      type name changed from 'int' to 'float'
-      type size hasn't changed
-    parameter 1 of type 'int' changed:
-      type name changed from 'int' to 'float'
-      type size hasn't changed
-    parameter 2 of type 'int' changed:
-      type name changed from 'int' to 'float'
-      type size hasn't changed
-

Does this mean you are also missing a category for "has changed to a
different type"? Or is that somehow the residual meaning
of NO_CHANGE_CATEGORY?

Thanks in advance.
>
>
You're welcome.


> --
> You are receiving this mail because:
> You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[gprocida at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #8 from gprocida at google dot com ---
Hi.

On Thu, 25 Mar 2021 at 11:22, dodji at redhat dot com via Libabigail <
libabigail@sourceware.org> wrote:

> https://sourceware.org/bugzilla/show_bug.cgi?id=27569
>
> --- Comment #7 from dodji at redhat dot com ---
> I have posted this tentative patch
> https://sourceware.org/pipermail/libabigail/2021q1/003301.html.
>
> Could you please try it in your environment to see if it addresses the
> issue
> and review it?
>
>
I had a quick look. It seems to do the trick.

I don't think you need to include new test files as at least one existing
test is affected by the change.

A couple of questions:

Can any other function level changes be missed? I'm guessing not, from
looking at the XML attributes on a function-decl.

In the bug, I wrote:

I also think the only reason problems have not been noticed before is that
is_filtered_out contains the suspicious-looking:

  if (category == NO_CHANGE_CATEGORY)
    return false;

If I remove this line and re-run tests, some other diffs disappear, such as:

---
/usr/local/google/home/gprocida/android/libabigail/build/../tests/data/test-diff-suppr/test10-changed-parm-c-report-0.txt
  2021-02-03 10:29:34.846116830 +0000
+++
/usr/local/google/home/gprocida/android/libabigail/build/tests/output/test-diff-suppr/test10-changed-parm-c-report-0.txt
   2021-03-25 16:28:56.472979039 +0000
@@ -1,16 +1,3 @@
-Functions changes summary: 0 Removed, 1 Changed, 0 Added function
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
function
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

-1 function with some indirect sub-type change:
-
-  [C] 'function int foo(int, int)' has some indirect sub-type changes:
-    return type changed:
-      type name changed from 'int' to 'float'
-      type size hasn't changed
-    parameter 1 of type 'int' changed:
-      type name changed from 'int' to 'float'
-      type size hasn't changed
-    parameter 2 of type 'int' changed:
-      type name changed from 'int' to 'float'
-      type size hasn't changed
-

Does this mean you are also missing a category for "has changed to a
different type"? Or is that somehow the residual meaning
of NO_CHANGE_CATEGORY?

Thanks in advance.
>
>
You're welcome.


> --
> You are receiving this mail because:
> You are on the CC list for the bug.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [Bug default/27569] abidiff misses a function parameter addition

[Dodji Seketeli]

Giuliano Procida via Libabigail <libabigail@sourceware.org> a écrit:

> I had a quick look. It seems to do the trick.

Thanks.

> I don't think you need to include new test files as at least one existing
> test is affected by the change.

Stricto sensu, you are right.  There are several other tests affected by
the change.

The reason why I am keeping this one is that it's context is a little
different from the others.

If anything, it's doing the analysis from
abixml files.

Also, looking at things a little deeper the function type impacted by
the change is also used in other data structures in the ABI graph and
that has intersting interactions in terms of the various categorizations
passes involved.  I think it is interesting to see the change being
detected and reported in that context.

> A couple of questions:
>
> Can any other function level changes be missed? I'm guessing not, from
> looking at the XML attributes on a function-decl.
>
> In the bug, I wrote:
>
> I also think the only reason problems have not been noticed before is that
> is_filtered_out contains the suspicious-looking:
>
>   if (category == NO_CHANGE_CATEGORY)
>     return false;

The NO_CHANGE_CATEGORY lumps together all changes that haven't yet been
categorized.  So what this line says is that if we are looking at a
change that is not (yet) categorized, then do not filter it out.

So it's not suspicious.

>
> If I remove this line and re-run tests, some other diffs disappear, such as:
>
> ---
> /usr/local/google/home/gprocida/android/libabigail/build/../tests/data/test-diff-suppr/test10-changed-parm-c-report-0.txt
>   2021-02-03 10:29:34.846116830 +0000
> +++
> /usr/local/google/home/gprocida/android/libabigail/build/tests/output/test-diff-suppr/test10-changed-parm-c-report-0.txt
>    2021-03-25 16:28:56.472979039 +0000
> @@ -1,16 +1,3 @@
> -Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> +Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
> function
>  Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>
> -1 function with some indirect sub-type change:
> -
> -  [C] 'function int foo(int, int)' has some indirect sub-type changes:
> -    return type changed:
> -      type name changed from 'int' to 'float'
> -      type size hasn't changed
> -    parameter 1 of type 'int' changed:
> -      type name changed from 'int' to 'float'
> -      type size hasn't changed
> -    parameter 2 of type 'int' changed:
> -      type name changed from 'int' to 'float'
> -      type size hasn't changed
> -
>
> Does this mean you are also missing a category for "has changed to a
> different type"? Or is that somehow the residual meaning
> of NO_CHANGE_CATEGORY?

From what I said above, I think it's more of the latter.

Cheers,

-- 
		Dodji

[Bug default/27569] abidiff misses a function parameter addition

[dodji at seketeli dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

--- Comment #9 from dodji at seketeli dot org ---
Giuliano Procida via Libabigail <libabigail@sourceware.org> a écrit:

> I had a quick look. It seems to do the trick.

Thanks.

> I don't think you need to include new test files as at least one existing
> test is affected by the change.

Stricto sensu, you are right.  There are several other tests affected by
the change.

The reason why I am keeping this one is that it's context is a little
different from the others.

If anything, it's doing the analysis from
abixml files.

Also, looking at things a little deeper the function type impacted by
the change is also used in other data structures in the ABI graph and
that has intersting interactions in terms of the various categorizations
passes involved.  I think it is interesting to see the change being
detected and reported in that context.

> A couple of questions:
>
> Can any other function level changes be missed? I'm guessing not, from
> looking at the XML attributes on a function-decl.
>
> In the bug, I wrote:
>
> I also think the only reason problems have not been noticed before is that
> is_filtered_out contains the suspicious-looking:
>
>   if (category == NO_CHANGE_CATEGORY)
>     return false;

The NO_CHANGE_CATEGORY lumps together all changes that haven't yet been
categorized.  So what this line says is that if we are looking at a
change that is not (yet) categorized, then do not filter it out.

So it's not suspicious.

>
> If I remove this line and re-run tests, some other diffs disappear, such as:
>
> ---
> /usr/local/google/home/gprocida/android/libabigail/build/../tests/data/test-diff-suppr/test10-changed-parm-c-report-0.txt
>   2021-02-03 10:29:34.846116830 +0000
> +++
> /usr/local/google/home/gprocida/android/libabigail/build/tests/output/test-diff-suppr/test10-changed-parm-c-report-0.txt
>    2021-03-25 16:28:56.472979039 +0000
> @@ -1,16 +1,3 @@
> -Functions changes summary: 0 Removed, 1 Changed, 0 Added function
> +Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added
> function
>  Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>
> -1 function with some indirect sub-type change:
> -
> -  [C] 'function int foo(int, int)' has some indirect sub-type changes:
> -    return type changed:
> -      type name changed from 'int' to 'float'
> -      type size hasn't changed
> -    parameter 1 of type 'int' changed:
> -      type name changed from 'int' to 'float'
> -      type size hasn't changed
> -    parameter 2 of type 'int' changed:
> -      type name changed from 'int' to 'float'
> -      type size hasn't changed
> -
>
> Does this mean you are also missing a category for "has changed to a
> different type"? Or is that somehow the residual meaning
> of NO_CHANGE_CATEGORY?

From what I said above, I think it's more of the latter.

Cheers,

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/27569] abidiff misses a function parameter addition

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27569

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #10 from dodji at redhat dot com ---
A patch has been applied for this issue at
https://sourceware.org/pipermail/libabigail/2021q1/003320.html in the master
branch.

So the problem should be fixed in the upcoming 2.0 release.

Thanks for filling this!

-- 
You are receiving this mail because:
You are on the CC list for the bug.