abidiff improvements for kernel UAPI checker

  Hi all,

As you may be aware from the Linux kernel mailing list threads I've been 
CC'ing you on, we're in the process of upstreaming a shell script that 
uses abidiff to compare Linux kernel userspace APIs between revisions. 
Linux UAPIs are supposed to be stable forever, so we're trying to detect 
when a patch breaks a UAPI.

If you haven't been following, the latest thread is here: 
https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/

At a high level, we're taking the header file before/after the patch, 
building it in a dummy C file, then taking advantage of abidiff's 
"--non-reachable-types" argument to compare all the symbols.

This method works great! However, there are a couple of classes of ABI 
differences that are detected but don't qualify as UAPI-breaking.

By far the most common class of differences occur with enum expansions.

For example, this change triggers finding: 
https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a 


Specifically this change to include/uapi/linux/dbcnl.h:

abidiff correctly reports there's an ABI difference:

     [C] 'enum ieee_attrs' changed:
       type size hasn't changed
       1 enumerator insertion:
         'ieee_attrs::DCB_ATTR_DCB_REWR_TABLE' value '12'
       1 enumerator change:
         'ieee_attrs::__DCB_ATTR_IEEE_MAX' from value '12' to '13' at 
dcbnl.h:416:1

It's quite common in the kernel to have the last variant in your enum be 
*_MAX, then define the max value of the enum based on that variant.

However, just adding a variant to this enum doesn't break userspace 
(unless you have a program that does something silly like this):

if (DCB_ATTR_IEEE_MAX > 12) {
     // crash the program
}

Assuming that programs will use the MAX value in a reasonable way, we 
can consider this change safe for userspace backwards-compatibility if 
it meets the following criteria:

   1. No variants are removed/renamed
   2. No variants have a changed value (unless it's the last one)

Technically, we could parse the stdout of abidiff to see if these 
conditions are met, but that'd be quite complicated and fragile. It 
seems a better choice to detect them when the data structures are available.

Do you have any ideas on how these condition could be detected from 
abidiff? We could spend some time to implement the logic, but I'm not 
sure if this kind of specific use case is within the scope of your project.

If so, could something be added like "--ignore-enum-expansion" (or some 
other flag)?

There are other similar "false positives" discussed in the LKML thread, 
but this first one is the largest and probably the easiest to detect 
algorithmically, so I figure it's a good starting point.

Thank you in advance for any help!

- John Moon
  

On 4/10/2023 5:45 PM, John Moon wrote:
> Hi all,
> 
> As you may be aware from the Linux kernel mailing list threads I've been 
> CC'ing you on, we're in the process of upstreaming a shell script that 
> uses abidiff to compare Linux kernel userspace APIs between revisions. 
> Linux UAPIs are supposed to be stable forever, so we're trying to detect 
> when a patch breaks a UAPI.
> 
> If you haven't been following, the latest thread is here: 
> https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/
> 
> At a high level, we're taking the header file before/after the patch, 
> building it in a dummy C file, then taking advantage of abidiff's 
> "--non-reachable-types" argument to compare all the symbols.
> 
> This method works great! However, there are a couple of classes of ABI 
> differences that are detected but don't qualify as UAPI-breaking.
> 
> By far the most common class of differences occur with enum expansions.
> 
> For example, this change triggers finding: 
> https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a

Any inputs here from the libabigail mailing list here?

---Trilok Soni
  

Hello, John & Satya,

John Moon <quic_johmoo@quicinc.com> a écrit:

[...]

> As you may be aware from the Linux kernel mailing list threads I've
> been CC'ing you on, we're in the process of upstreaming a shell script
> that uses abidiff to compare Linux kernel userspace APIs between
> revisions. Linux UAPIs are supposed to be stable forever, so we're
> trying to detect when a patch breaks a UAPI.
>
> If you haven't been following, the latest thread is here:
> https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/
>
> At a high level, we're taking the header file before/after the patch,
> building it in a dummy C file, then taking advantage of abidiff's 
> "--non-reachable-types" argument to compare all the symbols.
>
> This method works great! However, there are a couple of classes of ABI
> differences that are detected but don't qualify as UAPI-breaking.
>
> By far the most common class of differences occur with enum expansions.
>
> For example, this change triggers finding:
> https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a 
>
>
> Specifically this change to include/uapi/linux/dbcnl.h:
> diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> index 99047223ab26..7e15214aa5dd 100644
> --- a/include/uapi/linux/dcbnl.h
> +++ b/include/uapi/linux/dcbnl.h
> @@ -411,6 +411,7 @@ enum dcbnl_attrs {
>   * @DCB_ATTR_IEEE_PEER_PFC: peer PFC configuration - get only
>   * @DCB_ATTR_IEEE_PEER_APP: peer APP tlv - get only
>   * @DCB_ATTR_DCB_APP_TRUST_TABLE: selector trust table
> + * @DCB_ATTR_DCB_REWR_TABLE: rewrite configuration
>   */
>  enum ieee_attrs {
>         DCB_ATTR_IEEE_UNSPEC,
> @@ -425,6 +426,7 @@ enum ieee_attrs {
>         DCB_ATTR_IEEE_QCN_STATS,
>         DCB_ATTR_DCB_BUFFER,
>         DCB_ATTR_DCB_APP_TRUST_TABLE,
> +       DCB_ATTR_DCB_REWR_TABLE,
>         __DCB_ATTR_IEEE_MAX
>  };
>
>
> abidiff correctly reports there's an ABI difference:
>
>     [C] 'enum ieee_attrs' changed:
>       type size hasn't changed
>       1 enumerator insertion:
>         'ieee_attrs::DCB_ATTR_DCB_REWR_TABLE' value '12'
>       1 enumerator change:
>         'ieee_attrs::__DCB_ATTR_IEEE_MAX' from value '12' to '13' at
>         dcbnl.h:416:1
>
> It's quite common in the kernel to have the last variant in your enum
> be *_MAX, then define the max value of the enum based on that variant.
>
> However, just adding a variant to this enum doesn't break userspace
> (unless you have a program that does something silly like this):
>
> if (DCB_ATTR_IEEE_MAX > 12) {
>     // crash the program
> }
>
> Assuming that programs will use the MAX value in a reasonable way, we
> can consider this change safe for userspace backwards-compatibility if 
> it meets the following criteria:
>
>   1. No variants are removed/renamed
>   2. No variants have a changed value (unless it's the last one)

I think feature this would not be difficult to add and would indeed be a
useful feature for others, I believe.

Would it be okay if you'd have to carry a suppression specification
file that would have:

    $ cat filter-enums.suppr

    [suppress_type]
      type_kind = enum
      has_last_enumerator_change_only = true


Then you'd invoke the tool by doing:

    $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2

To get an idea of what suppression specifications do, you can read more
at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications.

Please note that the "has_last_enumerator_change_only" property is not
yet implemented.  I'd like to know first if this "user interface" would
fit your use case.  If it does then we can go ahead and add it.

[...]

> Technically, we could parse the stdout of abidiff to see if these
> conditions are met, but that'd be quite complicated and fragile. It 
> seems a better choice to detect them when the data structures are
> available.

Indeed, that is why I am proposing to extend the suppression
specifications we already have to handle this kind of cases.

> Do you have any ideas on how these condition could be detected from
> abidiff? We could spend some time to implement the logic, but I'm not 
> sure if this kind of specific use case is within the scope of your project.

Suppression specifications are precisely designed to handle this kind of
needs, and yes, that is in the scope of the project.  They are added on
a case by case basis as projects like yours raise the need for it.

> If so, could something be added like "--ignore-enum-expansion" (or
> some other flag)?

I think the suppression specification route would be better as the
combination of cases to "filter out" depending on each use case is
probably too big to handle via command line switches.

> There are other similar "false positives" discussed in the LKML
> thread, but this first one is the largest and probably the easiest to
> detect algorithmically, so I figure it's a good starting point.

Right, I'd be glad to be made aware about those and help tackle them.

> Thank you in advance for any help!

Thank you for considering abidiff and I hope we'll be useful.

Cheers,
  

Hello,

Dodji Seketeli <dodji@seketeli.org> a écrit:

>
> I think feature this would not be difficult to add and would indeed be a
> useful feature for others, I believe.
>
> Would it be okay if you'd have to carry a suppression specification
> file that would have:
>
>     $ cat filter-enums.suppr
>
>     [suppress_type]
>       type_kind = enum
>       has_last_enumerator_change_only = true
>
>
> Then you'd invoke the tool by doing:
>
>     $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2
>
> To get an idea of what suppression specifications do, you can read more
> at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications.
>
> Please note that the "has_last_enumerator_change_only" property is not
> yet implemented.  I'd like to know first if this "user interface" would
> fit your use case.  If it does then we can go ahead and add it.

Actually, I went ahead and played a little with this idea, in case you'd
be interested, so I have a patched libabigail tree with the above
implemented.  The actual property to set in the suppression
specification file is "allow_last_enumerator_change_only".

Here is an example of what it does:

$ cat -n test0-v0.c
     1	enum enum_type
     2	{
     3	  E0,
     4	  E1,
     5	  E_LAST
     6	};
     7
     8	struct some_type
     9	{
    10	  enum enum_type m;
    11	};
    12
    13	void
    14	foo(struct some_type* s __attribute__((unused)))
    15	{
    16	}
$ cat -n test0-v1.c
     1	enum enum_type
     2	{
     3	  E0,
     4	  E1,
     5	  E2,
     6	  E_LAST
     7	};
     8
     9	struct some_type
    10	{
    11	  enum enum_type m;
    12	};
    13
    14	void
    15	foo(struct some_type* s __attribute__((unused)))
    16	{
    17	}
$

    $ ./build/tools/abidiff test0-v0.o test0-v1.o || echo "exit code: $?"
    Functions changes summary: 0 Removed, 1 Changed, 0 Added function
    Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

    1 function with some indirect sub-type change:

      [C] 'function void foo(some_type*)' at test0-v0.c:14:1 has some indirect sub-type changes:
	parameter 1 of type 'some_type*' has sub-type changes:
	  in pointed to type 'struct some_type' at test0-v1.c:9:1:
	    type size hasn't changed
	    1 data member change:
	      type of 'enum_type m' changed:
		type size hasn't changed
		1 enumerator insertion:
		  'enum_type::E2' value '2'
		1 enumerator change:
		  'enum_type::E_LAST' from value '2' to '3' at test0-v1.c:1:1

    exit code: 4
    $ cat last-enum-change-only.suppr
    [suppress_type]
      type_kind = enum
      allow_last_enumerator_change_only = yes
    $
    $ /home/dodji/git/libabigail/filter-last-enumerator-change/build/tools/abidiff --suppr ./last-enum-change-only.suppr test0-v0.o test0-v1.o && echo "exit code: $?"
    Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
    Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

    exit code: 0
    $


The git tree that you can check you, compile and test at your
convenience is https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change.

The patch in question is this one: https://sourceware.org/git/?p=libabigail.git;a=commit;h=239ecaa7dc5de9e9a37650248aa00b5cfc2e578e

The git command to use would be:

$ git clone -b 'users/dodji/filter-last-enumerator-change' git://sourceware.org/git/libabigail.git

Once you've got the necessary dependencies for your system, I guess
compiling the tree would take:

    $ autoreconf
    $ configure --prefix=</your/prefix>
    $ make -j<amount-of-cpu-cores>
    $ make install

Then play with /your/prefix/usr/bin/abidiff.

[...]

I hope this helps to at least start a productive discussion.

Cheers,
  

On 4/21/2023 1:03 PM, Dodji Seketeli wrote:
> Hello,
> 
> Dodji Seketeli <dodji@seketeli.org> a écrit:
> 
>>
>> I think feature this would not be difficult to add and would indeed be a
>> useful feature for others, I believe.
>>
>> Would it be okay if you'd have to carry a suppression specification
>> file that would have:
>>
>>      $ cat filter-enums.suppr
>>
>>      [suppress_type]
>>        type_kind = enum
>>        has_last_enumerator_change_only = true
>>
>>
>> Then you'd invoke the tool by doing:
>>
>>      $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2
>>
>> To get an idea of what suppression specifications do, you can read more
>> at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications.
>>
>> Please note that the "has_last_enumerator_change_only" property is not
>> yet implemented.  I'd like to know first if this "user interface" would
>> fit your use case.  If it does then we can go ahead and add it.
> 
> Actually, I went ahead and played a little with this idea, in case you'd
> be interested, so I have a patched libabigail tree with the above
> implemented.  The actual property to set in the suppression
> specification file is "allow_last_enumerator_change_only".
> 
> Here is an example of what it does:
> 
> $ cat -n test0-v0.c
>       1	enum enum_type
>       2	{
>       3	  E0,
>       4	  E1,
>       5	  E_LAST
>       6	};
>       7
>       8	struct some_type
>       9	{
>      10	  enum enum_type m;
>      11	};
>      12
>      13	void
>      14	foo(struct some_type* s __attribute__((unused)))
>      15	{
>      16	}
> $ cat -n test0-v1.c
>       1	enum enum_type
>       2	{
>       3	  E0,
>       4	  E1,
>       5	  E2,
>       6	  E_LAST
>       7	};
>       8
>       9	struct some_type
>      10	{
>      11	  enum enum_type m;
>      12	};
>      13
>      14	void
>      15	foo(struct some_type* s __attribute__((unused)))
>      16	{
>      17	}
> $
> 
>      $ ./build/tools/abidiff test0-v0.o test0-v1.o || echo "exit code: $?"
>      Functions changes summary: 0 Removed, 1 Changed, 0 Added function
>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
>      1 function with some indirect sub-type change:
> 
>        [C] 'function void foo(some_type*)' at test0-v0.c:14:1 has some indirect sub-type changes:
> 	parameter 1 of type 'some_type*' has sub-type changes:
> 	  in pointed to type 'struct some_type' at test0-v1.c:9:1:
> 	    type size hasn't changed
> 	    1 data member change:
> 	      type of 'enum_type m' changed:
> 		type size hasn't changed
> 		1 enumerator insertion:
> 		  'enum_type::E2' value '2'
> 		1 enumerator change:
> 		  'enum_type::E_LAST' from value '2' to '3' at test0-v1.c:1:1
> 
>      exit code: 4
>      $ cat last-enum-change-only.suppr
>      [suppress_type]
>        type_kind = enum
>        allow_last_enumerator_change_only = yes
>      $
>      $ /home/dodji/git/libabigail/filter-last-enumerator-change/build/tools/abidiff --suppr ./last-enum-change-only.suppr test0-v0.o test0-v1.o && echo "exit code: $?"
>      Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
>      exit code: 0
>      $
> 
> 
> The git tree that you can check you, compile and test at your
> convenience is https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change.
> 
> The patch in question is this one: https://sourceware.org/git/?p=libabigail.git;a=commit;h=239ecaa7dc5de9e9a37650248aa00b5cfc2e578e
> 
> The git command to use would be:
> 
> $ git clone -b 'users/dodji/filter-last-enumerator-change' git://sourceware.org/git/libabigail.git
> 
> Once you've got the necessary dependencies for your system, I guess
> compiling the tree would take:
> 
>      $ autoreconf
>      $ configure --prefix=</your/prefix>
>      $ make -j<amount-of-cpu-cores>
>      $ make install
> 
> Then play with /your/prefix/usr/bin/abidiff.
> 
> [...]
> 
> I hope this helps to at least start a productive discussion.
> 
> Cheers,
> 

Hi Dodji,

Thanks so much for getting the ball rolling on this! I agree that 
suppressions seem to be the correct mechanism here and I don't foresee 
any issue using them from the check script we're working on.

Before seeing this, I had gotten started on implementing some (hacky) 
filters based on the output of abidiff. While testing on the Linux 
codebase, I found some interesting corner cases. I'll go into the 
details here so hopefully we can have more discussion and design the 
right solution.

There are basically three classes of "false positives" that we'd like to 
eventually be able to filter:

1) Enum expansion
2) Padding field expansion
3) Flex array expansion


=== Enum expansion ===
Here are some places in the kernel where the script reported expansion 
as a breaking change:

One "MAX" member: 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789

Two "MAX" members: 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398

Three "MAX" members: 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311

Having two is pretty common. I can only find the one occurrence of 
having three of these variants... maybe the suppression could take an 
"n_allowed" parameter of some sort?

Also, would it make sense to limit the check to enumerator variants that 
have somewhat common names for that case? It could evolve with time, but 
so far I've found "MAX", "LAST", "NUM", and "NBITS" being fairly common. 
Maybe the suppression could also take "allowed_strings"? Something like 
this?

[suppress_type]
   type_kind = enum
   allow_n_last_enumerator_changes = 3
   allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", 
".*NUM.*", ".*NBITS.*"]


