abidiff handling of migrations to flex arrays

abidiff handling of migrations to flex arrays

[John Moon]

Hi Dodji,

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? I think semantically we're trying to suppress "changes 
which take an array of size 1 and turn it into a flex array".

Thanks,
John

Re: abidiff handling of migrations to flex arrays

[Dodji Seketeli]

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

Re: abidiff handling of migrations to flex arrays

[John Moon]

On 10/27/2023 3:14 AM, Dodji Seketeli wrote:
> 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.


I agree. Though, I find that abidiff currently seems not to mind a 
transition from 0-length to flex:

  struct foo {
         __u32 x;
-       __u32 flex[0];
+       __u32 flex[];
  };

Even with --harmless, abidiff doesn't seem to care.

It seems the internal representation (at least with my versions of clang 
and gcc) consider flex[0] and flex[] the same. Like if I do this:

  struct foo {
         __u32 x;
-       __u32 flex[0];
+       __u32 flex[1];
  };

abidiff gives me:

   [C] 'struct foo' changed:
     type size changed from 32 to 64 (in bits)
     1 data member change:
       type of '__u32 flex[]' changed:
         type name changed from '__u32[]' to '__u32[1]'
         array type size changed from 'unknown' to 32
         array type subrange 1 changed length from 'unknown' to 1

So, maybe we only need to worry about the 1-length array conversion?

> 
> 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?

Yeah, I think that suppression spec looks reasonable. I'll file a 
feature request for it.

Thanks,
John