Re: abidiff improvements for kernel UAPI checker

[John Moon <quic_johmoo@quicinc.com>]

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