Hi Nick, Andy, On Tue, Jan 11, 2022 at 12:48:45PM -0800, Nick Desaulniers wrote: > On Tue, Jan 11, 2022 at 4:28 AM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > On Tue, Jan 11, 2022 at 12:47:17PM +0200, Sakari Ailus wrote: > > > Hi Nick, > > > > > > On Mon, Jan 10, 2022 at 03:11:18PM -0800, Nick Desaulniers wrote: > > > > On Mon, Jan 10, 2022 at 2:48 PM Sakari Ailus > > > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > > > > > Pointers V4L2 pixelformat and dataformat fields in a few packed structs > > > > > are directly passed to printk family of functions. > > > > > > > > I would rephrase the below statement... > > > > > > > > > This could result in an > > > > > unaligned access albeit no such possibility appears to exist at the > > > > > moment i.e. this clang warning appears to be a false positive. > > > > > > > > ...to: > > > > > > > > warning: taking address of packed member 'pixelformat' of class or > > > > structure 'v4l2_pix_format_mplane' may result in an unaligned pointer > > > > value [-Waddress-of-packed-member] > > > > > > > > The warning is correct; because `struct v4l2_pix_format_mplane` is > > > > __packed, it's members also have __aligned(1). Taking the address of > > > > such members results in the use of underaligned pointers which is UB > > > > and may be caught by UBSAN or fault on architectures without unaligned > > > > loads should the struct instance happen to be allocated without any > > > > natural alignment. > > > > > > Wouldn't that be the case only if the __packed attribute resulted in a > > > different memory layout than not having that attribute? > > > > > > All these fields are aligned by 4 so I don't see how this could be an > > > actual problem. > > > > packed means two things and developers often forgot about the second one: > > - the gaps between members in the data structures are removed > > - the instance of the data object may be on unaligned address > > Well put; the second is something that surprised me yesterday. I'd > like to say I'd forgotten, but I'm not sure I ever really knew that in > the first place...marking a struct as being packed seems like > shorthand for marking all of the members as having alignment of 1, > which makes sense since natural alignment requirements are what > prevent structure packing in the first place. I don't disagree with __packed allowing this but it is not the case here. The fields clang warns about are always aligned by 4. In other words, this warning is a false positive. -- Kind regards, Sakari Ailus
