[U-Boot] [PATCH v1] gcc-9: silence 'address-of-packed-member' warning

[Tom Rini]

On Fri, Nov 29, 2019 at 09:29:51PM +0100, Simon Goldschmidt wrote:
> 
> [bringing this back to the list, seems like I accidentally hit the single
> reply button before]
> 
> On 29.11.19 21:02, Andy Shevchenko wrote:
> > On Fri, Nov 29, 2019 at 06:56:44PM +0100, Simon Goldschmidt wrote:
> > > Andy Shevchenko <andriy.shevchenko at linux.intel.com> schrieb am Fr., 29.
> > > Nov. 2019, 18:48:
> > > 
> > > > GCC 9.x starts complaining about potential misalignment of the pointer to
> > > > the array (in this case alignment=2) in the packed (alignment=1)
> > > > structures.
> > > > 
> > > > Repeating Linus' Torvalds commit 6f303d60534c in the Linux kernel.
> > > > 
> > > > Original commit message:
> > > > 
> > > >    We already did this for clang, but now gcc has that warning too.
> > > >    Yes, yes, the address may be unaligned.  And that's kind of the point.
> > > > 
> > > > This in particular hides the warnings like
> > > > 
> > > > drivers/usb/gadget/composite.c:545:23: warning: taking address of packed
> > > > member of ‘struct usb_string_descriptor’ may result in an unaligned pointer
> > > > value [-Waddress-of-packed-member]
> > > >    545 |    collect_langs(sp, s->wData);
> > > > 
> > > 
> > > Is it really ok to hide this warning? This address is used for an u16
> > > pointer later, so it needs to be aligned. How do we ensure that wData is
> > > correctly aligned?
> > 
> > Why should it be?
> 
> Because this code is working on a packed struct, which may be unaligned. If
> it's not, why not remove packing on struct usb_string_descriptor instead of
> silencing this warning altogether?
> 
> > It worked before it will work after.
> 
> Who guarantees this struct is aligned/will stay aligned in the future?
> 
> Most of the platforms I know nowadays do work with unaligned access but are
> slow, so I think having the compiler warn here is good, no?
> 
> > 
> > > I took a different approach and fixed the called function to use byte
> > > access:
> > > 
> > > https://patchwork.ozlabs.org/patch/1199138/
> > 
> > So, that commit can be reverted then.
> 
> I admit this commit increases code size, but I'm not convinced that it's not
> necessary. If the access is always aligned, let's remove structure packing
> instead of reducing compiler warnings. (I still think most compiler warnings
> are good, not bad.)

In general terms, I agree that warnings can be helpful and good to know
when they exist.  Perhaps it's worth pursing this in the kernel
community to move this from and always-disable to something enabled at a
non-default W= number?

In more specific terms, we care so very much about binary size,
especially when it's not clearly a user-visible performance hit.  I do
not disagree with "X is technically wrong and should be fixed, now that
we see the warning showing it".  I also don't disagree with "with some
performance profiling we can see that unaligned access $here is a
problem".  But I do disagree with speculation on future CPUs not
supporting unaligned access or that it may be a performance problem.  We
don't control the former and can investigate the latter.

I also don't disagree with "as custodian for X I'm going to fix this
problem in my area.".

Thanks all!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191129/f1041ee8/attachment.sig>