This is the algorithm I was working on to detect these changes:
   - The type size did not change
   - The change is to an enum
   - The number of changed variants is < 4
   - The changed variants contain at least one of the above strings
   - The changed variants' values are all >= the last newly-inserted value


=== Padding field expansion ===
For example, you'll see lots of padding fields like this: 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908

These don't necessarily have to occur at the end of a struct. You could 
expand into a padding field in the middle somewhere.

This is the algorithm I was working on to detect these changes:
   - The type size did not change
   - There is only one data member being changed
   - The changed member name contains at least one of: "pad", 
"reserved", "end", or "last"
   - The first new member being inserted has an offset equal to the 
changed member's previous offset


=== Flex array expansion ===
For example: 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149.

Leaving a flex array at the end essentially gives the struct an 
"infinite" padding field. The algorithm to detect looks almost identical 
to the ordinary padding expansion except this class can only occur with 
the flex array at the end of the struct.


=== Anonymous Enums ===
Another issue that comes up when comparing ABI across wide swaths of 
kernel commit history are changes to anonymous enums. From what I can 
tell, there's not a great way to handle this. If someone adds a new 
anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) 
can change, so abidiff reports a new anonymous enum with all the same 
fields even though it was basically just a "rename".

For reference, this file is full of anonymous enums: 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. 
Basically any change in there is triggering a failure from the script.

I'm not sure how to handle these changes... there are possible ABI 
breakages when changing anonymous enums, but it's unclear to me how to 
detect them.

Again, thanks so much for taking the initiative here. Let me know what 
you think about the above topics and what next steps we could take!

- John
  

Hello John,

[...]

> Thanks so much for getting the ball rolling on this!

You are welcome!

> I agree that suppressions seem to be the correct mechanism here and
> I don't foresee any issue using them from the check script we're
> working on.

Nice to hear :-)

> Before seeing this, I had gotten started on implementing some (hacky)
> filters based on the output of abidiff. While testing on the Linux 
> codebase, I found some interesting corner cases. I'll go into the
> details here so hopefully we can have more discussion and design the 
> right solution.

Thank you.

> There are basically three classes of "false positives" that we'd like
> to eventually be able to filter:
> 
> 1) Enum expansion
> 2) Padding field expansion
> 3) Flex array expansion

ACK.

> === Enum expansion ===
> Here are some places in the kernel where the script reported expansion
> as a breaking change:
> 
> One "MAX" member:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789
> 
> Two "MAX" members:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398
> 
> Three "MAX" members:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311
> 
> Having two is pretty common. I can only find the one occurrence of
> having three of these variants... maybe the suppression could take an 
> "n_allowed" parameter of some sort?

I guess this can certainly be done, yes.  And it does make sense to
me.

> Also, would it make sense to limit the check to enumerator variants
> that have somewhat common names for that case? It could evolve with
> time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being
> fairly common. Maybe the suppression could also take
> "allowed_strings"? Something like this?

Just so you know, there is already the "changed_enumerators" property
that is available[1].  It takes the list of enumerators that are allowed
to change.  It doesn't support regular expressions, yet.  Maybe we can
either make it take enumerators or add a new one named
"changed_enumerators_regexp" which takes a list of regular expressions
describing the allowed changed enumerators.  In any case, that
property will have to be made to work in tandem with the
allow_n_last_enumarator_changes property.

[1]: https://sourceware.org/libabigail/manual/libabigail-concepts.
[Look for the "changed_enumerator" word in there]


> [suppress_type]
>   type_kind = enum
>   allow_n_last_enumerator_changes = 3
>   allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", ".*NUM.*", ".*NBITS.*"]

Right.  Or, rather, the syntax would be:

 [suppress_type]
   type_kind = enum
   allow_n_last_enumerator_changes = 3
   changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.*

(no need for the string delimiters as everything is considered a
 string by default)

Would that be OK?

> This is the algorithm I was working on to detect these changes:
>   - The type size did not change
>   - The change is to an enum
>   - The number of changed variants is < 4
>   - The changed variants contain at least one of the above strings
>   - The changed variants' values are all >= the last newly-inserted value

ACK.

