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

[Alexey Kardashevskiy]

On 25/09/2019 17:47, Thomas Huth wrote:
> 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. 

s/we're/we might be/
And it is not the case here anyway.

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


Sure, that was a cut-n-paste from one of few structs you did not relieve from packing :) Care to repost? Thanks,


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

-- 
Alexey