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

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

[Andy Shevchenko]

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);

drivers/usb/gadget/composite.c:550:24: warning: taking address of packed member of ‘struct usb_string_descriptor’ may result in an unaligned pointer value [-Waddress-of-packed-member]
  550 |     collect_langs(sp, s->wData);

drivers/usb/gadget/composite.c:555:25: warning: taking address of packed member of ‘struct usb_string_descriptor’ may result in an unaligned pointer value [-Waddress-of-packed-member]
  555 |      collect_langs(sp, s->wData);

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7485bc2594..a0469f6a9c 100644
--- a/Makefile
+++ b/Makefile
@@ -672,11 +672,12 @@ endif
 endif
 
 KBUILD_CFLAGS += $(call cc-option,-Wno-format-nonliteral)
+KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
+
 ifeq ($(cc-name),clang)
 KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
 KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
 KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
-KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
 KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
 endif
 
-- 
2.24.0

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

[Tom Rini]

On Fri, Nov 29, 2019 at 07:47:59PM +0200, Andy Shevchenko wrote:

> 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);
> 
> drivers/usb/gadget/composite.c:550:24: warning: taking address of packed member of ‘struct usb_string_descriptor’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>   550 |     collect_langs(sp, s->wData);
> 
> drivers/usb/gadget/composite.c:555:25: warning: taking address of packed member of ‘struct usb_string_descriptor’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>   555 |      collect_langs(sp, s->wData);
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 7485bc2594..a0469f6a9c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -672,11 +672,12 @@ endif
>  endif
>  
>  KBUILD_CFLAGS += $(call cc-option,-Wno-format-nonliteral)
> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> +
>  ifeq ($(cc-name),clang)
>  KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> -KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
>  KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
>  endif

Ooh, I wish I had thought to look at the kernel a while ago.  I very
much like this idea and need to run a test to see how much space we
re-grain with this patch and reverting the handful of reworks that might
not make as much long term sense to do.  Thanks!

-- 
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/00bd60fa/attachment.sig>

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

[Simon Goldschmidt]

[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@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.)

Regards,
Simon

[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@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>

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

[Tom Rini]

On Fri, Nov 29, 2019 at 07:47:59PM +0200, Andy Shevchenko wrote:

> 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);
> 
> drivers/usb/gadget/composite.c:550:24: warning: taking address of packed member of ‘struct usb_string_descriptor’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>   550 |     collect_langs(sp, s->wData);
> 
> drivers/usb/gadget/composite.c:555:25: warning: taking address of packed member of ‘struct usb_string_descriptor’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>   555 |      collect_langs(sp, s->wData);
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied to u-boot/master, thanks!

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