> === Padding field expansion ===
> For example, you'll see lots of padding fields like this:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908
> 
> These don't necessarily have to occur at the end of a struct. You
> could expand into a padding field in the middle somewhere.
> 
> This is the algorithm I was working on to detect these changes:
>   - The type size did not change
>   - There is only one data member being changed
>   - The changed member name contains at least one of: "pad",
>     "reserved", "end", or "last"
>   - The first new member being inserted has an offset equal to the
>     changed member's previous offset

Just so you know, there are a number of properties of the
[suppress_type] section that are designed to address this kind of
suppressions.  I think some can be extended to address this use case.

E.g, there is the has_data_member_inserted_at property which I believe
can help with this use case.  Here is a quick screen shot to show you
what it does so far, in the latest 2.3 libabigail version that got
recently released:

    $ cat -n test-v0.cc
	 1	#include <cstdint>
	 2	
	 3	struct S
	 4	{
	 5	  int a;
	 6	  char padding[64];
	 7	  int b;
	 8	};
	 9	
	10	void
	11	foo(S&)
	12	{}

    $ cat -n test-v1.cc
	 1	#include <cstdint>
	 2	
	 3	struct S
	 4	{
	 5	  int a;
	 6	  uint32_t added;
	 7	  char padding[60];
	 8	  int b;
	 9	};
	10	
	11	void
	12	foo(S&)
	13	{}

    $ diff -u test-v0.cc test-v1.cc
    --- test-v0.cc	2023-05-10 13:51:19.798072787 +0200
    +++ test-v1.cc	2023-05-10 13:54:53.879082831 +0200
    @@ -3,7 +3,8 @@
     struct S
     {
       int a;
    -  char padding[64];
    +  uint32_t added;
    +  char padding[60];
       int b;
     };

    $ gcc -g -c test-v{0,1}.cc
    $ abidiff test-v0.o test-v1.o
    Functions changes summary: 0 Removed, 1 Changed, 0 Added function
    Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

    1 function with some indirect sub-type change:

      [C] 'function void foo(S&)' at test-v0.cc:11:1 has some indirect sub-type changes:
	parameter 1 of type 'S&' has sub-type changes:
	  in referenced type 'struct S' at test-v1.cc:3:1:
	    type size hasn't changed
	    1 data member insertion:
	      'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1
	    1 data member change:
	      type of 'char padding[64]' changed:
		type name changed from 'char[64]' to 'char[60]'
		array type size changed from 512 to 480
		array type subrange 1 changed length from 64 to 60
	      and offset changed from 32 to 64 (in bits) (by +32 bits)

    $ cat padding.suppr
    [suppress_type]
      type_kind = struct
      has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding)

    $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr padding.suppr test-v0.o test-v1.o
    Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
    Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

    $ 

> 
> 
> === Flex array expansion ===
> For example:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149.
> 
> Leaving a flex array at the end essentially gives the struct an
> "infinite" padding field. The algorithm to detect looks almost
> identical to the ordinary padding expansion except this class can only
> occur with the flex array at the end of the struct.

I am not sure to understand this case in light of what we have been
talking about earlier.  If a data member is inserted anywhere around
the flex array field, I would have thought that it should be flagged
as being a meaningful change to report.  What am I missing?

> === Anonymous Enums ===
> Another issue that comes up when comparing ABI across wide swaths of
> kernel commit history are changes to anonymous enums. From what I can 
> tell, there's not a great way to handle this. If someone adds a new
> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) 
> can change, so abidiff reports a new anonymous enum with all the same
> fields even though it was basically just a "rename".
> 
> For reference, this file is full of anonymous enums:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. 
> Basically any change in there is triggering a failure from the
> script.

By 'any change', do you mean any addition of enumerator?

I think we can do something similar to what we do for anonymous
structs/unions, which it to consider content of the enums, rather than
just their "made-up" names.

I guess we need to have well defined examples of changes we want to
handle and I think we can come to a solution.

> I'm not sure how to handle these changes... there are possible ABI
> breakages when changing anonymous enums, but it's unclear to me how to 
> detect them.

Let's come up with some small examples first, I'd say.

> Again, thanks so much for taking the initiative here. Let me know what
> you think about the above topics and what next steps we could take!

No problem, my pleasure.

Cheers,
  

On 5/10/2023 7:21 AM, Dodji Seketeli wrote:
>> === Enum expansion ===
>> Here are some places in the kernel where the script reported expansion
>> as a breaking change:
>>
>> One "MAX" member:
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789
>>
>> Two "MAX" members:
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398
>>
>> Three "MAX" members:
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311
>>
>> Having two is pretty common. I can only find the one occurrence of
>> having three of these variants... maybe the suppression could take an
>> "n_allowed" parameter of some sort?
> 
> I guess this can certainly be done, yes.  And it does make sense to
> me.
> 
>> Also, would it make sense to limit the check to enumerator variants
>> that have somewhat common names for that case? It could evolve with
>> time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being
>> fairly common. Maybe the suppression could also take
>> "allowed_strings"? Something like this?
> 
> Just so you know, there is already the "changed_enumerators" property
> that is available[1].  It takes the list of enumerators that are allowed
> to change.  It doesn't support regular expressions, yet.  Maybe we can
> either make it take enumerators or add a new one named
> "changed_enumerators_regexp" which takes a list of regular expressions
> describing the allowed changed enumerators.  In any case, that
> property will have to be made to work in tandem with the
> allow_n_last_enumarator_changes property.
> 
> [1]: https://sourceware.org/libabigail/manual/libabigail-concepts.
> [Look for the "changed_enumerator" word in there]
> 
> 
>> [suppress_type]
>>    type_kind = enum
>>    allow_n_last_enumerator_changes = 3
>>    allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", ".*NUM.*", ".*NBITS.*"]
> 
> Right.  Or, rather, the syntax would be:
> 
>   [suppress_type]
>     type_kind = enum
>     allow_n_last_enumerator_changes = 3
>     changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.*
> 
> (no need for the string delimiters as everything is considered a
>   string by default)
> 
> Would that be OK?

Yes, this looks perfect!

>> === Padding field expansion ===
>> For example, you'll see lots of padding fields like this:
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908
>>
>> These don't necessarily have to occur at the end of a struct. You
>> could expand into a padding field in the middle somewhere.
>>
>> This is the algorithm I was working on to detect these changes:
>>    - The type size did not change
>>    - There is only one data member being changed
>>    - The changed member name contains at least one of: "pad",
>>      "reserved", "end", or "last"
>>    - The first new member being inserted has an offset equal to the
>>      changed member's previous offset
> 
> Just so you know, there are a number of properties of the
> [suppress_type] section that are designed to address this kind of
> suppressions.  I think some can be extended to address this use case.
> 
> E.g, there is the has_data_member_inserted_at property which I believe
> can help with this use case.  Here is a quick screen shot to show you
> what it does so far, in the latest 2.3 libabigail version that got
> recently released:
> 
>      $ cat -n test-v0.cc
> 	 1	#include <cstdint>
> 	 2	
> 	 3	struct S
> 	 4	{
> 	 5	  int a;
> 	 6	  char padding[64];
> 	 7	  int b;
> 	 8	};
> 	 9	
> 	10	void
> 	11	foo(S&)
> 	12	{}
> 
>      $ cat -n test-v1.cc
> 	 1	#include <cstdint>
> 	 2	
> 	 3	struct S
> 	 4	{
> 	 5	  int a;
> 	 6	  uint32_t added;
> 	 7	  char padding[60];
> 	 8	  int b;
> 	 9	};
> 	10	
> 	11	void
> 	12	foo(S&)
> 	13	{}
> 
>      $ diff -u test-v0.cc test-v1.cc
>      --- test-v0.cc	2023-05-10 13:51:19.798072787 +0200
>      +++ test-v1.cc	2023-05-10 13:54:53.879082831 +0200
>      @@ -3,7 +3,8 @@
>       struct S
>       {
>         int a;
>      -  char padding[64];
>      +  uint32_t added;
>      +  char padding[60];
>         int b;
>       };
> 
>      $ gcc -g -c test-v{0,1}.cc
>      $ abidiff test-v0.o test-v1.o
>      Functions changes summary: 0 Removed, 1 Changed, 0 Added function
>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
>      1 function with some indirect sub-type change:
> 
>        [C] 'function void foo(S&)' at test-v0.cc:11:1 has some indirect sub-type changes:
> 	parameter 1 of type 'S&' has sub-type changes:
> 	  in referenced type 'struct S' at test-v1.cc:3:1:
> 	    type size hasn't changed
> 	    1 data member insertion:
> 	      'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1
> 	    1 data member change:
> 	      type of 'char padding[64]' changed:
> 		type name changed from 'char[64]' to 'char[60]'
> 		array type size changed from 512 to 480
> 		array type subrange 1 changed length from 64 to 60
> 	      and offset changed from 32 to 64 (in bits) (by +32 bits)
> 
>      $ cat padding.suppr
>      [suppress_type]
>        type_kind = struct
>        has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding)
> 
>      $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr padding.suppr test-v0.o test-v1.o
>      Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
> 
>      $
> 

This looks very close! Could we just repeat the suppression for each of 
the member names we're interested in ignoring or would we need to add 
support for the "offset_of_first_data_member_regexp()" function to take 
in a list of regexp?

>>
>>
>> === Flex array expansion ===
>> For example:
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149.
>>
>> Leaving a flex array at the end essentially gives the struct an
>> "infinite" padding field. The algorithm to detect looks almost
>> identical to the ordinary padding expansion except this class can only
>> occur with the flex array at the end of the struct.
> 
> I am not sure to understand this case in light of what we have been
> talking about earlier.  If a data member is inserted anywhere around
> the flex array field, I would have thought that it should be flagged
> as being a meaningful change to report.  What am I missing?

Flex arrays can only be at the end of a struct, so I guess the rule 
should be "you can add members between existing members and the ending 
flex array".

E.g. this should be fine:

  struct foo {
      __u32 a;
      __u32 b;
+    __u32 c;
      char pad[];
  };

But not this:

  struct foo {
      __u32 a;
+    __u32 c;
      __u32 b;
      char pad[];
  };


>> === Anonymous Enums ===
>> Another issue that comes up when comparing ABI across wide swaths of
>> kernel commit history are changes to anonymous enums. From what I can
>> tell, there's not a great way to handle this. If someone adds a new
>> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6)
>> can change, so abidiff reports a new anonymous enum with all the same
>> fields even though it was basically just a "rename".
>>
>> For reference, this file is full of anonymous enums:
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15.
>> Basically any change in there is triggering a failure from the
>> script.
> 
> By 'any change', do you mean any addition of enumerator?
> 
> I think we can do something similar to what we do for anonymous
> structs/unions, which it to consider content of the enums, rather than
> just their "made-up" names.
> 
> I guess we need to have well defined examples of changes we want to
> handle and I think we can come to a solution.
> 
>> I'm not sure how to handle these changes... there are possible ABI
>> breakages when changing anonymous enums, but it's unclear to me how to
>> detect them.
> 
> Let's come up with some small examples first, I'd say.

