[Bug default/28319] New: abidw - regression in treatment of anonymous enums in structs

[Bug default/28319] New: abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

            Bug ID: 28319
           Summary: abidw - regression in treatment of anonymous enums in
                    structs
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

This has been seem with kernels like:

https://ci.android.com/builds/submitted/7705975/kernel_abi_aarch64/latest/vmlinux

Current master conflates the anonymous enums in struct net_device and struct
drm_buf.

Comparing old (a test case, so perhaps fairly old) XML and newly-generated XML
gives diffs like:

[...]
                                      in pointed to type 'struct drm_buf' at
drm_legacy.h:50:1:
                                        type size hasn't changed
                                        1 data member changes (2 filtered):
                                          type of '__anonymous_enum__ list'
changed:
                                            type size hasn't changed
                                            6 enumerator deletions:
                                             
'__anonymous_enum__::DRM_LIST_NONE' value '0'
                                             
'__anonymous_enum__::DRM_LIST_FREE' value '1'
                                             
'__anonymous_enum__::DRM_LIST_WAIT' value '2'
                                             
'__anonymous_enum__::DRM_LIST_PEND' value '3'
                                             
'__anonymous_enum__::DRM_LIST_PRIO' value '4'
                                             
'__anonymous_enum__::DRM_LIST_RECLAIM' value '5'
                                            6 enumerator insertions:
                                             
'__anonymous_enum__1::NETREG_UNINITIALIZED' value '0'
                                             
'__anonymous_enum__1::NETREG_REGISTERED' value '1'
                                             
'__anonymous_enum__1::NETREG_UNREGISTERING' value '2'
                                             
'__anonymous_enum__1::NETREG_UNREGISTERED' value '3'
                                             
'__anonymous_enum__1::NETREG_RELEASED' value '4'
                                             
'__anonymous_enum__1::NETREG_DUMMY' value '5'

[...]

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #1 from gprocida at google dot com ---
Bisected to 1cfbff1b3037d1049bdff7e86de27c3a86af23b3.

$ build/tools/abidw /tmp/vmlinux | egrep 'DRM_LIST_NONE|NETREG_REGISTERED'

"good"

      <enumerator name='NETREG_REGISTERED' value='1'/>
      <enumerator name='NETREG_REGISTERED' value='1'/>
      <enumerator name='DRM_LIST_NONE' value='0'/>
      <enumerator name='DRM_LIST_NONE' value='0'/>

"bad"

      <enumerator name='NETREG_REGISTERED' value='1'/>

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[dodji at redhat dot com]

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

--- Comment #2 from dodji at redhat dot com ---
Hello,

Can I please get a link to an "old" kernel for the comparison please?

I got the "new" one from
https://ci.android.com/builds/submitted/7705975/kernel_abi_aarch64/latest/vmlinux
just fine.

Thanks.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[woodard at redhat dot com]

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

Ben Woodard <woodard at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |28316


Referenced Bugs:

https://sourceware.org/bugzilla/show_bug.cgi?id=28316
[Bug 28316] instability in anonymous enums
-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28319
Bug 28319 depends on bug 28316, which changed state.

Bug 28316 Summary: Failure to represent typedef named anonymous enums
https://sourceware.org/bugzilla/show_bug.cgi?id=28316

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

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #3 from gprocida at google dot com ---
The kernel link is the first line of the bug description. It can be downloaded
incognito.

This is still reproducible at current master
c757289e0f5c78cc3e3cdc192a4f390ef2967499.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #4 from gprocida at google dot com ---
Hi Dodji.

Following up on the last few bug comments: "old" and "new" refer to abidw
before and after commit 1cfbff1b3037d1049bdff7e86de27c3a86af23b3.

Looking at that commit and the new code in more detail, the binary-only enum
comparison behaviour was added to address issues with a GIMP library ABI and
makes abidw ignore enumerator names when canonicalising types.

For Android and especially the kernel, we are extremely unlikely to have a
use-case where binary-only enum comparison is useful, because we are very
unlikely to link code against an old .a.

On the other hand, the new behaviour is causing the regression noted in this
bug.

