Re: [U-Boot] [PATCH v1] gcc-9: silence 'address-of-packed-member' warning
[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:
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:
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!
participants (2)
-
Simon Goldschmidt -
Tom Rini