Here's one example of the issue: 
https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d 
(specifically the change to include/uapi/rdma/hns-abi.h).

Here, a variant was added to two anonymous enums:

  enum {
      HNS_ROCE_EXSGE_FLAGS = 1 << 0,
      HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1,
+    HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2,
  };

  enum {
       HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0,
       HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1,
+     HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2,
  };

abidiff outputs:

     [C] 'enum __anonymous_enum__1' changed:
       type size hasn't changed
       1 enumerator deletion:
         '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1'
       3 enumerator insertions:
         '__anonymous_enum__::HNS_ROCE_EXSGE_FLAGS' value '1'
         '__anonymous_enum__::HNS_ROCE_RQ_INLINE_FLAGS' value '2'
         '__anonymous_enum__::HNS_ROCE_CQE_INLINE_FLAGS' value '4'
   1 added type unreachable from any public interface:
     [A] 'enum __anonymous_enum__1' at hns-abi.h:94:1

It seems like it mixed them up while processing? I'm not exactly sure 
what's going on internally.

Another example: 
https://github.com/w1ldptr/linux/commit/d219df60a70ed0739aa5dd34b477763311fc5a7b 
(specifically the change to include/uapi/linux/bpf.h).

Here, just one enum was modified:

  enum {
      BPF_F_ADJ_ROOM_FIXED_GSO	= (1ULL << 0),
      BPF_F_ADJ_ROOM_ENCAP_L3_IPV4	= (1ULL << 1),
      BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	= (1ULL << 2),
      BPF_F_ADJ_ROOM_ENCAP_L4_GRE	= (1ULL << 3),
      BPF_F_ADJ_ROOM_ENCAP_L4_UDP	= (1ULL << 4),
      BPF_F_ADJ_ROOM_NO_CSUM_RESET	= (1ULL << 5),
      BPF_F_ADJ_ROOM_ENCAP_L2_ETH	= (1ULL << 6),
+    BPF_F_ADJ_ROOM_DECAP_L3_IPV4	= (1ULL << 7),
+    BPF_F_ADJ_ROOM_DECAP_L3_IPV6	= (1ULL << 8),
  };


But abidiff outputs:

     [C] 'enum __anonymous_enum__9' changed:
       type size hasn't changed
       3 enumerator deletions:
         '__anonymous_enum__9::BPF_F_ZERO_CSUM_TX' value '2'
         '__anonymous_enum__9::BPF_F_DONT_FRAGMENT' value '4'
         '__anonymous_enum__9::BPF_F_SEQ_NUMBER' value '8'
       9 enumerator insertions:
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_FIXED_GSO' value '1'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV4' value '2'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV6' value '4'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_GRE' value '8'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_UDP' value '16'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_NO_CSUM_RESET' value '32'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L2_ETH' value '64'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV4' value '128'
         '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV6' value '256'
   3 added types unreachable from any public interface:
     [A] 'enum __anonymous_enum__9' at bpf.h:5781:1
     [A] 'struct bpf_rb_node' at bpf.h:6931:1
     [A] 'struct bpf_rb_root' at bpf.h:6926:1

Any ideas on how to handle these issues?

Thank you!

- John
  

On 5/23/2023 12:59 PM, John Moon wrote:
> On 5/10/2023 7:21 AM, Dodji Seketeli wrote:
>>> === Enum expansion ===
>>> Here are some places in the kernel where the script reported expansion
>>> as a breaking change:
>>>
>>> One "MAX" member:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1789
>>>
>>> Two "MAX" members:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L6398
>>>
>>> Three "MAX" members:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/nl80211.h#L3311
>>>
>>> Having two is pretty common. I can only find the one occurrence of
>>> having three of these variants... maybe the suppression could take an
>>> "n_allowed" parameter of some sort?
>>
>> I guess this can certainly be done, yes.  And it does make sense to
>> me.
>>
>>> Also, would it make sense to limit the check to enumerator variants
>>> that have somewhat common names for that case? It could evolve with
>>> time, but so far I've found "MAX", "LAST", "NUM", and "NBITS" being
>>> fairly common. Maybe the suppression could also take
>>> "allowed_strings"? Something like this?
>>
>> Just so you know, there is already the "changed_enumerators" property
>> that is available[1].  It takes the list of enumerators that are allowed
>> to change.  It doesn't support regular expressions, yet.  Maybe we can
>> either make it take enumerators or add a new one named
>> "changed_enumerators_regexp" which takes a list of regular expressions
>> describing the allowed changed enumerators.  In any case, that
>> property will have to be made to work in tandem with the
>> allow_n_last_enumarator_changes property.
>>
>> [1]: https://sourceware.org/libabigail/manual/libabigail-concepts.
>> [Look for the "changed_enumerator" word in there]
>>
>>
>>> [suppress_type]
>>>    type_kind = enum
>>>    allow_n_last_enumerator_changes = 3
>>>    allow_n_last_enumerator_change_names = [".*MAX.*", ".*LAST.*", 
>>> ".*NUM.*", ".*NBITS.*"]
>>
>> Right.  Or, rather, the syntax would be:
>>
>>   [suppress_type]
>>     type_kind = enum
>>     allow_n_last_enumerator_changes = 3
>>     changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.*
>>
>> (no need for the string delimiters as everything is considered a
>>   string by default)
>>
>> Would that be OK?
> 
> Yes, this looks perfect!
> 
>>> === Padding field expansion ===
>>> For example, you'll see lots of padding fields like this:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fuse.h#L908
>>>
>>> These don't necessarily have to occur at the end of a struct. You
>>> could expand into a padding field in the middle somewhere.
>>>
>>> This is the algorithm I was working on to detect these changes:
>>>    - The type size did not change
>>>    - There is only one data member being changed
>>>    - The changed member name contains at least one of: "pad",
>>>      "reserved", "end", or "last"
>>>    - The first new member being inserted has an offset equal to the
>>>      changed member's previous offset
>>
>> Just so you know, there are a number of properties of the
>> [suppress_type] section that are designed to address this kind of
>> suppressions.  I think some can be extended to address this use case.
>>
>> E.g, there is the has_data_member_inserted_at property which I believe
>> can help with this use case.  Here is a quick screen shot to show you
>> what it does so far, in the latest 2.3 libabigail version that got
>> recently released:
>>
>>      $ cat -n test-v0.cc
>>      1    #include <cstdint>
>>      2
>>      3    struct S
>>      4    {
>>      5      int a;
>>      6      char padding[64];
>>      7      int b;
>>      8    };
>>      9
>>     10    void
>>     11    foo(S&)
>>     12    {}
>>
>>      $ cat -n test-v1.cc
>>      1    #include <cstdint>
>>      2
>>      3    struct S
>>      4    {
>>      5      int a;
>>      6      uint32_t added;
>>      7      char padding[60];
>>      8      int b;
>>      9    };
>>     10
>>     11    void
>>     12    foo(S&)
>>     13    {}
>>
>>      $ diff -u test-v0.cc test-v1.cc
>>      --- test-v0.cc    2023-05-10 13:51:19.798072787 +0200
>>      +++ test-v1.cc    2023-05-10 13:54:53.879082831 +0200
>>      @@ -3,7 +3,8 @@
>>       struct S
>>       {
>>         int a;
>>      -  char padding[64];
>>      +  uint32_t added;
>>      +  char padding[60];
>>         int b;
>>       };
>>
>>      $ gcc -g -c test-v{0,1}.cc
>>      $ abidiff test-v0.o test-v1.o
>>      Functions changes summary: 0 Removed, 1 Changed, 0 Added function
>>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>>
>>      1 function with some indirect sub-type change:
>>
>>        [C] 'function void foo(S&)' at test-v0.cc:11:1 has some 
>> indirect sub-type changes:
>>     parameter 1 of type 'S&' has sub-type changes:
>>       in referenced type 'struct S' at test-v1.cc:3:1:
>>         type size hasn't changed
>>         1 data member insertion:
>>           'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1
>>         1 data member change:
>>           type of 'char padding[64]' changed:
>>         type name changed from 'char[64]' to 'char[60]'
>>         array type size changed from 512 to 480
>>         array type subrange 1 changed length from 64 to 60
>>           and offset changed from 32 to 64 (in bits) (by +32 bits)
>>
>>      $ cat padding.suppr
>>      [suppress_type]
>>        type_kind = struct
>>        has_data_member_inserted_at = 
>> offset_of_first_data_member_regexp(.*padding)
>>
>>      $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr 
>> padding.suppr test-v0.o test-v1.o
>>      Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 
>> 0 Added function
>>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>>
>>      $
>>
> 
> This looks very close! Could we just repeat the suppression for each of 
> the member names we're interested in ignoring or would we need to add 
> support for the "offset_of_first_data_member_regexp()" function to take 
> in a list of regexp?
> 
>>>
>>>
>>> === Flex array expansion ===
>>> For example:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149.
>>>
>>> Leaving a flex array at the end essentially gives the struct an
>>> "infinite" padding field. The algorithm to detect looks almost
>>> identical to the ordinary padding expansion except this class can only
>>> occur with the flex array at the end of the struct.
>>
>> I am not sure to understand this case in light of what we have been
>> talking about earlier.  If a data member is inserted anywhere around
>> the flex array field, I would have thought that it should be flagged
>> as being a meaningful change to report.  What am I missing?
> 
> Flex arrays can only be at the end of a struct, so I guess the rule 
> should be "you can add members between existing members and the ending 
> flex array".
> 
> E.g. this should be fine:
> 
>   struct foo {
>       __u32 a;
>       __u32 b;
> +    __u32 c;
>       char pad[];
>   };
> 
> But not this:
> 
>   struct foo {
>       __u32 a;
> +    __u32 c;
>       __u32 b;
>       char pad[];
>   };
> 
> 
>>> === Anonymous Enums ===
>>> Another issue that comes up when comparing ABI across wide swaths of
>>> kernel commit history are changes to anonymous enums. From what I can
>>> tell, there's not a great way to handle this. If someone adds a new
>>> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6)
>>> can change, so abidiff reports a new anonymous enum with all the same
>>> fields even though it was basically just a "rename".
>>>
>>> For reference, this file is full of anonymous enums:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15.
>>> Basically any change in there is triggering a failure from the
>>> script.
>>
>> By 'any change', do you mean any addition of enumerator?
>>
>> I think we can do something similar to what we do for anonymous
>> structs/unions, which it to consider content of the enums, rather than
>> just their "made-up" names.
>>
>> I guess we need to have well defined examples of changes we want to
>> handle and I think we can come to a solution.
>>
>>> I'm not sure how to handle these changes... there are possible ABI
>>> breakages when changing anonymous enums, but it's unclear to me how to
>>> detect them.
>>
>> Let's come up with some small examples first, I'd say.
> 
> Here's one example of the issue: 
> https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d (specifically the change to include/uapi/rdma/hns-abi.h).
> 
> Here, a variant was added to two anonymous enums:
> 
>   enum {
>       HNS_ROCE_EXSGE_FLAGS = 1 << 0,
>       HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1,
> +    HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2,
>   };
> 
>   enum {
>        HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0,
>        HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1,
> +     HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2,
>   };
> 
> abidiff outputs:
> 
>      [C] 'enum __anonymous_enum__1' changed:
>        type size hasn't changed
>        1 enumerator deletion:
>          '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1'
>        3 enumerator insertions:
>          '__anonymous_enum__::HNS_ROCE_EXSGE_FLAGS' value '1'
>          '__anonymous_enum__::HNS_ROCE_RQ_INLINE_FLAGS' value '2'
>          '__anonymous_enum__::HNS_ROCE_CQE_INLINE_FLAGS' value '4'
>    1 added type unreachable from any public interface:
>      [A] 'enum __anonymous_enum__1' at hns-abi.h:94:1
> 
> It seems like it mixed them up while processing? I'm not exactly sure 
> what's going on internally.
> 
> Another example: 
> https://github.com/w1ldptr/linux/commit/d219df60a70ed0739aa5dd34b477763311fc5a7b (specifically the change to include/uapi/linux/bpf.h).
> 
> Here, just one enum was modified:
> 
>   enum {
>       BPF_F_ADJ_ROOM_FIXED_GSO    = (1ULL << 0),
>       BPF_F_ADJ_ROOM_ENCAP_L3_IPV4    = (1ULL << 1),
>       BPF_F_ADJ_ROOM_ENCAP_L3_IPV6    = (1ULL << 2),
>       BPF_F_ADJ_ROOM_ENCAP_L4_GRE    = (1ULL << 3),
>       BPF_F_ADJ_ROOM_ENCAP_L4_UDP    = (1ULL << 4),
>       BPF_F_ADJ_ROOM_NO_CSUM_RESET    = (1ULL << 5),
>       BPF_F_ADJ_ROOM_ENCAP_L2_ETH    = (1ULL << 6),
> +    BPF_F_ADJ_ROOM_DECAP_L3_IPV4    = (1ULL << 7),
> +    BPF_F_ADJ_ROOM_DECAP_L3_IPV6    = (1ULL << 8),
>   };
> 
> 
> But abidiff outputs:
> 
>      [C] 'enum __anonymous_enum__9' changed:
>        type size hasn't changed
>        3 enumerator deletions:
>          '__anonymous_enum__9::BPF_F_ZERO_CSUM_TX' value '2'
>          '__anonymous_enum__9::BPF_F_DONT_FRAGMENT' value '4'
>          '__anonymous_enum__9::BPF_F_SEQ_NUMBER' value '8'
>        9 enumerator insertions:
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_FIXED_GSO' value '1'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV4' value '2'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV6' value '4'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_GRE' value '8'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_UDP' value '16'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_NO_CSUM_RESET' value '32'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L2_ETH' value '64'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV4' value '128'
>          '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV6' value '256'
>    3 added types unreachable from any public interface:
>      [A] 'enum __anonymous_enum__9' at bpf.h:5781:1
>      [A] 'struct bpf_rb_node' at bpf.h:6931:1
>      [A] 'struct bpf_rb_root' at bpf.h:6926:1
> 
> Any ideas on how to handle these issues?
> 
> Thank you!
> 
> - John
>

