Re: abidiff handling of migrations to flex arrays

[Dodji Seketeli <dodji@seketeli.org>]

Hello John,

John Moon <quic_johmoo@quicinc.com> a écrit:

[...]

> I was reviewing notes that Greg KH gave me in the last revision of the
> script.
>
> https://lore.kernel.org/lkml/2023041136-donator-faceplate-5f91@gregkh/
>
> Notably, this section:
>
>>>  Addition/use of flex arrays:
>>>    - include/uapi/linux/rseq.h (f7b01bb0b57f)
>>>    - include/uapi/scsi/scsi_bsg_mpi3mr.h (c6f2e6b6eaaf)
>> That is not a breakage, that's a tool problem.
>> 
>>>  Type change:
>>>    - include/uapi/scsi/scsi_bsg_ufs.h (3f5145a615238)
>> Again, not a real breakage, size is still the same.
>> 
>>>  Additions into existing struct:
>>>    - include/uapi/drm/amdgpu_drm.h (b299221faf9b)
>>>    - include/uapi/linux/perf_event.h (09519ec3b19e)
>>>    - include/uapi/linux/virtio_blk.h (95bfec41bd3d)
>> Adding data to the end of a structure is a well-known way to extend
>> the
>> api, in SOME instances if it is used properly.
>> So again, not a break.
>
> If we look at these examples, we can now effectively suppress all of
> them except for include/uapi/scsi/scsi_bsg_mpi3mr.h (c6f2e6b6eaaf).
>
> https://github.com/torvalds/linux/commit/c6f2e6b6eaaf883df482cb94f302acad9b80a2a4
>
> Basically, the change takes a struct which ends in a "fake flex array"
> and turns it into a real flex array:
>
> struct foo {
>         int x;
>         int y;
>         int end[1];
> };
>
> ...
>
> struct foo {
>         int x;
>         int y;
>         int end[];
> };
>
> abidiff flags this change with:
>
>   [C] 'struct foo' changed:
>     type size changed from 96 to 64 (in bits)
>     1 data member change:
>       type of 'int end[1]' changed:
>         type name changed from 'int[1]' to 'int[]'
>         array type size changed from 32 to 'unknown'
>         array type subrange 1 changed length from 1 to 'unknown'
>
> These sorts of changes are all over the kernel since they recently
> decided to move to using flex arrays and are continuing to make
> updates similar to this one.
>
> I tried looking through the docs and couldn't find a way to suppress
> this kind of change. Do you know of a way to do it using the existing 
> suppressions?

Unfortunately, no :-(

I think we have to come up with a new suppression specification syntax
for this.

> I think semantically we're trying to suppress "changes
> which take an array of size 1 and turn it into a flex array".

Yes, but I think it's a little broader than that.

The initial array size can also be zero, as that is how GCC allowed
expressing flexible array members prior to the standardization of that
feature in C99: https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html.
Clang does that too.

And of course both compilers also allow the array size of 1.

There is a nice article that summarizes these forms of flexible array
data member at
https://developers.redhat.com/articles/2022/09/29/benefits-limitations-flexible-array-members#standard_flexible_array_member_behavior.

So if we are going to support suppressing ABI changes that move from
pre-standardization form to the standardized form of FAMs, I think we
need to support both pre-standardization forms.

How about a suppression specification syntax that looks like this?

    [suppress_type]
      type_kind = struct
      has_size_change = true
      has_strict_flexible_array_data_member_conversion

Maybe we should file a bug and discuss the details about it there?

Cheers,

-- 
		Dodji