Message ID | 20191129174759.20082-1-andriy.shevchenko@linux.intel.com |
---|---|
State | Accepted |
Commit | 53dc8ae66c5ca8dac65bd6f51c0cbb8b0d22aa2a |
Delegated to: | Tom Rini |
Headers | show |
Series | [U-Boot,v1] gcc-9: silence 'address-of-packed-member' warning | expand |
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!
[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
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!
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!
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
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(-)