Hi Dodji,

I wanted to follow up with you and see if/how we can assist with these 
features. I'm happy to jump in and try implementation if we agree on the 
feature requirements, though I'm not very familiar with the codebase.

To summarize, here are the four usecases we have so far and plans to 
address them:

1. Enum expansion
    - Add support for this kind of suppression:
    [suppress_type]
      type_kind = enum
      allow_n_last_enumerator_changes = 3
      changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.*


2. Padding field expansion
    - Current feature set is close, but how can we handle multiple names?
    - Do we want support for this kind suppression (multiple regex's)?
    [suppress_type]
       type_kind = struct
     has_data_member_inserted_at = 
offset_of_first_data_member_regexp(.*padding, .*reserved, .*unused)


3. Flex array expansion
    - Not sure how to detect/suppress this condition
    - Flex arrays can only be at the ends of structs, so we just need to 
make sure new struct members come "second to last"


4. Anonymous enums
    - Not sure how to associate these enums when diffing since their 
"made up" names don't always match before/after a patch.


I can try tackling items 1 and 2 if you'd like - just let me know! For 
items 3 and 4, can we work up a path forward?

Thanks so much!

- John
  

Hello John,

Sorry for the delayed reply.

[...]


> On 5/10/2023 7:21 AM, Dodji Seketeli wrote:

[...]

>>> === Enum expansion ===

[...]


>> the syntax would be:
>>   [suppress_type]
>>     type_kind = enum
>>     allow_n_last_enumerator_changes = 3
>>     changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.*
>> (no need for the string delimiters as everything is considered a
>>   string by default)
>> Would that be OK?

John Moon wrote:

>
> Yes, this looks perfect!
>

OK, deal.

>>> === Padding field expansion ===

[...]



>>      $ cat -n test-v0.cc
>> 	 1	#include <cstdint>
>> 	 2	
>> 	 3	struct S
>> 	 4	{
>> 	 5	  int a;
>> 	 6	  char padding[64];
>> 	 7	  int b;
>> 	 8	};
>> 	 9	
>> 	10	void
>> 	11	foo(S&)
>> 	12	{}
>>      $ cat -n test-v1.cc
>> 	 1	#include <cstdint>
>> 	 2	
>> 	 3	struct S
>> 	 4	{
>> 	 5	  int a;
>> 	 6	  uint32_t added;
>> 	 7	  char padding[60];
>> 	 8	  int b;
>> 	 9	};
>> 	10	
>> 	11	void
>> 	12	foo(S&)
>> 	13	{}
>>      $ diff -u test-v0.cc test-v1.cc
>>      --- test-v0.cc	2023-05-10 13:51:19.798072787 +0200
>>      +++ test-v1.cc	2023-05-10 13:54:53.879082831 +0200
>>      @@ -3,7 +3,8 @@
>>       struct S
>>       {
>>         int a;
>>      -  char padding[64];
>>      +  uint32_t added;
>>      +  char padding[60];
>>         int b;
>>       };
>>      $ gcc -g -c test-v{0,1}.cc
>>      $ abidiff test-v0.o test-v1.o
>>      Functions changes summary: 0 Removed, 1 Changed, 0 Added function
>>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>>      1 function with some indirect sub-type change:
>>        [C] 'function void foo(S&)' at test-v0.cc:11:1 has some
>> indirect sub-type changes:
>> 	parameter 1 of type 'S&' has sub-type changes:
>> 	  in referenced type 'struct S' at test-v1.cc:3:1:
>> 	    type size hasn't changed
>> 	    1 data member insertion:
>> 	      'uint32_t added', at offset 32 (in bits) at test-v1.cc:6:1
>> 	    1 data member change:
>> 	      type of 'char padding[64]' changed:
>> 		type name changed from 'char[64]' to 'char[60]'
>> 		array type size changed from 512 to 480
>> 		array type subrange 1 changed length from 64 to 60
>> 	      and offset changed from 32 to 64 (in bits) (by +32 bits)
>>      $ cat padding.suppr
>>      [suppress_type]
>>        type_kind = struct
>>        has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding)
>>      $ /home/dodji/git/libabigail/master/build/tools/abidiff --suppr
>> padding.suppr test-v0.o test-v1.o
>>      Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
>>      Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>>      $
>>

John Moon wrote:

>
> This looks very close! Could we just repeat the suppression for each
> of the member names we're interested in ignoring or would we need to
> add support for the "offset_of_first_data_member_regexp()" function to
> take in a list of regexp?

This has been designed so that (I haven't tested it right now) having one [suppress_type]
directive per data member insertion we want should work, E.g:

      [suppress_type]
        type_kind = struct
        # Suppress a data member insertion after a data member
        # ending with the suffix 'padding'.
        has_data_member_inserted_at = offset_of_first_data_member_regexp(.*padding)

      [suppress_type]
        type_kind = struct
        # Suppress a data member insertion after a data member ending
        # with the suffix 'buffer'
        has_data_member_inserted_at = offset_of_first_data_member_regexp(.*buffer)


John Moon wrote:

>>> === Flex array expansion ===
>>> For example:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/rseq.h#L149.
>>>
>>> Leaving a flex array at the end essentially gives the struct an
>>> "infinite" padding field. The algorithm to detect looks almost
>>> identical to the ordinary padding expansion except this class can only
>>> occur with the flex array at the end of the struct.


Dodji Seketeli wrote:

>> I am not sure to understand this case in light of what we have been
>> talking about earlier.  If a data member is inserted anywhere around
>> the flex array field, I would have thought that it should be flagged
>> as being a meaningful change to report.  What am I missing?

John Moon wrote:

>
> Flex arrays can only be at the end of a struct, so I guess the rule
> should be "you can add members between existing members and the ending 
> flex array".
>
> E.g. this should be fine:
>
>  struct foo {
>      __u32 a;
>      __u32 b;
> +    __u32 c;
>      char pad[];
>  };

But in this case, the size of "struct foo" changed and the offset of the
data member 'pad' changed too.  So I'd flag this as a meaningful ABI
change to report in all cases.  For instance, if a struct embeds this
type, then the size of that struct changes etc.

What am I missing?

>
> But not this:
>
>  struct foo {
>      __u32 a;
> +    __u32 c;
>      __u32 b;
>      char pad[];
>  };
>

Yes, this change is meaningful to report in all cases too.

>>> === Anonymous Enums ===
>>> Another issue that comes up when comparing ABI across wide swaths of
>>> kernel commit history are changes to anonymous enums. From what I can
>>> tell, there's not a great way to handle this. If someone adds a new
>>> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6)
>>> can change, so abidiff reports a new anonymous enum with all the same
>>> fields even though it was basically just a "rename".
>>>
>>> For reference, this file is full of anonymous enums:
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15.
>>> Basically any change in there is triggering a failure from the
>>> script.
>> By 'any change', do you mean any addition of enumerator?
>> I think we can do something similar to what we do for anonymous
>> structs/unions, which it to consider content of the enums, rather than
>> just their "made-up" names.
>> I guess we need to have well defined examples of changes we want to
>> handle and I think we can come to a solution.
>> 
>>> I'm not sure how to handle these changes... there are possible ABI
>>> breakages when changing anonymous enums, but it's unclear to me how to
>>> detect them.
>> Let's come up with some small examples first, I'd say.
>
> Here's one example of the issue:
> https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d 
> (specifically the change to include/uapi/rdma/hns-abi.h).
>
> Here, a variant was added to two anonymous enums:
>
>  enum {
>      HNS_ROCE_EXSGE_FLAGS = 1 << 0,
>      HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1,
> +    HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2,
>  };
>
>  enum {
>       HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0,
>       HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1,
> +     HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2,
>  };
>
> abidiff outputs:
>
>     [C] 'enum __anonymous_enum__1' changed:
>       type size hasn't changed
>       1 enumerator deletion:
>         '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1'
>       3 enumerator insertions:
>         '__anonymous_enum__::HNS_ROCE_EXSGE_FLAGS' value '1'
>         '__anonymous_enum__::HNS_ROCE_RQ_INLINE_FLAGS' value '2'
>         '__anonymous_enum__::HNS_ROCE_CQE_INLINE_FLAGS' value '4'
>   1 added type unreachable from any public interface:
>     [A] 'enum __anonymous_enum__1' at hns-abi.h:94:1
>
> It seems like it mixed them up while processing? I'm not exactly sure
> what's going on internally.

