| Message ID | 20240410-pack-v1-2-70f287dd8a66@chromium.org |
|---|---|
| State | Accepted |
| Commit | 2abcd952e1999d0f1006068c455a524bec37883d |
| Headers | show |
| Series | media: Fix missing warnings for llvm am32 | expand |
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
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 >
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 >> > >
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));
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(-)