Re: abidiff improvements for kernel UAPI checker

[John Moon <quic_johmoo@quicinc.com>]

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