* abidiff handling of migrations to flex arrays
@ 2023-10-25 23:31 John Moon
2023-10-27 10:14 ` Dodji Seketeli
0 siblings, 1 reply; 3+ messages in thread
From: John Moon @ 2023-10-25 23:31 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Satya Durga Srinivasu Prabhala, Trilok Soni, libabigail
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: abidiff handling of migrations to flex arrays
2023-10-25 23:31 abidiff handling of migrations to flex arrays John Moon
@ 2023-10-27 10:14 ` Dodji Seketeli
2023-10-27 17:51 ` John Moon
0 siblings, 1 reply; 3+ messages in thread
From: Dodji Seketeli @ 2023-10-27 10:14 UTC (permalink / raw)
To: John Moon; +Cc: Satya Durga Srinivasu Prabhala, Trilok Soni, libabigail
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: abidiff handling of migrations to flex arrays
2023-10-27 10:14 ` Dodji Seketeli
@ 2023-10-27 17:51 ` John Moon
0 siblings, 0 replies; 3+ messages in thread
From: John Moon @ 2023-10-27 17:51 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Satya Durga Srinivasu Prabhala, Trilok Soni, libabigail
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-27 17:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-25 23:31 abidiff handling of migrations to flex arrays John Moon
2023-10-27 10:14 ` Dodji Seketeli
2023-10-27 17:51 ` John Moon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).