While there may be a more nuanced resolution possible, the best way forward for
us is seems to be to put the new behaviour under flag control.

I'll start work on this now. Do you have any preference as to whether
binary-only enum comparison should be the default and what the flag should be
called?

From the perspective of adding the binary-only comparison feature, only the
GIMP test is improved, various other tests exhibit worse behaviour. Details
follow.

Regards.

runtestannotate and runtestreaddwarf

test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi
test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi

2 different (member type) enums are conflated
1 of them disappears from its containing class
it wasn't referred to elsewhere in the ABI

test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi
test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi

enum tcmalloc::ThreadCache::__anonymous_enum__ disappears
it wasn't referred to elsewhere in ABI

test-annotate/test-anonymous-members-0.o.abi

3 anonymous enums (all used) are conflated
2 of them vanish from their containing type

test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi

1 anonymous enum disappears from its containing type
it wasn't referred to elsewhere in ABI

test-abidiff/test-enum0-report.txt

This was an enum test where the name of an enumerator changed. The report no
longer shows any diff.

runtestaltdwarf

the GIMP test changes a lot, as expected

runtestdiffdwarf

test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt

The report no longer includes some enumerator name changes.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #5 from gprocida at google dot com ---
My references to "canonicalization" should be adjusted to "pre-canonicalization
resolution".

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-01-11
     Ever confirmed|0                           |1

--- Comment #6 from dodji at redhat dot com ---
hmmh, rather than a flag, I'd rather have a proper fix that makes both the gimp
test case and yours work.

I'll look into this if you don't beat me to it.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #7 from gprocida at google dot com ---
If you can achieve a fix for both simultaneously that would be great.

We are approaching an Android release cycle phase transition and would like to
get  a new version of the ABI monitoring tools cut very soon. Essentially, we
won't wait for this fix.

If you aren't keen on a flag, then it makes sense for me to take the commit I
posted at https://sourceware.org/pipermail/libabigail/2022q1/004062.html and
invert the default. We can then use this for Android pending a more complete
fix from you, at which point it can be reverted.

This removes any time pressure for a quick fix and ensures we have an easier
time with our next couple of ABI monitoring releases - no need to change glue
scripts, in particular.

I hope that makes sense.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #8 from gprocida at google dot com ---
I tried another version which left the default at the environment level intact.

https://sourceware.org/pipermail/libabigail/2022q1/004071.html

However, type comparisons performed *outside* compare_before_canonicalisation
were still being done ignoring enumerator names and this is what seems to
matter (at least for the vmlinux test case here).

As things stand, the use_enum_binary_only_equality code in
compare_before_canonicalisation does nothing as the values before/after are all
true.

Fixing the default in environment fixes this bug.

I'll post a new commit.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #9 from gprocida at google dot com ---
New, and I hope last, commit:
https://sourceware.org/pipermail/libabigail/2022q1/004074.html

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #10 from gprocida at google dot com ---
Created attachment 13904
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13904&action=edit
XML for the kernel

XML extracted with abidw --type-id-style hash at:

commit before commit that caused regression
commit that caused regression
current master
current master plus one line fix

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[dodji at redhat dot com]

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

--- Comment #11 from dodji at redhat dot com ---
Created attachment 13905
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13905&action=edit
Candidate patch

This is a candidate patch to better fix the initial issue, hopefully without
the regression you are seeing.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #12 from gprocida at google dot com ---
Across many tests, both fixes work just as well.

Your fix is preferred as it removes extra parameterisation of comparison
operations.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

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

--- Comment #13 from dodji at redhat dot com ---
This should now hopefully be fixed by commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=85694917418060370ed012954e7cce9b3109a260.

Thanks!

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

gprocida at google dot com changed:

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

--- Comment #14 from gprocida at google dot com ---
Hi Dodji.

1. I mentioned on IRC, there is a bit of debris left after your last commit.

git grep use_enum_binary_only_equality

2. Just for the record, if you think this is working as intended, please
re-close the bug.

When I tested in comment 12, I looked for changes in abidw output and there was
none.

There is a regression in abidiff though.

The two enums in these small code fragments

enum E6 { a6 = 6 } v6;

