| Message ID | 20190920090643.9724-1-thuth@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libusb: Fix compiler warnings with GCC 9 in the USB code | expand |
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(®s->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. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/libusb/usb-ehci.c | 6 +++++- > lib/libusb/usb-ehci.h | 8 +++++--- > lib/libusb/usb-ohci.h | 3 ++- > lib/libusb/usb-xhci.h | 25 ++++++++++++++++++------- > 4 files changed, 30 insertions(+), 12 deletions(-) > > diff --git a/lib/libusb/usb-ehci.c b/lib/libusb/usb-ehci.c > index 60af9e1..299e9da 100644 > --- a/lib/libusb/usb-ehci.c > +++ b/lib/libusb/usb-ehci.c > @@ -38,7 +38,11 @@ static void dump_ehci_regs(struct ehci_hcd *ehcd) > dprintf("\n - HCIVERSION %04X", read_reg16(&cap_regs->hciversion)); > dprintf("\n - HCSPARAMS %08X", read_reg32(&cap_regs->hcsparams)); > dprintf("\n - HCCPARAMS %08X", read_reg32(&cap_regs->hccparams)); > - dprintf("\n - HCSP_PORTROUTE %016llX", read_reg64(&cap_regs->portroute)); > + dprintf("\n - HCSP_PORTROUTE %02X %02X %02X %02X %02X %02X %02X %02X", > + read_reg8(&cap_regs->portroute[0]), read_reg8(&cap_regs->portroute[1]), > + read_reg8(&cap_regs->portroute[2]), read_reg8(&cap_regs->portroute[3]), > + read_reg8(&cap_regs->portroute[4]), read_reg8(&cap_regs->portroute[5]), > + read_reg8(&cap_regs->portroute[6]), read_reg8(&cap_regs->portroute[7])); > dprintf("\n"); > > dprintf("\n - USBCMD %08X", read_reg32(&op_regs->usbcmd)); > diff --git a/lib/libusb/usb-ehci.h b/lib/libusb/usb-ehci.h > index 2955a9c..a834e95 100644 > --- a/lib/libusb/usb-ehci.h > +++ b/lib/libusb/usb-ehci.h > @@ -28,8 +28,9 @@ struct ehci_cap_regs { > uint16_t hciversion; > uint32_t hcsparams; > uint32_t hccparams; > - uint64_t portroute; > -} __attribute__ ((packed)); > + uint8_t portroute[8]; > +}; > +_Static_assert(sizeof(struct ehci_cap_regs) == 20, "padding in ehci_cap_regs"); > > struct ehci_op_regs { > uint32_t usbcmd; > @@ -42,7 +43,8 @@ struct ehci_op_regs { > uint32_t reserved[9]; > uint32_t configflag; > uint32_t portsc[0]; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct ehci_op_regs) == 68, "padding in ehci_op_regs"); > > struct ehci_framelist { > uint32_t fl_ptr[FL_SIZE]; > diff --git a/lib/libusb/usb-ohci.h b/lib/libusb/usb-ohci.h > index f4535fd..1f38047 100644 > --- a/lib/libusb/usb-ohci.h > +++ b/lib/libusb/usb-ohci.h > @@ -47,7 +47,8 @@ struct ohci_regs { > uint32_t rh_desc_b; > uint32_t rh_status; > uint32_t rh_ps[5]; > -} __attribute__((packed)); > +}; > +_Static_assert(sizeof(struct ohci_regs) == 104, "padding in ohci_regs"); > > #define EDA_FADDR(x) ((x & 0x7F)) > #define EDA_EP(x) ((x & 0x0F) << 7) > diff --git a/lib/libusb/usb-xhci.h b/lib/libusb/usb-xhci.h > index 02b382b..07c3957 100644 > --- a/lib/libusb/usb-xhci.h > +++ b/lib/libusb/usb-xhci.h > @@ -37,7 +37,8 @@ struct xhci_cap_regs { > #define XHCI_HCCPARAMS_XECP(x) ((x & 0xFFFF0000) >> 16) > uint32_t dboff; > uint32_t rtsoff; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_cap_regs) == 28, "padding in xhci_cap_regs"); > > /* USB 3.0: Section 7 and 7.2 */ > #define XHCI_XECP_CAP_ID(x) ((x & 0xF)) > @@ -90,7 +91,8 @@ struct xhci_port_regs { > uint32_t portpmsc; > uint32_t portli; > uint32_t reserved; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_port_regs) == 16, "padding in xhci_port_regs"); > > struct port_state { > bool PP; > @@ -132,7 +134,10 @@ struct xhci_op_regs { > /* USB Port register set */ > #define XHCI_PORT_MAX 256 > struct xhci_port_regs prs[XHCI_PORT_MAX]; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_op_regs) == 0x400 + > + XHCI_PORT_MAX * sizeof(struct xhci_port_regs), > + "padding in xhci_op_regs"); > > /* > * 5.5.2 Interrupter Register Set > @@ -148,7 +153,8 @@ struct xhci_int_regs { > #define XHCI_ERST_ADDR_MASK (~(0x3FUL)) > uint64_t erdp; > #define XHCI_ERDP_MASK (~(0xFUL)) > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_int_regs) == 32, "padding in xhci_int_regs"); > > /* 5.5 Host Controller Runtime Registers */ > struct xhci_run_regs { > @@ -156,12 +162,16 @@ struct xhci_run_regs { > uint8_t reserved[28]; > #define XHCI_IRS_MAX 1024 > struct xhci_int_regs irs[XHCI_IRS_MAX]; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_run_regs) == 32 + > + XHCI_IRS_MAX * sizeof(struct xhci_int_regs), > + "padding in xhci_run_regs"); > > /* 5.6 Doorbell Registers*/ > struct xhci_db_regs { > uint32_t db[256]; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_db_regs) == 1024, "padding in xhci_db_regs"); > > #define COMP_SUCCESS 1 > > @@ -259,7 +269,8 @@ struct xhci_erst_entry { > uint64_t addr; > uint32_t size; > uint32_t reserved; > -} __attribute__ ((packed)); > +}; > +_Static_assert(sizeof(struct xhci_erst_entry) == 16, "padding in xhci_erst_entry"); > > struct xhci_erst { > struct xhci_erst_entry *entries; >
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(®s->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. Thomas
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(®s->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)));
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(®s->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
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(®s->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 >
diff --git a/lib/libusb/usb-ehci.c b/lib/libusb/usb-ehci.c index 60af9e1..299e9da 100644 --- a/lib/libusb/usb-ehci.c +++ b/lib/libusb/usb-ehci.c @@ -38,7 +38,11 @@ static void dump_ehci_regs(struct ehci_hcd *ehcd) dprintf("\n - HCIVERSION %04X", read_reg16(&cap_regs->hciversion)); dprintf("\n - HCSPARAMS %08X", read_reg32(&cap_regs->hcsparams)); dprintf("\n - HCCPARAMS %08X", read_reg32(&cap_regs->hccparams)); - dprintf("\n - HCSP_PORTROUTE %016llX", read_reg64(&cap_regs->portroute)); + dprintf("\n - HCSP_PORTROUTE %02X %02X %02X %02X %02X %02X %02X %02X", + read_reg8(&cap_regs->portroute[0]), read_reg8(&cap_regs->portroute[1]), + read_reg8(&cap_regs->portroute[2]), read_reg8(&cap_regs->portroute[3]), + read_reg8(&cap_regs->portroute[4]), read_reg8(&cap_regs->portroute[5]), + read_reg8(&cap_regs->portroute[6]), read_reg8(&cap_regs->portroute[7])); dprintf("\n"); dprintf("\n - USBCMD %08X", read_reg32(&op_regs->usbcmd)); diff --git a/lib/libusb/usb-ehci.h b/lib/libusb/usb-ehci.h index 2955a9c..a834e95 100644 --- a/lib/libusb/usb-ehci.h +++ b/lib/libusb/usb-ehci.h @@ -28,8 +28,9 @@ struct ehci_cap_regs { uint16_t hciversion; uint32_t hcsparams; uint32_t hccparams; - uint64_t portroute; -} __attribute__ ((packed)); + uint8_t portroute[8]; +}; +_Static_assert(sizeof(struct ehci_cap_regs) == 20, "padding in ehci_cap_regs"); struct ehci_op_regs { uint32_t usbcmd; @@ -42,7 +43,8 @@ struct ehci_op_regs { uint32_t reserved[9]; uint32_t configflag; uint32_t portsc[0]; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct ehci_op_regs) == 68, "padding in ehci_op_regs"); struct ehci_framelist { uint32_t fl_ptr[FL_SIZE]; diff --git a/lib/libusb/usb-ohci.h b/lib/libusb/usb-ohci.h index f4535fd..1f38047 100644 --- a/lib/libusb/usb-ohci.h +++ b/lib/libusb/usb-ohci.h @@ -47,7 +47,8 @@ struct ohci_regs { uint32_t rh_desc_b; uint32_t rh_status; uint32_t rh_ps[5]; -} __attribute__((packed)); +}; +_Static_assert(sizeof(struct ohci_regs) == 104, "padding in ohci_regs"); #define EDA_FADDR(x) ((x & 0x7F)) #define EDA_EP(x) ((x & 0x0F) << 7) diff --git a/lib/libusb/usb-xhci.h b/lib/libusb/usb-xhci.h index 02b382b..07c3957 100644 --- a/lib/libusb/usb-xhci.h +++ b/lib/libusb/usb-xhci.h @@ -37,7 +37,8 @@ struct xhci_cap_regs { #define XHCI_HCCPARAMS_XECP(x) ((x & 0xFFFF0000) >> 16) uint32_t dboff; uint32_t rtsoff; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_cap_regs) == 28, "padding in xhci_cap_regs"); /* USB 3.0: Section 7 and 7.2 */ #define XHCI_XECP_CAP_ID(x) ((x & 0xF)) @@ -90,7 +91,8 @@ struct xhci_port_regs { uint32_t portpmsc; uint32_t portli; uint32_t reserved; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_port_regs) == 16, "padding in xhci_port_regs"); struct port_state { bool PP; @@ -132,7 +134,10 @@ struct xhci_op_regs { /* USB Port register set */ #define XHCI_PORT_MAX 256 struct xhci_port_regs prs[XHCI_PORT_MAX]; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_op_regs) == 0x400 + + XHCI_PORT_MAX * sizeof(struct xhci_port_regs), + "padding in xhci_op_regs"); /* * 5.5.2 Interrupter Register Set @@ -148,7 +153,8 @@ struct xhci_int_regs { #define XHCI_ERST_ADDR_MASK (~(0x3FUL)) uint64_t erdp; #define XHCI_ERDP_MASK (~(0xFUL)) -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_int_regs) == 32, "padding in xhci_int_regs"); /* 5.5 Host Controller Runtime Registers */ struct xhci_run_regs { @@ -156,12 +162,16 @@ struct xhci_run_regs { uint8_t reserved[28]; #define XHCI_IRS_MAX 1024 struct xhci_int_regs irs[XHCI_IRS_MAX]; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_run_regs) == 32 + + XHCI_IRS_MAX * sizeof(struct xhci_int_regs), + "padding in xhci_run_regs"); /* 5.6 Doorbell Registers*/ struct xhci_db_regs { uint32_t db[256]; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_db_regs) == 1024, "padding in xhci_db_regs"); #define COMP_SUCCESS 1 @@ -259,7 +269,8 @@ struct xhci_erst_entry { uint64_t addr; uint32_t size; uint32_t reserved; -} __attribute__ ((packed)); +}; +_Static_assert(sizeof(struct xhci_erst_entry) == 16, "padding in xhci_erst_entry"); struct xhci_erst { struct xhci_erst_entry *entries;
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(®s->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. Signed-off-by: Thomas Huth <thuth@redhat.com> --- lib/libusb/usb-ehci.c | 6 +++++- lib/libusb/usb-ehci.h | 8 +++++--- lib/libusb/usb-ohci.h | 3 ++- lib/libusb/usb-xhci.h | 25 ++++++++++++++++++------- 4 files changed, 30 insertions(+), 12 deletions(-)