Re: abidiff improvements for kernel UAPI checker

[Dodji Seketeli <dodji@seketeli.org>]

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,


-- 
		Dodji