Yes, this is a bug.  I need to look into it.

>
> Another example:
> https://github.com/w1ldptr/linux/commit/d219df60a70ed0739aa5dd34b477763311fc5a7b 
> (specifically the change to include/uapi/linux/bpf.h).
>
> Here, just one enum was modified:
>
>  enum {
>      BPF_F_ADJ_ROOM_FIXED_GSO	= (1ULL << 0),
>      BPF_F_ADJ_ROOM_ENCAP_L3_IPV4	= (1ULL << 1),
>      BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	= (1ULL << 2),
>      BPF_F_ADJ_ROOM_ENCAP_L4_GRE	= (1ULL << 3),
>      BPF_F_ADJ_ROOM_ENCAP_L4_UDP	= (1ULL << 4),
>      BPF_F_ADJ_ROOM_NO_CSUM_RESET	= (1ULL << 5),
>      BPF_F_ADJ_ROOM_ENCAP_L2_ETH	= (1ULL << 6),
> +    BPF_F_ADJ_ROOM_DECAP_L3_IPV4	= (1ULL << 7),
> +    BPF_F_ADJ_ROOM_DECAP_L3_IPV6	= (1ULL << 8),
>  };
>
>
> But abidiff outputs:
>
>     [C] 'enum __anonymous_enum__9' changed:
>       type size hasn't changed
>       3 enumerator deletions:
>         '__anonymous_enum__9::BPF_F_ZERO_CSUM_TX' value '2'
>         '__anonymous_enum__9::BPF_F_DONT_FRAGMENT' value '4'
>         '__anonymous_enum__9::BPF_F_SEQ_NUMBER' value '8'
>       9 enumerator insertions:
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_FIXED_GSO' value '1'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV4' value '2'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L3_IPV6' value '4'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_GRE' value '8'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L4_UDP' value '16'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_NO_CSUM_RESET' value '32'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_ENCAP_L2_ETH' value '64'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV4' value '128'
>         '__anonymous_enum__14::BPF_F_ADJ_ROOM_DECAP_L3_IPV6' value '256'
>   3 added types unreachable from any public interface:
>     [A] 'enum __anonymous_enum__9' at bpf.h:5781:1
>     [A] 'struct bpf_rb_node' at bpf.h:6931:1
>     [A] 'struct bpf_rb_root' at bpf.h:6926:1
>
> Any ideas on how to handle these issues?

These are bugs that ought to be fixed, I'll certainly look into them.

>
> Thank you!

Thank you for this excellent write up.

[...]


John Moon wrote:


> Hi Dodji,
>
> I wanted to follow up with you and see if/how we can assist with these
> features. I'm happy to jump in and try implementation if we agree on
> the feature requirements, though I'm not very familiar with the
> codebase.
>
> To summarize, here are the four usecases we have so far and plans to
> address them:
>
> 1. Enum expansion
>    - Add support for this kind of suppression:
>    [suppress_type]
>      type_kind = enum
>      allow_n_last_enumerator_changes = 3
>      changed_enumerators_regexp = .*MAX.*, .*LAST.*, .*NUM.*, .*NBITS.*

Right.

>
> 2. Padding field expansion
>    - Current feature set is close, but how can we handle multiple names?
>    - Do we want support for this kind suppression (multiple regex's)?
>    [suppress_type]
>       type_kind = struct
>     has_data_member_inserted_at =
>     offset_of_first_data_member_regexp(.*padding, .*reserved,
>    .*unused)

I think this one can be handled already by writing one type suppression
specification per pattern of data member after which insertion is
allowed.  Would this work for you at least for a start?


> 3. Flex array expansion
>    - Not sure how to detect/suppress this condition
>    - Flex arrays can only be at the ends of structs, so we just need
>      to make sure new struct members come "second to last"

I am still not sure why we think these can be suppressed; I am pretty
sure I am missing something but I am not sure what yet.


> 4. Anonymous enums
>    - Not sure how to associate these enums when diffing since their
>      "made up" names don't always match before/after a patch.
>

This is a plain bug that ought to be fixed, I think.

> I can try tackling items 1 and 2 if you'd like - just let me know! For
> items 3 and 4, can we work up a path forward?

First thank you for proposing your help, that would be greatly
appreciated :-) So, I have created a branch "kuapi-verify" at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/kuapi-verify.

Its purpose is to contain the code tu support this kernel uapi
verification effort, on the libabigail side.  I've folded the content of
the branch "last-enumerator-change" into it.  That branch is at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change.

So the idea is that we can fold the necessary features into
"kuapi-verify" as we progress on them, if you like.  If you fancy, you
can start looking into how to extend the content of the branch
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/filter-last-enumerator-change
to make it support the feature 1.  In the mean time, I'll look into fixing 4.  Until
then, I'll be more clear on 3, I guess.

I am available on IRC at irc.oftc.net#libabigail during western
European working hours if you need higher bandwidth exchanges.  I'd be
glad to chat with you there!

Thank you for following up!

Cheers,
  

Hello John,

John Moon <quic_johmoo@quicinc.com> a écrit:

[...]

> === Anonymous Enums ===
> Another issue that comes up when comparing ABI across wide swaths of
> kernel commit history are changes to anonymous enums. From what I can 
> tell, there's not a great way to handle this. If someone adds a new
> anonymous enum, the tag abidiff gives (e.g. enum __anonymous_enum__6) 
> can change, so abidiff reports a new anonymous enum with all the same
> fields even though it was basically just a "rename".
>
> For reference, this file is full of anonymous enums:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15. 
> Basically any change in there is triggering a failure from the script.

[...]

Dodji Seketeli <dodji@seketeli.org> a écrit:

[...]

> I think we can do something similar to what we do for anonymous
> structs/unions, which it to consider content of the enums, rather than
> just their "made-up" names.

So, I have an initial implementation for handling the comparison of
anonymous enums and it's in the 'users/dodji/better-anon-enums' branch.

Maybe you can try this branch on your test case to see if you still have
issues regarding anonymous enums?  If you still do, I'd be very much
interested in reproducing the issue locally on my system and extract a
smaller reproducer that could end up in the regression test suite of
libabigail.

I hope this can be useful somehow.

[...]

Cheers and many thanks!
  

Hey again,

[...]

Dodji Seketeli <dodji@seketeli.org> a écrit:

> So, I have an initial implementation for handling the comparison of
> anonymous enums and it's in the 'users/dodji/better-anon-enums' branch.

I forgot to add a link to the branch.  It's at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums.

So cloning it is via:

    $ git clone git://sourceware.org/git/libabigail.git -b users/dodji/better-anon-enums

[...]

Cheers,
  

On 9/22/2023 4:51 AM, Dodji Seketeli wrote:
> Hey again,
> 
> [...]
> 
> Dodji Seketeli <dodji@seketeli.org> a écrit:
> 
>> So, I have an initial implementation for handling the comparison of
>> anonymous enums and it's in the 'users/dodji/better-anon-enums' branch.
> 
> I forgot to add a link to the branch.  It's at
> https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums.
> 
> So cloning it is via:
> 
>      $ git clone git://sourceware.org/git/libabigail.git -b users/dodji/better-anon-enums
> 
> [...]
> 
> Cheers,
> 

Hi Dodji,

Thanks for the quick turnaround! I gave this a try on my end and it 
looks like the patch is doing what it's supposed to, but it's not 
producing the desired behavior.

