[PATCH 1/1] usb: avoid -Werror=address-of-packed-member

[Tom Rini]

On Tue, Dec 17, 2019 at 08:52:01PM +0100, Marek Vasut wrote:
> On 12/17/19 8:32 PM, Heinrich Schuchardt wrote:
> > On 12/17/19 1:19 PM, Marek Vasut wrote:
> >> On 12/17/19 12:59 PM, Heinrich Schuchardt wrote:
> >>> On 12/17/19 12:19 PM, Marek Vasut wrote:
> >>>> On 12/17/19 12:14 PM, Heinrich Schuchardt wrote:
> >>>>> On 12/17/19 11:00 AM, Marek Vasut wrote:
> >>>>>> On 12/17/19 10:27 AM, Heinrich Schuchardt wrote:
> >>>>>>> With GCC 9.2.1 errors of type -Werror=address-of-packed-member occur
> >>>>>>> when
> >>>>>>> passing a member of a packed structure to le16_to_cpus() on a big
> >>>>>>> endian
> >>>>>>> system (e.g. P2041RDB_defconfig).
> >>>>>>>
> >>>>>>> Replace le16_to_cpus() by get_unaligned_le16(). Check
> >>>>>>> defined(__BIG_ENDIAN)
> >>>>>>> to avoid the introduction of unnecessary instructions on little
> >>>>>>> endian
> >>>>>>> systems as seen on aarch64.
> >>>>>>
> >>>>>> I would expect the compiler would optimize such stuff out ?
> >>>>>> Can we do without the ifdef ?
> >>>>>
> >>>>> When compiling qemu_arm64_defconfig without the #ifdef the GCC 9.2.1
> >>>>> adds assembler instructions:
> >>>>
> >>>> Why ?
> >>>>
> >>>
> >>> I am not a GCC developer. I simply observed that GCC currently cannot
> >>> optimize this away on its own. That is why I added the #ifdef.
> >>
> >> Are we now adding workarounds instead of solving issues properly?
> >>
> >>> Identifying that bit operations like << 8 in __get_unaligned_le16() have
> >>> zero effect is not an easy task when developing a compiler. You would
> >>> have to follow the flow of every bit.
> >>
> >> Maybe the fix is then to somehow optimize the get_unaligned_le16() to
> >> help the compiler ?
> > 
> > Inside get_unaligned_le16() it is not known that we will be reassigning
> > to the same memory location. So I cannot imagine what to improve here.
> > 
> > You could invent new functions for in place byte swapping. But that
> > would only move the #ifdef to a different place.
> 
> Isn't there already such a function in Linux ?
> 
> Also, why would you need the ifdef if the compiler would now know that
> the operation is noop ?

This particular patch looks to me exactly why we want to follow the
Linux Kernel and disable this particular warning for GCC like we already
do for LLVM.

-- 
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/20191230/02ff3b3b/attachment.sig>