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

[Thomas Huth]

On 25/09/2019 02.40, Alexey Kardashevskiy wrote:
> 
> 
> 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?

The alignment is defined in the ELF ABI, so unless you try to compile
SLOF with a compiler that has a completely different ABI in mind, the
"natural" alignment is fixed. Thus if you mind the _Static_assert()s so
much, we could also simply omit them.

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

Again, the warning indicates that we're dealing with undefined behavior
here. If you shut up the warning with -Wno-address-of-packed-member,
there is a risk (a small one, but still possible) that future versions
of GCC just silently generate code that you don't expect (or even invoke
some nasal demons [1]).

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

Hmm, does that make the warnings go away, even without
-Wno-address-of-packed-member ? ... I think that might then be an
option, too. But I'd rather write it as:

 __attribute__((packed, aligned(4)));

  Thomas


[1] See https://en.wikipedia.org/wiki/Undefined_behavior for example