media: Fix missing warnings for llvm am32

[PATCH 2/3] media: dvb: Fix dtvs_stats packing.

Posted by Ricardo Ribalda 1 year, 10 months ago

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@chromium.org>
---
 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));
 
 

-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

Posted by Hans Verkuil 1 year, 10 months ago

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@chromium.org>
> ---
>  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.

I'm not going to accept this unless it is clear that there are no ABI changes.

Note that the ABI test in the build scripts only tests V4L2 at the moment,
not the DVB API.

Regards,

	Hans

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

Posted by Ricardo Ribalda 1 year, 10 months ago

Hi Hans

On Fri, 12 Apr 2024 at 16:21, Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@chromium.org>
> > ---
> >  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?

Regards!

>
> Note that the ABI test in the build scripts only tests V4L2 at the moment,
> not the DVB API.
>
> Regards,
>
>         Hans
>


-- 
Ricardo Ribalda

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

Posted by Hans Verkuil 1 year, 9 months ago

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@xs4all.nl> 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@chromium.org>
>>> ---
>>>  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
>>
> 
>

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

Posted by Ricardo Ribalda 1 year, 9 months ago

Hi Hans

On Mon, 15 Apr 2024 at 11:51, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> 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@xs4all.nl> 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@chromium.org>
> >>> ---
> >>>  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.

That sounds great. Thanks!

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

Ferpect!, I will update it in media-ci

Thanks!
>
> 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
> >>
> >
> >
>


-- 
Ricardo Ribalda