Re: abidiff improvements for kernel UAPI checker

[John Moon <quic_johmoo@quicinc.com>]

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