enum E6 { a6 = 6, b6 = 6 } v6;

are treated as ABI equivalent as they share the same set of enumerator values.

Giuliano.

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

Re: [Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[Dodji Seketeli]

gprocida at google dot com via Libabigail <libabigail@sourceware.org> a
écrit:

> Hi Dodji.

Hey Giuliano,

> git grep use_enum_binary_only_equality

Now, from what I am seeing, that one is not used anywhere anymore.  The
last use of it was in an obsolete comment.  I removed it in this commit
https://sourceware.org/pipermail/libabigail/2022q1/004184.html.

[...]


> When I tested in comment 12, I looked for changes in abidw output and there was
> none.
>
> There is a regression in abidiff though.
>
> The two enums in these small code fragments
>
> enum E6 { a6 = 6 } v6;
>
> enum E6 { a6 = 6, b6 = 6 } v6;
>
> are treated as ABI equivalent ...

Yes, this is as intended.  In the function
equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k)
in abi-ir.cc, you can see the comment:

  // Now compare the enumerators.  Note that the order of declaration
  // of enumerators should not matter in the comparison.
  //
  // Also if an enumerator value is redundant, that shouldn't impact
  // the comparison.
  //
  // In that case, note that the two enums below are considered equal:
  //
  // enum foo
  //     {
  //       e0 = 0;
  //       e1 = 1;
  //       e2 = 2;
  //     };
  //
  //     enum foo
  //     {
  //       e0 = 0;
  //       e1 = 1;
  //       e2 = 2;
  //       e_added = 1; // <-- this value is redundant with the value
  //                    //     of the enumerator e1.
  //     };
  //
  // These two enums are considered equal.

> ... as they share the same set of enumerator values.

It's rather because they have the same enumerator names /and/ values,
modulo one /additional/ enumerator which value is redundant with the
existing ones.  So in this newer way of seeing things, the previous
concept of "binary-only equality" is not used anymore.

[...]

> 2. Just for the record, if you think this is working as intended, please
> re-close the bug.

Yes, I think it's working as intended.  I'll wait for you to double
check if you see an obvious issue.  If not, I'll let you close the bug.
Would that work for you?

Thanks.

-- 
		Dodji

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[dodji at seketeli dot org]

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

--- Comment #15 from dodji at seketeli dot org ---
gprocida at google dot com via Libabigail <libabigail@sourceware.org> a
écrit:

> Hi Dodji.

Hey Giuliano,

> git grep use_enum_binary_only_equality

Now, from what I am seeing, that one is not used anywhere anymore.  The
last use of it was in an obsolete comment.  I removed it in this commit
https://sourceware.org/pipermail/libabigail/2022q1/004184.html.

[...]


> When I tested in comment 12, I looked for changes in abidw output and there was
> none.
>
> There is a regression in abidiff though.
>
> The two enums in these small code fragments
>
> enum E6 { a6 = 6 } v6;
>
> enum E6 { a6 = 6, b6 = 6 } v6;
>
> are treated as ABI equivalent ...

Yes, this is as intended.  In the function
equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k)
in abi-ir.cc, you can see the comment:

  // Now compare the enumerators.  Note that the order of declaration
  // of enumerators should not matter in the comparison.
  //
  // Also if an enumerator value is redundant, that shouldn't impact
  // the comparison.
  //
  // In that case, note that the two enums below are considered equal:
  //
  // enum foo
  //     {
  //       e0 = 0;
  //       e1 = 1;
  //       e2 = 2;
  //     };
  //
  //     enum foo
  //     {
  //       e0 = 0;
  //       e1 = 1;
  //       e2 = 2;
  //       e_added = 1; // <-- this value is redundant with the value
  //                    //     of the enumerator e1.
  //     };
  //
  // These two enums are considered equal.

> ... as they share the same set of enumerator values.

It's rather because they have the same enumerator names /and/ values,
modulo one /additional/ enumerator which value is redundant with the
existing ones.  So in this newer way of seeing things, the previous
concept of "binary-only equality" is not used anymore.

[...]

> 2. Just for the record, if you think this is working as intended, please
> re-close the bug.

