Re: abidiff improvements for kernel UAPI checker

[Dodji Seketeli <dodji@seketeli.org>]

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,

-- 
		Dodji