[SLOF] [PATCH] libusb: Fix compiler warnings with GCC 9 in the USB code

[Alexey Kardashevskiy]

On 24/09/2019 19:09, Thomas Huth wrote:
> On 24/09/2019 11.03, Alexey Kardashevskiy wrote:
>>
>>
>> On 20/09/2019 19:06, Thomas Huth wrote:
>>> GCC 9 emits a lot of compiler warnings in the USB code like this:
>>>
>>> [...]
>>> usb-ohci.c: In function ‘ohci_get_last_frame’:
>>> usb-ohci.c:972:20: warning: taking address of packed member of ‘struct ohci_regs’
>>>  may result in an unaligned pointer value [-Waddress-of-packed-member]
>>>   972 |  return read_reg32(&regs->fm_num);
>>>       |                    ^~~~~~~~~~~~~
>>> 	[CC]	usb-ehci.o
>>> usb-ehci.c: In function ‘ehci_hub_check_ports’:
>>> usb-ehci.c:63:25: warning: taking address of packed member of ‘struct ehci_cap_regs’
>>>  may result in an unaligned pointer value [-Waddress-of-packed-member]
>>>    63 |  num_ports = read_reg32(&ehcd->cap_regs->hcsparams) & HCS_NPORTS_MASK;
>>>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
>>> usb-ehci.c:66:23: warning: taking address of packed member of ‘struct ehci_op_regs’
>>>  may result in an unaligned pointer value [-Waddress-of-packed-member]
>>>    66 |   portsc = read_reg32(&ehcd->op_regs->portsc[i]);
>>>       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~
>>> [...]
>>> usb-xhci.c:227:20: warning: taking address of packed member of ‘struct xhci_op_regs’
>>>  may result in an unaligned pointer value [-Waddress-of-packed-member]
>>>   227 |  xhci_wait_for_cnr(&op->usbsts);
>>>       |                    ^~~~~~~~~~~
>>> usb-xhci.c: In function ‘xhci_poll_event’:
>>> usb-xhci.c:309:14: warning: taking address of packed member of ‘struct xhci_int_regs’
>>>  may result in an unaligned pointer value [-Waddress-of-packed-member]
>>>   309 |  write_reg64(&xhcd->run_regs->irs[0].erdp, val);
>>>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> [...]
>>>
>>> Most of the structures are naturally aligned, so we can simply drop the
>>> "packed" attribute and use a _Static_assert() instead to make sure that
>>> there is no padding here.
>>>
>>> The only struct that needed some more attention is struct ehci_cap_regs.
>>> The portroute field is not aligned to a natural boundary here. But
>>> looking at the EHCI spec, it seems like this field is not meant as
>>> a full 64-bit value anyway, but rather an array of nibbles, so switch
>>> it to an array instead, then we can also drop the "packed" attribute
>>> here.
>>
>> This one is harder to fix properly (if I knew what "properly" means
>> here). I think that for this one the right thing seems to be disabling
>> -Waddress-of-packed-member warning whatsoever. May be add an assert for
>> hcidev->base alignment.
>>
>> Adding asserts as below will silence gcc but won't protect us against
>> (very unlikely to happen but nevertheless) unaligned pointers.
> 
> I disagree. Disabling -Waddress-of-packed-member will silence the
> warning, but won't protect us from unaligned pointers. With my patch,
> the structs should be naturally aligned all the time.

The registers are mapped from a device, "natural" from gcc does not
apply there, it is a different scope. You suggest we trust gcc not to
add any padding and what if it does in the future? If we are confident
that this cannot happen for some reason - then we do not need these
asserts; if we think this is somehow possible, then what? Fix this
again? "packed" is correct here, and gcc is just wrong in this
particular case.

Disabling -Waddress-of-packed-member means we take care of alignments
ourselves (which is PCI regs alignment + "packed" = explicit, clear, good).


Why is this (below) bad in particular?

iff --git a/lib/libusb/usb-ohci.h b/lib/libusb/usb-ohci.h
index f4535fd..e051f0f 100644
--- a/lib/libusb/usb-ohci.h
+++ b/lib/libusb/usb-ohci.h
@@ -47,7 +47,7 @@ struct ohci_regs {
        uint32_t rh_desc_b;
        uint32_t rh_status;
        uint32_t rh_ps[5];
-} __attribute__((packed));
+} __attribute__((packed)) __attribute__((aligned(4)));


-- 
Alexey