Yes, I think it's working as intended.  I'll wait for you to double
check if you see an obvious issue.  If not, I'll let you close the bug.
Would that work for you?

Thanks.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

--- Comment #16 from gprocida at google dot com ---
Hello.

On Tue, 1 Mar 2022 at 11:22, dodji at seketeli dot org
<sourceware-bugzilla@sourceware.org> wrote:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28319
>
> --- Comment #15 from dodji at seketeli dot org ---
> gprocida at google dot com via Libabigail <libabigail@sourceware.org> a
> écrit:
>
> > Hi Dodji.
>
> Hey Giuliano,
>
> > git grep use_enum_binary_only_equality
>
> Now, from what I am seeing, that one is not used anywhere anymore.  The
> last use of it was in an obsolete comment.  I removed it in this commit
> https://sourceware.org/pipermail/libabigail/2022q1/004184.html.
>
> [...]
>
>
> > When I tested in comment 12, I looked for changes in abidw output and there was
> > none.
> >
> > There is a regression in abidiff though.
> >
> > The two enums in these small code fragments
> >
> > enum E6 { a6 = 6 } v6;
> >
> > enum E6 { a6 = 6, b6 = 6 } v6;
> >
> > are treated as ABI equivalent ...
>
> Yes, this is as intended.  In the function
> equals(const enum_type_decl& l, const enum_type_decl& r, change_kind* k)
> in abi-ir.cc, you can see the comment:
>
>   // Now compare the enumerators.  Note that the order of declaration
>   // of enumerators should not matter in the comparison.
>   //
>   // Also if an enumerator value is redundant, that shouldn't impact
>   // the comparison.
>   //
>   // In that case, note that the two enums below are considered equal:
>   //
>   // enum foo
>   //     {
>   //       e0 = 0;
>   //       e1 = 1;
>   //       e2 = 2;
>   //     };
>   //
>   //     enum foo
>   //     {
>   //       e0 = 0;
>   //       e1 = 1;
>   //       e2 = 2;
>   //       e_added = 1; // <-- this value is redundant with the value
>   //                    //     of the enumerator e1.
>   //     };
>   //
>   // These two enums are considered equal.
>
> > ... as they share the same set of enumerator values.
>
> It's rather because they have the same enumerator names /and/ values,
> modulo one /additional/ enumerator which value is redundant with the
> existing ones.  So in this newer way of seeing things, the previous
> concept of "binary-only equality" is not used anymore.
>

enum E6 { a6 = 6 } v6;
vs
enum E6 { a6 = 6, b6 = 6 } v6;

Well, the sets of enumerator values are:
{ 6 } vs { 6 }
and the sets of enumerator names are:
{ a6 } vs { a6, b6 }

So they don't have the same enumerator names.

As you point out, the addition of "b6" does not change the meaning of
any other enumerator. I think we understand each other.

It's just a question as to whether this should be considered an ABI
difference. Perhaps it should be a "harmless" difference, though I
would not want to add more complexity to code that is not regularly
exercised in production.

I also discovered recently the concept of C (and this does not exist
in C++) type compatibility.

https://en.cppreference.com/w/c/language/type#Compatible_types
https://www.ibm.com/docs/en/zos/2.1.0?topic=types-compatibility-structures-unions-enumerations-c-only

These documents could be more clear, but I think they imply that these
two are compatible.

enum E { b = 2, a = 1 };
enum E { a = 1, b = 2 };

On first reading, I thought cppreference also implied that these two
are compatible (which would be relevant here), but I don't think they
are:

enum E { a = 1, b = 2 };
enum E { a = 1 };

> [...]
>
> > 2. Just for the record, if you think this is working as intended, please
> > re-close the bug.
>
> Yes, I think it's working as intended.  I'll wait for you to double
> check if you see an obvious issue.  If not, I'll let you close the bug.
> Would that work for you?
>

Sure.

> Thanks.
>
> --
> You are receiving this mail because:
> You reported the bug.

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

[Bug default/28319] abidw - regression in treatment of anonymous enums in structs

[gprocida at google dot com]

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

gprocida at google dot com changed:

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

--- Comment #17 from gprocida at google dot com ---
I think the code behaviour is well understood.

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