Hi Ricardo,
On 12/04/2024 17:00, Ricardo Ribalda wrote:
> Hi Hans
>
> On Fri, 12 Apr 2024 at 16:21, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
>> On 10/04/2024 14:24, Ricardo Ribalda wrote:
>>> The structure is packed, which requires that all its fields need to be
>>> also packed.
>>>
>>> ./include/uapi/linux/dvb/frontend.h:854:2: warning: field within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]
>>>
>>> Explicitly set the inner union as packed.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>>> ---
>>> include/uapi/linux/dvb/frontend.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
>>> index 7e0983b987c2d..8d38c6befda8d 100644
>>> --- a/include/uapi/linux/dvb/frontend.h
>>> +++ b/include/uapi/linux/dvb/frontend.h
>>> @@ -854,7 +854,7 @@ struct dtv_stats {
>>> union {
>>> __u64 uvalue; /* for counters and relative scales */
>>> __s64 svalue; /* for 0.001 dB measures */
>>> - };
>>> + } __attribute__ ((packed));
>>> } __attribute__ ((packed));
>>
>> This is used in the public API, and I think this change can cause ABI changes.
>>
>> Can you compare the layouts? Also between gcc and llvm since gcc never warned
>> about this.
>
> The pahole output looks the same in both cases:
>
> https://godbolt.org/z/oK4desv7Y
> vs
> https://godbolt.org/z/E36MjPr7v
>
> And it is also the same for all the compiler versions that I tried.
>
>
> struct dtv_stats {
> uint8_t scale; /* 0 1 */
> union {
> uint64_t uvalue; /* 1 8 */
> int64_t svalue; /* 1 8 */
> }; /* 1 8 */
>
> /* size: 9, cachelines: 1, members: 2 */
> /* last cacheline: 9 bytes */
> } __attribute__((__packed__));
>
>
>
> struct dtv_stats {
> uint8_t scale; /* 0 1 */
> union {
> uint64_t uvalue; /* 1 8 */
> int64_t svalue; /* 1 8 */
> }; /* 1 8 */
>
> /* size: 9, cachelines: 1, members: 2 */
> /* last cacheline: 9 bytes */
> } __attribute__((__packed__));
>
>
>>
>> I'm not going to accept this unless it is clear that there are no ABI changes.
>
> Is there something else that I can try?
No, that's what I needed. I also found some clang discussions here:
https://github.com/llvm/llvm-project/issues/55520
I propose that I add the following sentence to these three packing patches:
"Marking the inner union as 'packed' does not change the layout, since the
whole struct is already packed, it just silences the clang warning. See
also this llvm discussion: https://github.com/llvm/llvm-project/issues/55520";
If you are OK with that, then I can add that to your patches.
Related to this: I added CEC and DVB support to the ABI checks in the build
scripts. And fixed a bunch of mistakes there (e.g. 'false=true' where I meant
to write 'fail=true'!) that made the ABI checks useless.
I updated the abi/* files accordingly as well.
Regards,
Hans
>
> Regards!
>
>>
>> Note that the ABI test in the build scripts only tests V4L2 at the moment,
>> not the DVB API.
>>
>> Regards,
>>
>> Hans
>>
>
>
