Re: [PATCH 1/1] v4l: Avoid unaligned access warnings when printing 4cc modifiers

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@linux.intel.com> 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@linux.intel.com> 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