Let's zoom in on this example: 
https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d 
(specifically, the change to include/uapi/rdma/hns-abi.h:

  enum {
  	HNS_ROCE_EXSGE_FLAGS = 1 << 0,
  	HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1,
+	HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2,
  };

  enum {
  	HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0,
  	HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1,
+	HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2,
  };


Before your patch, abidiff reports:

     [C] 'enum __anonymous_enum__1' changed:
       type size hasn't changed
       2 enumerator deletions:
         '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1'
         '__anonymous_enum__1::HNS_ROCE_RSP_RQ_INLINE_FLAGS' value '2'
       3 enumerator insertions:
         '__anonymous_enum__::HNS_ROCE_EXSGE_FLAGS' value '1'
         '__anonymous_enum__::HNS_ROCE_RQ_INLINE_FLAGS' value '2'
         '__anonymous_enum__::HNS_ROCE_CQE_INLINE_FLAGS' value '4'
   1 added type unreachable from any public interface:
     [A] 'enum __anonymous_enum__1' at hns-abi.h:94:1

After your patch:

   2 removed types unreachable from any public interface:
     [D] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' 
at hns-abi.h:88:1
     [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, 
HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:93:1
   2 added types unreachable from any public interface:
     [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=4, HNS_ROCE_EXSGE_FLAGS=1, 
HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1
     [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=4, 
HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at 
hns-abi.h:94:1


I probably should have thought of this before, but simply changing the 
anon enum's "name" to be a concatenation of members wouldn't work to 
associate them across a diff. When you add a new enum variant, the 
"name" of the enum changes too, so abidiff thinks they're different enums.

I've given it a bit of thought and I'm not sure how the two enums could 
be associated when diffing... I'm not sure if the diff corpus has any 
context-awareness that could help here. Maybe you can think of something 
clever?

If not, I think this output is at least easier to post-process! For 
example, I could add logic to the script to do something like "if 
there's a deleted enum whose contents are present in an added enum, 
assume it's an addition to an anonymous enum and ignore".

Let me know what you think!

Thanks,
John
  

On 9/22/2023 11:28 AM, John Moon via Libabigail wrote:
> If not, I think this output is at least easier to post-process! For 
> example, I could add logic to the script to do something like "if 
> there's a deleted enum whose contents are present in an added enum, 
> assume it's an addition to an anonymous enum and ignore".
> 


Proof of concept to filter out anon enum additions from input like this:

   2 removed types unreachable from any public interface:
     [D] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' 
at hns-abi.h:88:1
     [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, 
HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:93:1
   2 added types unreachable from any public interface:
     [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=4, HNS_ROCE_EXSGE_FLAGS=1, 
HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1
     [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=4, 
HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at 
hns-abi.h:94:1

---
#!/bin/bash
set -o errexit

# Take a line and form a normalized string representation
get_repr() {
   local -r line="$1"
   echo "$line" | grep -o '{.*}' \
                | tr -d '{} ' \
                | sed -e 's/,$//g' -e 's/,/\n/g' \
                | sort -t= -k2 -n \
                | xargs
}

input="${1:-/dev/stdin}"
deleted_anon_enum_reprs=()
added_anon_enum_reprs=()
while read -r line; do
   if echo "$line" | grep -q "\[D\] 'enum {"; then
     deleted_anon_enum_reprs+=("$(get_repr "$line")")
   fi
   if echo "$line" | grep -q "\[A\] 'enum {"; then
     added_anon_enum_reprs+=("$(get_repr "$line")")
   fi
done < "$input"

# If a deleted enum's representation appears in an added enum's
# representation, assume they're the same enum (just being added
# on to).
legit_breaks=${#deleted_anon_enum_reprs[@]}
for del_repr in "${deleted_anon_enum_reprs[@]}"; do
   for add_repr in "${added_anon_enum_reprs[@]}"; do
     if echo "$add_repr" | grep -q "^${del_repr}"; then
       legit_breaks=$((legit_breaks - 1))
       break
     fi
   done
done

echo "$legit_breaks breaks"
---

There may be a better way to do this sort of thing in a suppression, but 
I'm not sure how given they operate on diffs (which wouldn't be created 
in this case).

It feels hacky as the abidiff stdout isn't a stable API itself, but if 
it sounds reasonable to you, I can bake the above filtering logic into 
the next rev submitted upstream. Just let me know!

Thanks,
John
  

Hello John!

John Moon <quic_johmoo@quicinc.com> a écrit:

[...]

> Thanks for the quick turnaround! I gave this a try on my end and it
> looks like the patch is doing what it's supposed to, but it's not 
> producing the desired behavior.

Thanks John for quickly testing this and providing feedback!  It's
really appreciated.

> Let's zoom in on this example:
> https://github.com/torvalds/linux/commit/1d91855304c2046115ee10be2c93161d93d5d40d 
> (specifically, the change to include/uapi/rdma/hns-abi.h:
>
>  enum {
>  	HNS_ROCE_EXSGE_FLAGS = 1 << 0,
>  	HNS_ROCE_RQ_INLINE_FLAGS = 1 << 1,
> +	HNS_ROCE_CQE_INLINE_FLAGS = 1 << 2,
>  };
>
>  enum {
>  	HNS_ROCE_RSP_EXSGE_FLAGS = 1 << 0,
>  	HNS_ROCE_RSP_RQ_INLINE_FLAGS = 1 << 1,
> +	HNS_ROCE_RSP_CQE_INLINE_FLAGS = 1 << 2,
>  };
>
>
> Before your patch, abidiff reports:
>
>     [C] 'enum __anonymous_enum__1' changed:
>       type size hasn't changed
>       2 enumerator deletions:
>         '__anonymous_enum__1::HNS_ROCE_RSP_EXSGE_FLAGS' value '1'
>         '__anonymous_enum__1::HNS_ROCE_RSP_RQ_INLINE_FLAGS' value '2'
>       3 enumerator insertions:
>         '__anonymous_enum__::HNS_ROCE_EXSGE_FLAGS' value '1'
>         '__anonymous_enum__::HNS_ROCE_RQ_INLINE_FLAGS' value '2'
>         '__anonymous_enum__::HNS_ROCE_CQE_INLINE_FLAGS' value '4'
>   1 added type unreachable from any public interface:
>     [A] 'enum __anonymous_enum__1' at hns-abi.h:94:1
>
> After your patch:
>
>   2 removed types unreachable from any public interface:
>     [D] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }'
>     at hns-abi.h:88:1
>     [D] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1,
>     HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at hns-abi.h:93:1
>   2 added types unreachable from any public interface:
>     [A] 'enum {HNS_ROCE_CQE_INLINE_FLAGS=4, HNS_ROCE_EXSGE_FLAGS=1,
>     HNS_ROCE_RQ_INLINE_FLAGS=2, }' at hns-abi.h:88:1
>     [A] 'enum {HNS_ROCE_RSP_CQE_INLINE_FLAGS=4,
>     HNS_ROCE_RSP_EXSGE_FLAGS=1, HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' at 
> hns-abi.h:94:1

Right.

>
> I probably should have thought of this before, but simply changing the
> anon enum's "name" to be a concatenation of members wouldn't work to 
> associate them across a diff. When you add a new enum variant, the
> "name" of the enum changes too, so abidiff thinks they're different
> enums.

Actually, no, it's me who hasn't been clear.  This patch was "just" the
beginning.  The goal is to avoid the "renumbering" that happens on the
internal anonymous names (e.g, __anonymous_enum__1 turning into
__anonymous_enum__2) whenever a new enum is added in the middle of a
translation unit.

> I've given it a bit of thought and I'm not sure how the two enums
> could be associated when diffing... I'm not sure if the diff corpus
> has any context-awareness that could help here. Maybe you can think of
> something clever?

Now that the renumbering issue is solved by using the flat
representation to designate the anonymous enum, we need to teach the
comparison engine that an enum that got removed and then added again is
actually an enum that 'changed'.  We do similar things already for other
kinds of types in the comparison engine.  Basically, if a removed
anonymous enum and an added anonymous enum have enumerators in common,
we assume that we are looking at a changed enumerator.

So, I have updated the branch with some more patches and I think we
should now be close to what we want, regarding this issue.

If you update that branch and look at the patch that is at its tip, you
can see that in the test suite, in tests/data/test-abidiff-exit/, I've
added two files test-anonymous-enums-change-v0.c and
test-anonymous-enums-change-v1.c that are compiled and compared.  Let's
look at the diff of these two files:

        $ diff -u test-anonymous-enums-change-v0.c test-anonymous-enums-change-v1.c 
        --- test-anonymous-enums-change-v0.c	2023-09-26 10:13:54.880093568 +0200
        +++ test-anonymous-enums-change-v1.c	2023-09-26 10:14:46.073213038 +0200
        @@ -1,6 +1,6 @@
        -/*
        - *  Compile this with:
        - *    gcc -c -g -fno-eliminate-unused-debug-types test-anonymous-enums-change-v0.c 
        +/* 
        + * Compile this with:
        + *    gcc -c -g -fno-eliminate-unused-debug-types test-anonymous-enums-change-v1.c 
          */

         enum
        @@ -13,14 +13,19 @@
           {
             E3_0,
             E3_1,
        +    E3_2
           };

         enum
           {
             E4_0,
             E4_1,
        -    E4_2,
        -    E4_LAST_ELEMENT
        +  };
        +
        +enum
        +  {
        +    E5_0,
        +    E5_1,
           };

         enum

        $

So, an anonymous enum type has one enumerator added, another one has
enumerators removed, and there is an addition of an anonymous enum type
to the translation unit.

Now let's look at what abidiff reports:

        $ ~/git/libabigail/better-anon-enums/build/tools/abidiff --non-reachable-types test-anonymous-enums-change-v0.o test-anonymous-enums-change-v1.o || echo "$?"
        Functions changes summary: 0 Removed, 0 Changed, 0 Added function
        Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
        Unreachable types summary: 0 removed, 1 changed (1 filtered out), 1 added types

        1 changed type unreachable from any public interface:

          [C] 'enum {E4_0=0, E4_1=1, E4_2=2, E4_LAST_ELEMENT=3, }' changed:
            type size hasn't changed
            2 enumerator deletions:
              'E4_2' value '2'
              'E4_LAST_ELEMENT' value '3'

        1 added type unreachable from any public interface:

          [A] 'enum {E5_0=0, E5_1=1, }' at test-anonymous-enums-change-v1.c:26:1

        12

        $

Hopefully, that is closer to what we'd expect.

We see in the summary of the change report that one change was filtered
out.  Let's use the '--harmless' option to see what the filtered out
change was:

        $ ~/git/libabigail/better-anon-enums/build/tools/abidiff --non-reachable-types --harmless test-anonymous-enums-change-v0.o test-anonymous-enums-change-v1.o
        Functions changes summary: 0 Removed, 0 Changed, 0 Added function
        Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
        Unreachable types summary: 0 removed, 2 changed, 1 added types

        2 changed types unreachable from any public interface:

          [C] 'enum {E4_0=0, E4_1=1, E4_2=2, E4_LAST_ELEMENT=3, }' changed:
            type size hasn't changed
            2 enumerator deletions:
              'E4_2' value '2'
              'E4_LAST_ELEMENT' value '3'

          [C] 'enum {E3_0=0, E3_1=1, }' changed:
            type size hasn't changed
            1 enumerator insertion:
              'E3_2' value '2'

        1 added type unreachable from any public interface:

          [A] 'enum {E5_0=0, E5_1=1, }' at test-anonymous-enums-change-v1.c:26:1


So, it's the change involving an enumerator insertion that was initially
filtered out as a harmless change.

[...]


> If not, I think this output is at least easier to post-process!

In general, I am not for post-processing the output of abidiff.  I think
it's much more maintainable to make it emit what we want directly.

>For example, I could add logic to the script to do something like "if
>there's a deleted enum whose contents are present in an added enum,
>assume it's an addition to an anonymous enum and ignore".

Let's try harder to get things right, first ;-)

Many thanks.

Cheers,
  

On 9/26/2023 1:38 AM, Dodji Seketeli wrote:
>> I probably should have thought of this before, but simply changing the
>> anon enum's "name" to be a concatenation of members wouldn't work to
>> associate them across a diff. When you add a new enum variant, the
>> "name" of the enum changes too, so abidiff thinks they're different
>> enums.
> 
> Actually, no, it's me who hasn't been clear.  This patch was "just" the
> beginning.  The goal is to avoid the "renumbering" that happens on the
> internal anonymous names (e.g, __anonymous_enum__1 turning into
> __anonymous_enum__2) whenever a new enum is added in the middle of a
> translation unit.
> 

Ahh, understood.

>> I've given it a bit of thought and I'm not sure how the two enums
>> could be associated when diffing... I'm not sure if the diff corpus
>> has any context-awareness that could help here. Maybe you can think of
>> something clever?
> 
> Now that the renumbering issue is solved by using the flat
> representation to designate the anonymous enum, we need to teach the
> comparison engine that an enum that got removed and then added again is
> actually an enum that 'changed'.  We do similar things already for other
> kinds of types in the comparison engine.  Basically, if a removed
> anonymous enum and an added anonymous enum have enumerators in common,
> we assume that we are looking at a changed enumerator.
> 
> So, I have updated the branch with some more patches and I think we
> should now be close to what we want, regarding this issue.
> 

Excellent! I'm glad this could be handled in the comparison engine. It 
seems like it's working perfectly! With the latest abidiff from your 
branch, my script reports 1d91855304c2 in the kernel as compatible!

If I add in the --harmless option just to test, it does report:

     [C] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1, 
HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' changed:
       type size hasn't changed
       1 enumerator insertion:
         'HNS_ROCE_RSP_CQE_INLINE_FLAGS' value '4'
     [C] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }' 
changed:
       type size hasn't changed
       1 enumerator insertion:
         'HNS_ROCE_CQE_INLINE_FLAGS' value '4'

This is the exact behavior we needed.

For some perspective, from kernel version v6.1 -> v6.2, there were 19 
such changes to anon enums. Most of them also include a _MAX variant at 
the end, but at least once I implement the suppression, it should work 
on these too:

     [C] 'enum {MDBE_ATTR_SOURCE=1, MDBE_ATTR_UNSPEC=0, 
__MDBE_ATTR_MAX=2, }' changed:
       type size hasn't changed
       3 enumerator insertions:
         'MDBE_ATTR_SRC_LIST' value '2'
         'MDBE_ATTR_GROUP_MODE' value '3'
         'MDBE_ATTR_RTPROT' value '4'
       1 enumerator change:
         '__MDBE_ATTR_MAX' from value '2' to '5' at if_bridge.h:723:1

> 
> 
>> If not, I think this output is at least easier to post-process!
> 
> In general, I am not for post-processing the output of abidiff.  I think
> it's much more maintainable to make it emit what we want directly.
> 
>> For example, I could add logic to the script to do something like "if
>> there's a deleted enum whose contents are present in an added enum,
>> assume it's an addition to an anonymous enum and ignore".
> 
> Let's try harder to get things right, first ;-)
> 

Agreed, thanks for steering us back on course!

- John
  

Hello John,


> On 9/26/2023 1:38 AM, Dodji Seketeli wrote:

[...]


>> Now that the renumbering issue is solved by using the flat
>> representation to designate the anonymous enum, we need to teach the
>> comparison engine that an enum that got removed and then added again is
>> actually an enum that 'changed'.  We do similar things already for other
>> kinds of types in the comparison engine.  Basically, if a removed
>> anonymous enum and an added anonymous enum have enumerators in common,
>> we assume that we are looking at a changed enumerator.
>> So, I have updated the branch with some more patches and I think we
>> should now be close to what we want, regarding this issue.
>> 

John Moon <quic_johmoo@quicinc.com> a écrit:

> Excellent! I'm glad this could be handled in the comparison engine. It
> seems like it's working perfectly! With the latest abidiff from your 
> branch, my script reports 1d91855304c2 in the kernel as compatible!

Whoah!

>
> If I add in the --harmless option just to test, it does report:
>
>     [C] 'enum {HNS_ROCE_RSP_EXSGE_FLAGS=1,
>     HNS_ROCE_RSP_RQ_INLINE_FLAGS=2, }' changed:
>       type size hasn't changed
>       1 enumerator insertion:
>         'HNS_ROCE_RSP_CQE_INLINE_FLAGS' value '4'
>     [C] 'enum {HNS_ROCE_EXSGE_FLAGS=1, HNS_ROCE_RQ_INLINE_FLAGS=2, }'
>     changed:
>       type size hasn't changed
>       1 enumerator insertion:
>         'HNS_ROCE_CQE_INLINE_FLAGS' value '4'
>
> This is the exact behavior we needed.

Good.

I have however, noticed a little (subtle) issue with the returned value
of abidiff when used with the --harmless option.  It only sets the
ABIDIFF_ABI_CHANGE bit instead of /also/ setting the
ABIDIFF_ABI_INCOMPATIBLE_CHANGE bit, as it does when you don't provide
the --harmless.  I have thus added this patch to the set in the
'better-anon-enums' branch:
https://sourceware.org/git/?p=libabigail.git;a=commit;h=58e93b384aa1fe9a3508d108e7b1af7c7489e680.

I think (but I can be wrong) that this should not radically change the
result of check-uapi.sh.

> For some perspective, from kernel version v6.1 -> v6.2, there were 19
> such changes to anon enums. Most of them also include a _MAX variant
> at the end, but at least once I implement the suppression, it should
> work on these too:
>
>     [C] 'enum {MDBE_ATTR_SOURCE=1, MDBE_ATTR_UNSPEC=0,
>     __MDBE_ATTR_MAX=2, }' changed:
>       type size hasn't changed
>       3 enumerator insertions:
>         'MDBE_ATTR_SRC_LIST' value '2'
>         'MDBE_ATTR_GROUP_MODE' value '3'
>         'MDBE_ATTR_RTPROT' value '4'
>       1 enumerator change:
>         '__MDBE_ATTR_MAX' from value '2' to '5' at if_bridge.h:723:1
>

Thanks for this perspective.

To collect all the patches that we are slowly accumulating to support
check-uapi.sh, I have created the branch check-uapi-support at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/check-uapi-support.
It doesn't yet have any patches other than what we have in master.

But if you find that a patch is "useful" to achieve the goal of
supporting check-uapi.sh, we can merge it in that new branch if you
agree.  Once we are happy with the status of that branch, we can all
review it and consider merging it into the mainline. What do you think
about that proposal?

[...]

> thanks for steering us back on course!

My pleasure.  But really, thank *you* for the quick feedback.

[...]

Cheers,
  

Hello,

John Moon <quic_johmoo@quicinc.com> a écrit:

[...]

> To summarize, here are the four usecases we have so far and plans to
> address them:

[...]

> 3. Flex array expansion
>    - Not sure how to detect/suppress this condition
>    - Flex arrays can only be at the ends of structs, so we just need
>      to make sure new struct members come "second to last"

So, I have a patch to support this case.

It's in the branch https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/fam-suppr.

The commit itself is https://sourceware.org/git/?p=libabigail.git;a=commit;h=e609f46734b07b551c1b3dbc1eeb2cea61c4ba34

Here is what the commit log says:

    Consider this code example:

        $ cat test-v0.c
         1	struct foo
         2	{
         3	  int member0;
         4	  char pad[]; /* <-- flexible array member.  */
         5	};
         6
         7	void
         8	foo(struct foo * p __attribute__((unused)))
         9	{
        10	}
        $

    Consider this new version of the code where a data member has been
    added right before the flexible array member:

        $ cat -n test-v1.c
         1	struct foo
         2	{
         3	  int member0;
         4	  char member1; /*<-- added member.  */
         5	  char pad[]; /* <-- flexible array member.  */
         6	};
         7
         8	void
         9	foo(struct foo * p __attribute__((unused)))
        10	{
        11	}
        $

    Here is what abidiff reports about the change:

        $ abidiff test-v0.o test-v1.o || echo "returned value: $?"
        Functions changes summary: 0 Removed, 1 Changed, 0 Added function
        Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

        1 function with some indirect sub-type change:

          [C] 'function void foo(foo*)' at test-v0.c:8:1 has some indirect sub-type changes:
            parameter 1 of type 'foo*' has sub-type changes:
              in pointed to type 'struct foo' at test-v1.c:1:1:
                type size changed from 32 to 64 (in bits)
                1 data member insertion:
                  'char member1', at offset 32 (in bits) at test-v1.c:4:1
                1 data member change:
                  'char pad[]' offset changed from 32 to 40 (in bits) (by +8 bits)

        returned value: 4
        $

    This patch allows users to suppress this change report using a new
    property value to the "has_data_member_inserted_at" property of the
    [suppress_type] directive.  The resulting suppression specification
    reads:

        $ cat -n foo.suppr
             1	[suppress_type]
             2	 type_kind = struct
             3       name = foo
             4	 has_data_member_inserted_at = offset_of_flexible_array_data_member
             5	 has_size_change = yes
        $

    With this suppression specification the previous command now gives:

        $ abidiff --suppr foo.suppr test-v0.o test-v1.o && echo "returned value: $?"
        Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
        Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

        returned value: 0
        $

I hope this is close enough to what we expect.

If after testing and reviewing this patch it's good enough for you,
please let me know so that I can fold it into the WIP branch of the
"UAPI checker support" at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/check-uapi-support.


Please note that I have filed a bug to track the progress of this
overall "check-uapi-support" effort at
https://sourceware.org/bugzilla/show_bug.cgi?id=30931.  Feel free to
follow that bug there.  You can reply to the bugzilla's emails using
your mail user agent.

> 4. Anonymous enums
>    - Not sure how to associate these enums when diffing since their
>      "made up" names don't always match before/after a patch.

As I said in an other email, this task led to the patch set
https://inbox.sourceware.org/libabigail/87r0mapspt.fsf@redhat.com/ which
is in the branch
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums.
I am waiting for your feedback to fold that patch set into the WIP
branch "check-uapi-support".

> I can try tackling items 1 and 2 if you'd like - just let me know! For
> items 3 and 4, can we work up a path forward?

[...]

Hopefully things are moving somewhat now :-)

Cheers,