| Message ID | 20220422192031.24895-1-devendra.tewari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | videodev2.h: apply packed attribute to union | expand |
Hi Devendra, Quoting Devendra Tewari (2022-04-22 20:20:31) > Fixes clang warning: field within 'v4l2_ext_control' is less than Can you detail which version of clang this occurs with? Have you tried more than one version? > 'v4l2_ext_control::(anonymous union > > Signed-off-by: Devendra Tewari <devendra.tewari@gmail.com> > --- > include/uapi/linux/videodev2.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 3768a0a80830..767c52c722cd 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1765,7 +1765,7 @@ struct v4l2_ext_control { > struct v4l2_ctrl_vp9_compressed_hdr __user *p_vp9_compressed_hdr_probs; > struct v4l2_ctrl_vp9_frame __user *p_vp9_frame; > void __user *ptr; > - }; > + } __attribute__ ((packed)); This is a curious fix. It's applying a packed attribute to the union, which I presume means that it's then applying the packed attribute to any item in the union. The items are all either: __s32, __s64, values - or pointers. While applying this attribute here may fix the compiler warning, I'm not sure it's clear why this is required. This file also has other locations where a union inside a packed struct is not marked as packed. Should all unions be marked with the attribute? Is there any more context from the compiler warning beyond what is reported above? -- Kieran > } __attribute__ ((packed)); > > struct v4l2_ext_controls { > -- > 2.25.1 >
> No dia 7 de out. de 2022, às 18:58, Kieran Bingham <kieran.bingham@ideasonboard.com> escreveu: > > Hi Devendra, > > Quoting Devendra Tewari (2022-04-22 20:20:31) >> Fixes clang warning: field within 'v4l2_ext_control' is less than > > Can you detail which version of clang this occurs with? Have you tried > more than one version? > This started happening with version 14.0.1 and I continue to see it with version 15. > >> 'v4l2_ext_control::(anonymous union >> >> Signed-off-by: Devendra Tewari <devendra.tewari@gmail.com> >> --- >> include/uapi/linux/videodev2.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 3768a0a80830..767c52c722cd 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -1765,7 +1765,7 @@ struct v4l2_ext_control { >> struct v4l2_ctrl_vp9_compressed_hdr __user *p_vp9_compressed_hdr_probs; >> struct v4l2_ctrl_vp9_frame __user *p_vp9_frame; >> void __user *ptr; >> - }; >> + } __attribute__ ((packed)); > > This is a curious fix. It's applying a packed attribute to the union, > which I presume means that it's then applying the packed attribute to > any item in the union. > > The items are all either: __s32, __s64, values - or pointers. > > While applying this attribute here may fix the compiler warning, I'm not > sure it's clear why this is required. This file also has other > locations where a union inside a packed struct is not marked as packed. > Should all unions be marked with the attribute? Interesting - I need to look deeper into packed. > Is there any more context from the compiler warning beyond what is > reported above? I'll post a more detailed log asap. > > -- > Kieran > > >> } __attribute__ ((packed)); >> >> struct v4l2_ext_controls { >> -- >> 2.25.1 >> Thanks, Devendra
> On 7 Oct 2022, at 20:04, Devendra Tewari <devendra.tewari@gmail.com> wrote: > > > >> No dia 7 de out. de 2022, às 18:58, Kieran Bingham <kieran.bingham@ideasonboard.com> escreveu: >> >> Hi Devendra, >> >> Quoting Devendra Tewari (2022-04-22 20:20:31) >>> Fixes clang warning: field within 'v4l2_ext_control' is less than >> >> Can you detail which version of clang this occurs with? Have you tried >> more than one version? >> > > This started happening with version 14.0.1 and I continue to see it with version 15. > >> >>> 'v4l2_ext_control::(anonymous union >>> >>> Signed-off-by: Devendra Tewari <devendra.tewari@gmail.com> >>> --- >>> include/uapi/linux/videodev2.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>> index 3768a0a80830..767c52c722cd 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -1765,7 +1765,7 @@ struct v4l2_ext_control { >>> struct v4l2_ctrl_vp9_compressed_hdr __user *p_vp9_compressed_hdr_probs; >>> struct v4l2_ctrl_vp9_frame __user *p_vp9_frame; >>> void __user *ptr; >>> - }; >>> + } __attribute__ ((packed)); >> >> This is a curious fix. It's applying a packed attribute to the union, >> which I presume means that it's then applying the packed attribute to >> any item in the union. >> >> The items are all either: __s32, __s64, values - or pointers. >> >> While applying this attribute here may fix the compiler warning, I'm not >> sure it's clear why this is required. This file also has other >> locations where a union inside a packed struct is not marked as packed. >> Should all unions be marked with the attribute? > > Interesting - I need to look deeper into packed. > >> Is there any more context from the compiler warning beyond what is >> reported above? > > I'll post a more detailed log asap. This is the log with clang 15 compiler… ../git/src/libcamera/v4l2_device.cpp | In file included from ../git/src/libcamera/v4l2_device.cpp:8: | In file included from ../git/include/libcamera/internal/v4l2_device.h:15: | ../git/include/linux/videodev2.h:1724:2: error: field within 'v4l2_ext_control' is less aligned than 'v4l2_ext_control::(anonymous union at ../git/include/linux/videodev2.h:1724:2)' and is usually due to 'v4l2_ext_control' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access] | union { | ^ | 1 error generated. Compiler version... # clang --version clang version 15.0.1 (https://github.com/llvm/llvm-project 29d395a1b7a8176abb1d6278f7df98301fbe7744) Target: x86_64-unknown-linux-gnu Thread model: posix > >> >> -- >> Kieran >> >> >>> } __attribute__ ((packed)); >>> >>> struct v4l2_ext_controls { >>> -- >>> 2.25.1 >>> > Thanks, > Devendra Resending because mailing list daemon is unhappy with rich text. Thanks, Devendra
> On 7 Oct 2022, at 20:04, Devendra Tewari <devendra.tewari@gmail.com> wrote: > > > >> No dia 7 de out. de 2022, às 18:58, Kieran Bingham <kieran.bingham@ideasonboard.com> escreveu: >> >> Hi Devendra, >> >> Quoting Devendra Tewari (2022-04-22 20:20:31) >>> Fixes clang warning: field within 'v4l2_ext_control' is less than >> >> Can you detail which version of clang this occurs with? Have you tried >> more than one version? >> > > This started happening with version 14.0.1 and I continue to see it with version 15. > >> >>> 'v4l2_ext_control::(anonymous union >>> >>> Signed-off-by: Devendra Tewari <devendra.tewari@gmail.com> >>> --- >>> include/uapi/linux/videodev2.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>> index 3768a0a80830..767c52c722cd 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -1765,7 +1765,7 @@ struct v4l2_ext_control { >>> struct v4l2_ctrl_vp9_compressed_hdr __user *p_vp9_compressed_hdr_probs; >>> struct v4l2_ctrl_vp9_frame __user *p_vp9_frame; >>> void __user *ptr; >>> - }; >>> + } __attribute__ ((packed)); >> >> This is a curious fix. It's applying a packed attribute to the union, >> which I presume means that it's then applying the packed attribute to >> any item in the union. >> >> The items are all either: __s32, __s64, values - or pointers. >> >> While applying this attribute here may fix the compiler warning, I'm not >> sure it's clear why this is required. This file also has other >> locations where a union inside a packed struct is not marked as packed. >> Should all unions be marked with the attribute? > > Interesting - I need to look deeper into packed. The only explanation I can think of is that this may be the only instance where a union inside a packed struct has other structs that are not packed. > >> Is there any more context from the compiler warning beyond what is >> reported above? > > I'll post a more detailed log asap. > >> >> -- >> Kieran >> >> >>> } __attribute__ ((packed)); >>> >>> struct v4l2_ext_controls { >>> -- >>> 2.25.1 >>> > Thanks, > Devendra Resending because mail daemon rejected rich text message. Thanks, Devendra
Hi Devendra, Cleaning up some old patches in On 08/10/2022 13:48, Devendra Tewari wrote: > > >> On 7 Oct 2022, at 20:04, Devendra Tewari <devendra.tewari@gmail.com> wrote: >> >> >> >>> No dia 7 de out. de 2022, às 18:58, Kieran Bingham <kieran.bingham@ideasonboard.com> escreveu: >>> >>> Hi Devendra, >>> >>> Quoting Devendra Tewari (2022-04-22 20:20:31) >>>> Fixes clang warning: field within 'v4l2_ext_control' is less than >>> >>> Can you detail which version of clang this occurs with? Have you tried >>> more than one version? >>> >> >> This started happening with version 14.0.1 and I continue to see it with version 15. >> >>> >>>> 'v4l2_ext_control::(anonymous union >>>> >>>> Signed-off-by: Devendra Tewari <devendra.tewari@gmail.com> >>>> --- >>>> include/uapi/linux/videodev2.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>>> index 3768a0a80830..767c52c722cd 100644 >>>> --- a/include/uapi/linux/videodev2.h >>>> +++ b/include/uapi/linux/videodev2.h >>>> @@ -1765,7 +1765,7 @@ struct v4l2_ext_control { >>>> struct v4l2_ctrl_vp9_compressed_hdr __user *p_vp9_compressed_hdr_probs; >>>> struct v4l2_ctrl_vp9_frame __user *p_vp9_frame; >>>> void __user *ptr; >>>> - }; >>>> + } __attribute__ ((packed)); >>> >>> This is a curious fix. It's applying a packed attribute to the union, >>> which I presume means that it's then applying the packed attribute to >>> any item in the union. >>> >>> The items are all either: __s32, __s64, values - or pointers. >>> >>> While applying this attribute here may fix the compiler warning, I'm not >>> sure it's clear why this is required. This file also has other >>> locations where a union inside a packed struct is not marked as packed. >>> Should all unions be marked with the attribute? >> >> Interesting - I need to look deeper into packed. > > The only explanation I can think of is that this may be the only instance where a union inside a packed struct has other structs that are not packed. > >> >>> Is there any more context from the compiler warning beyond what is >>> reported above? >> >> I'll post a more detailed log asap. I haven't heard anything back, so I am rejecting this. I am also afraid that changing this could cause unexpected ABI changes. Regards, Hans >> >>> >>> -- >>> Kieran >>> >>> >>>> } __attribute__ ((packed)); >>>> >>>> struct v4l2_ext_controls { >>>> -- >>>> 2.25.1 >>>> >> Thanks, >> Devendra > > Resending because mail daemon rejected rich text message. > > Thanks, > Devendra >
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 3768a0a80830..767c52c722cd 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -1765,7 +1765,7 @@ struct v4l2_ext_control { struct v4l2_ctrl_vp9_compressed_hdr __user *p_vp9_compressed_hdr_probs; struct v4l2_ctrl_vp9_frame __user *p_vp9_frame; void __user *ptr; - }; + } __attribute__ ((packed)); } __attribute__ ((packed)); struct v4l2_ext_controls {
Fixes clang warning: field within 'v4l2_ext_control' is less than 'v4l2_ext_control::(anonymous union Signed-off-by: Devendra Tewari <devendra.tewari@gmail.com> --- include/uapi/linux/videodev2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)