| Message ID | 20190919174859.29360-1-thuth@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libnet: Fix compiler warnings with GCC 9 | expand |
On Thu, 19 Sep 2019 19:48:59 +0200 Thomas Huth <thuth@redhat.com> wrote: > When compiling the libnet code with GCC 9, there are some new > compiler warnings popping up now: > > ipv6.c: In function ‘handle_ipv6’: > ipv6.c:152:21: warning: taking address of packed member of ‘struct ip6hdr’ > may result in an unaligned pointer value [-Waddress-of-packed-member] > 152 | if (! find_ip6addr(&(ip6->dst))) > | ^~~~~~~~~~~ > ipv6.c: In function ‘ip6_checksum’: > ipv6.c:455:2: warning: converting a packed ‘struct ip6hdr’ pointer > (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result > in an unaligned pointer value [-Waddress-of-packed-member] > 455 | pip6h = (unsigned short *) &pseudo_ip6h; > | ^~~~~ > In file included from ipv6.c:21: > ipv6.h:86:8: note: defined here > 86 | struct ip6hdr { > | ^~~~~~ > ipv6.c:522:8: warning: converting a packed ‘struct icmp6hdr’ pointer > (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result > in an unaligned pointer value [-Waddress-of-packed-member] > 522 | ip6h->pl >> 1); > | ^~~~ > In file included from ipv6.c:22: > icmpv6.h:123:8: note: defined here > 123 | struct icmp6hdr { > | ^~~~~~~~ > etc. > > The entries in struct ip6hdr are naturally aligned, so we can simply > drop the __attribute__ ((packed)) here and use a _Static_assert() for > the correct size instead. > icmp6hdr is a more complex struct since it contains a union of > packed structs, but the entries before the union are naturally aligned, > too, so we can silence the compiler warning by dropping the "packed" > attribute from the struct icmp6hdr and just asserting that the union > is at the correct offset. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/libnet/icmpv6.h | 4 +++- > lib/libnet/ipv6.h | 3 ++- > 2 files changed, 5 insertions(+), 2 deletions(-) This fixes building the s390 ccw bios with netboot support on a Fedora 30 s390 LPAR for me. Tested-by: Cornelia Huck <cohuck@redhat.com> The changes also look sane to me. Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 20/09/2019 03:48, Thomas Huth wrote: > When compiling the libnet code with GCC 9, there are some new > compiler warnings popping up now: > > ipv6.c: In function ‘handle_ipv6’: > ipv6.c:152:21: warning: taking address of packed member of ‘struct ip6hdr’ > may result in an unaligned pointer value [-Waddress-of-packed-member] > 152 | if (! find_ip6addr(&(ip6->dst))) > | ^~~~~~~~~~~ > ipv6.c: In function ‘ip6_checksum’: > ipv6.c:455:2: warning: converting a packed ‘struct ip6hdr’ pointer > (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result > in an unaligned pointer value [-Waddress-of-packed-member] > 455 | pip6h = (unsigned short *) &pseudo_ip6h; > | ^~~~~ > In file included from ipv6.c:21: > ipv6.h:86:8: note: defined here > 86 | struct ip6hdr { > | ^~~~~~ > ipv6.c:522:8: warning: converting a packed ‘struct icmp6hdr’ pointer > (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result > in an unaligned pointer value [-Waddress-of-packed-member] > 522 | ip6h->pl >> 1); > | ^~~~ > In file included from ipv6.c:22: > icmpv6.h:123:8: note: defined here > 123 | struct icmp6hdr { > | ^~~~~~~~ > etc. > > The entries in struct ip6hdr are naturally aligned, so we can simply > drop the __attribute__ ((packed)) here and use a _Static_assert() for > the correct size instead. > icmp6hdr is a more complex struct since it contains a union of > packed structs, but the entries before the union are naturally aligned, > too, so we can silence the compiler warning by dropping the "packed" > attribute from the struct icmp6hdr and just asserting that the union > is at the correct offset. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/libnet/icmpv6.h | 4 +++- > lib/libnet/ipv6.h | 3 ++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/libnet/icmpv6.h b/lib/libnet/icmpv6.h > index 41b0c70..ba51524 100644 > --- a/lib/libnet/icmpv6.h > +++ b/lib/libnet/icmpv6.h > @@ -130,6 +130,8 @@ struct icmp6hdr { > struct router_solicitation router_solicit; > struct router_advertisement ra; > } icmp6body; > -} __attribute((packed)); > +}; > +_Static_assert((long)&(((struct icmp6hdr *)NULL)->icmp6body) == 4, > + "unexpected padding in icmp6hdr"); > > #endif > diff --git a/lib/libnet/ipv6.h b/lib/libnet/ipv6.h > index 7b71b50..5fb718e 100644 > --- a/lib/libnet/ipv6.h > +++ b/lib/libnet/ipv6.h > @@ -90,7 +90,8 @@ struct ip6hdr { > uint8_t hl; /**< Hop limit */ > ip6_addr_t src; /**< IPv6 source address */ > ip6_addr_t dst; /**< IPv6 destination address */ > -} __attribute((packed)); The (packed) rather explicitly says it is a binary format which is a good thing. > +}; > +_Static_assert(sizeof(struct ip6hdr) == 40, "unexpected padding in ip6hdr"); This hurts my eyes. I propose this approach (gcc8 compile tested only, I still to compile my own gcc9): https://github.com/aik/SLOF/commit/2cdb340ac5026d0478eb873c0ad57bd6dc0a289f Does this work? Basically instead of passing "unaligned" pointers, do the right thing and pass values instead, we are not that much restricted in SLOF to save bytes. Or disable this particular warning. What exactly does it protect against? We can do unaligned memory access except few very special cases such as spinlocks and this is not the case here. Same comments apply to the USB patch. Thanks, > > /** \struct packeth > * Struct with pointers to headers within a packet >
On 23/09/2019 04.21, Alexey Kardashevskiy wrote: > > > On 20/09/2019 03:48, Thomas Huth wrote: >> When compiling the libnet code with GCC 9, there are some new >> compiler warnings popping up now: >> >> ipv6.c: In function ‘handle_ipv6’: >> ipv6.c:152:21: warning: taking address of packed member of ‘struct ip6hdr’ >> may result in an unaligned pointer value [-Waddress-of-packed-member] >> 152 | if (! find_ip6addr(&(ip6->dst))) >> | ^~~~~~~~~~~ >> ipv6.c: In function ‘ip6_checksum’: >> ipv6.c:455:2: warning: converting a packed ‘struct ip6hdr’ pointer >> (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result >> in an unaligned pointer value [-Waddress-of-packed-member] >> 455 | pip6h = (unsigned short *) &pseudo_ip6h; >> | ^~~~~ >> In file included from ipv6.c:21: >> ipv6.h:86:8: note: defined here >> 86 | struct ip6hdr { >> | ^~~~~~ >> ipv6.c:522:8: warning: converting a packed ‘struct icmp6hdr’ pointer >> (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result >> in an unaligned pointer value [-Waddress-of-packed-member] >> 522 | ip6h->pl >> 1); >> | ^~~~ >> In file included from ipv6.c:22: >> icmpv6.h:123:8: note: defined here >> 123 | struct icmp6hdr { >> | ^~~~~~~~ >> etc. >> >> The entries in struct ip6hdr are naturally aligned, so we can simply >> drop the __attribute__ ((packed)) here and use a _Static_assert() for >> the correct size instead. >> icmp6hdr is a more complex struct since it contains a union of >> packed structs, but the entries before the union are naturally aligned, >> too, so we can silence the compiler warning by dropping the "packed" >> attribute from the struct icmp6hdr and just asserting that the union >> is at the correct offset. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> lib/libnet/icmpv6.h | 4 +++- >> lib/libnet/ipv6.h | 3 ++- >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/lib/libnet/icmpv6.h b/lib/libnet/icmpv6.h >> index 41b0c70..ba51524 100644 >> --- a/lib/libnet/icmpv6.h >> +++ b/lib/libnet/icmpv6.h >> @@ -130,6 +130,8 @@ struct icmp6hdr { >> struct router_solicitation router_solicit; >> struct router_advertisement ra; >> } icmp6body; >> -} __attribute((packed)); >> +}; >> +_Static_assert((long)&(((struct icmp6hdr *)NULL)->icmp6body) == 4, >> + "unexpected padding in icmp6hdr"); >> >> #endif >> diff --git a/lib/libnet/ipv6.h b/lib/libnet/ipv6.h >> index 7b71b50..5fb718e 100644 >> --- a/lib/libnet/ipv6.h >> +++ b/lib/libnet/ipv6.h >> @@ -90,7 +90,8 @@ struct ip6hdr { >> uint8_t hl; /**< Hop limit */ >> ip6_addr_t src; /**< IPv6 source address */ >> ip6_addr_t dst; /**< IPv6 destination address */ >> -} __attribute((packed)); > > > The (packed) rather explicitly says it is a binary format which is a > good thing. It's ok for architectures where you know that the CPU can do unaligned memory accesses. But it's ugly in generic code that should be compilable on any architecture, then these packed structs are often rather a nuisance - we've had this problems in the QEMU sources a couple of times already. So I rather prefer to write code *without* the "packed" attribute nowadays. >> +}; >> +_Static_assert(sizeof(struct ip6hdr) == 40, "unexpected padding in ip6hdr"); > > This hurts my eyes. Really? Would it be better if it was wrapped in a nice macro? > I propose this approach (gcc8 compile tested only, I still to compile my > own gcc9): > > https://github.com/aik/SLOF/commit/2cdb340ac5026d0478eb873c0ad57bd6dc0a289f > > Does this work? Basically instead of passing "unaligned" pointers, do > the right thing and pass values instead, we are not that much restricted > in SLOF to save bytes. That approach looks fine at a first glance to me, too -- at least for the libnet code. But your patch didn't fix all warnings yet. These are left: lib/libnet/ipv6.c: In function ‘ip6_checksum’: lib/libnet/ipv6.c:452:2: warning: converting a packed ‘struct ip6hdr’ pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result in an unaligned pointer value [-Waddress-of-packed-member] 452 | pip6h = (unsigned short *) &pseudo_ip6h; | ^~~~~ In file included from lib/libnet/ipv6.c:21: lib/libnet/ipv6.h:86:8: note: defined here 86 | struct ip6hdr { | ^~~~~~ CC pc-bios/s390-ccw/ndp.o lib/libnet/ipv6.c: In function ‘send_ipv6’: lib/libnet/ipv6.c:519:8: warning: converting a packed ‘struct icmp6hdr’ pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result in an unaligned pointer value [-Waddress-of-packed-member] 519 | ip6h->pl >> 1); | ^~~~ In file included from lib/libnet/ipv6.c:22: lib/libnet/icmpv6.h:123:8: note: defined here 123 | struct icmp6hdr { | ^~~~~~~~ lib/libnet/ipv6.c:527:32: warning: taking address of packed member of ‘struct ip6hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] 527 | gw = ipv6_get_default_router(&ip6h->src); | ^~~~~~~~~~ lib/libnet/icmpv6.c: In function ‘handle_ra’: lib/libnet/icmpv6.c:171:11: warning: taking address of packed member of ‘struct ip6hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] 171 | rtr_ip = (ip6_addr_t *) &ip6h->src; | ^~~~~~~~~~~~~~~~~~~~~~~~~ At least the ip6_checksum()-related warnings can not be silenced that easily with your approach of passing parameters by value instead of pointer... > Or disable this particular warning. What exactly > does it protect against? If I've googled it right, assigning an unaligned pointer value is undefined behavior according to the C standard. So the compiler could do weird stuff if we simply ignore it. Thomas
On 23/09/2019 16:16, Thomas Huth wrote: > On 23/09/2019 04.21, Alexey Kardashevskiy wrote: >> >> >> On 20/09/2019 03:48, Thomas Huth wrote: >>> When compiling the libnet code with GCC 9, there are some new >>> compiler warnings popping up now: >>> >>> ipv6.c: In function ‘handle_ipv6’: >>> ipv6.c:152:21: warning: taking address of packed member of ‘struct ip6hdr’ >>> may result in an unaligned pointer value [-Waddress-of-packed-member] >>> 152 | if (! find_ip6addr(&(ip6->dst))) >>> | ^~~~~~~~~~~ >>> ipv6.c: In function ‘ip6_checksum’: >>> ipv6.c:455:2: warning: converting a packed ‘struct ip6hdr’ pointer >>> (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result >>> in an unaligned pointer value [-Waddress-of-packed-member] >>> 455 | pip6h = (unsigned short *) &pseudo_ip6h; >>> | ^~~~~ >>> In file included from ipv6.c:21: >>> ipv6.h:86:8: note: defined here >>> 86 | struct ip6hdr { >>> | ^~~~~~ >>> ipv6.c:522:8: warning: converting a packed ‘struct icmp6hdr’ pointer >>> (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result >>> in an unaligned pointer value [-Waddress-of-packed-member] >>> 522 | ip6h->pl >> 1); >>> | ^~~~ >>> In file included from ipv6.c:22: >>> icmpv6.h:123:8: note: defined here >>> 123 | struct icmp6hdr { >>> | ^~~~~~~~ >>> etc. >>> >>> The entries in struct ip6hdr are naturally aligned, so we can simply >>> drop the __attribute__ ((packed)) here and use a _Static_assert() for >>> the correct size instead. >>> icmp6hdr is a more complex struct since it contains a union of >>> packed structs, but the entries before the union are naturally aligned, >>> too, so we can silence the compiler warning by dropping the "packed" >>> attribute from the struct icmp6hdr and just asserting that the union >>> is at the correct offset. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> lib/libnet/icmpv6.h | 4 +++- >>> lib/libnet/ipv6.h | 3 ++- >>> 2 files changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/libnet/icmpv6.h b/lib/libnet/icmpv6.h >>> index 41b0c70..ba51524 100644 >>> --- a/lib/libnet/icmpv6.h >>> +++ b/lib/libnet/icmpv6.h >>> @@ -130,6 +130,8 @@ struct icmp6hdr { >>> struct router_solicitation router_solicit; >>> struct router_advertisement ra; >>> } icmp6body; >>> -} __attribute((packed)); >>> +}; >>> +_Static_assert((long)&(((struct icmp6hdr *)NULL)->icmp6body) == 4, >>> + "unexpected padding in icmp6hdr"); >>> >>> #endif >>> diff --git a/lib/libnet/ipv6.h b/lib/libnet/ipv6.h >>> index 7b71b50..5fb718e 100644 >>> --- a/lib/libnet/ipv6.h >>> +++ b/lib/libnet/ipv6.h >>> @@ -90,7 +90,8 @@ struct ip6hdr { >>> uint8_t hl; /**< Hop limit */ >>> ip6_addr_t src; /**< IPv6 source address */ >>> ip6_addr_t dst; /**< IPv6 destination address */ >>> -} __attribute((packed)); >> >> >> The (packed) rather explicitly says it is a binary format which is a >> good thing. > > It's ok for architectures where you know that the CPU can do unaligned > memory accesses. But it's ugly in generic code that should be compilable > on any architecture, then these packed structs are often rather a > nuisance - we've had this problems in the QEMU sources a couple of times > already. So I rather prefer to write code *without* the "packed" > attribute nowadays. It is a binary format, just a different scope, how is this a nuisance? They have to be packed anyway. >>> +}; >>> +_Static_assert(sizeof(struct ip6hdr) == 40, "unexpected padding in ip6hdr"); >> >> This hurts my eyes. > > Really? Would it be better if it was wrapped in a nice macro? This one at least checks the whole struct size but the other one was checking an offset, that one did hurt. A programming language should provide guarantees of this kind of alignments and (packed) does exactly this. >> I propose this approach (gcc8 compile tested only, I still to compile my >> own gcc9): >> >> https://github.com/aik/SLOF/commit/2cdb340ac5026d0478eb873c0ad57bd6dc0a289f >> >> Does this work? Basically instead of passing "unaligned" pointers, do >> the right thing and pass values instead, we are not that much restricted >> in SLOF to save bytes. > > That approach looks fine at a first glance to me, too -- at least for > the libnet code. > > But your patch didn't fix all warnings yet. These are left: Sure, I have not tried to fix that, this is just a direction, you are welcome to finish the task :) Or I can do that when I get time to compile gcc9, recover my ipv6 setup for testing and try myself. > > lib/libnet/ipv6.c: In function ‘ip6_checksum’: > lib/libnet/ipv6.c:452:2: warning: converting a packed ‘struct ip6hdr’ > pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) > may result in an unaligned pointer value [-Waddress-of-packed-member] > 452 | pip6h = (unsigned short *) &pseudo_ip6h; > | ^~~~~ > In file included from lib/libnet/ipv6.c:21: > lib/libnet/ipv6.h:86:8: note: defined here > 86 | struct ip6hdr { > | ^~~~~~ > CC pc-bios/s390-ccw/ndp.o > lib/libnet/ipv6.c: In function ‘send_ipv6’: > lib/libnet/ipv6.c:519:8: warning: converting a packed ‘struct icmp6hdr’ > pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) > may result in an unaligned pointer value [-Waddress-of-packed-member] > 519 | ip6h->pl >> 1); > | ^~~~ > In file included from lib/libnet/ipv6.c:22: > lib/libnet/icmpv6.h:123:8: note: defined here > 123 | struct icmp6hdr { > | ^~~~~~~~ > lib/libnet/ipv6.c:527:32: warning: taking address of packed member of > ‘struct ip6hdr’ may result in an unaligned pointer value > [-Waddress-of-packed-member] > 527 | gw = ipv6_get_default_router(&ip6h->src); > | ^~~~~~~~~~ > lib/libnet/icmpv6.c: In function ‘handle_ra’: > lib/libnet/icmpv6.c:171:11: warning: taking address of packed member of > ‘struct ip6hdr’ may result in an unaligned pointer value > [-Waddress-of-packed-member] > 171 | rtr_ip = (ip6_addr_t *) &ip6h->src; > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > At least the ip6_checksum()-related warnings can not be silenced that > easily with your approach of passing parameters by value instead of > pointer... Then use unions: https://github.com/aik/SLOF/commit/db37446ad ip6_checksum() is weird btw. >> Or disable this particular warning. What exactly >> does it protect against? > If I've googled it right, assigning an unaligned pointer value is > undefined behavior according to the C standard. So the compiler could do > weird stuff if we simply ignore it. Huh. Well. There we go - no taking references of struct members.
On 23/09/2019 08.44, Alexey Kardashevskiy wrote: > > > On 23/09/2019 16:16, Thomas Huth wrote: >> On 23/09/2019 04.21, Alexey Kardashevskiy wrote: >>> >>> >>> On 20/09/2019 03:48, Thomas Huth wrote: >>>> When compiling the libnet code with GCC 9, there are some new >>>> compiler warnings popping up now: >>>> >>>> ipv6.c: In function ‘handle_ipv6’: >>>> ipv6.c:152:21: warning: taking address of packed member of ‘struct ip6hdr’ >>>> may result in an unaligned pointer value [-Waddress-of-packed-member] >>>> 152 | if (! find_ip6addr(&(ip6->dst))) >>>> | ^~~~~~~~~~~ >>>> ipv6.c: In function ‘ip6_checksum’: >>>> ipv6.c:455:2: warning: converting a packed ‘struct ip6hdr’ pointer >>>> (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result >>>> in an unaligned pointer value [-Waddress-of-packed-member] >>>> 455 | pip6h = (unsigned short *) &pseudo_ip6h; >>>> | ^~~~~ >>>> In file included from ipv6.c:21: >>>> ipv6.h:86:8: note: defined here >>>> 86 | struct ip6hdr { >>>> | ^~~~~~ >>>> ipv6.c:522:8: warning: converting a packed ‘struct icmp6hdr’ pointer >>>> (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result >>>> in an unaligned pointer value [-Waddress-of-packed-member] >>>> 522 | ip6h->pl >> 1); >>>> | ^~~~ >>>> In file included from ipv6.c:22: >>>> icmpv6.h:123:8: note: defined here >>>> 123 | struct icmp6hdr { >>>> | ^~~~~~~~ >>>> etc. >>>> >>>> The entries in struct ip6hdr are naturally aligned, so we can simply >>>> drop the __attribute__ ((packed)) here and use a _Static_assert() for >>>> the correct size instead. >>>> icmp6hdr is a more complex struct since it contains a union of >>>> packed structs, but the entries before the union are naturally aligned, >>>> too, so we can silence the compiler warning by dropping the "packed" >>>> attribute from the struct icmp6hdr and just asserting that the union >>>> is at the correct offset. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> lib/libnet/icmpv6.h | 4 +++- >>>> lib/libnet/ipv6.h | 3 ++- >>>> 2 files changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/lib/libnet/icmpv6.h b/lib/libnet/icmpv6.h >>>> index 41b0c70..ba51524 100644 >>>> --- a/lib/libnet/icmpv6.h >>>> +++ b/lib/libnet/icmpv6.h >>>> @@ -130,6 +130,8 @@ struct icmp6hdr { >>>> struct router_solicitation router_solicit; >>>> struct router_advertisement ra; >>>> } icmp6body; >>>> -} __attribute((packed)); >>>> +}; >>>> +_Static_assert((long)&(((struct icmp6hdr *)NULL)->icmp6body) == 4, >>>> + "unexpected padding in icmp6hdr"); >>>> >>>> #endif >>>> diff --git a/lib/libnet/ipv6.h b/lib/libnet/ipv6.h >>>> index 7b71b50..5fb718e 100644 >>>> --- a/lib/libnet/ipv6.h >>>> +++ b/lib/libnet/ipv6.h >>>> @@ -90,7 +90,8 @@ struct ip6hdr { >>>> uint8_t hl; /**< Hop limit */ >>>> ip6_addr_t src; /**< IPv6 source address */ >>>> ip6_addr_t dst; /**< IPv6 destination address */ >>>> -} __attribute((packed)); >>> >>> >>> The (packed) rather explicitly says it is a binary format which is a >>> good thing. >> >> It's ok for architectures where you know that the CPU can do unaligned >> memory accesses. But it's ugly in generic code that should be compilable >> on any architecture, then these packed structs are often rather a >> nuisance - we've had this problems in the QEMU sources a couple of times >> already. So I rather prefer to write code *without* the "packed" >> attribute nowadays. > > It is a binary format, just a different scope, how is this a nuisance? If the pointer is unaligned and you somehow assign it to a non-packed pointer, the code won't work on architectures like Sparc and certain ARM chips. We've had to fix many spots in QEMU due to this in the past already, see for example: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3b8afb41bc8eef42c3e https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7357b2215978debf2fd https://git.qemu.org/?p=qemu.git;a=commitdiff;h=5d45a332920d6dd5536 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=55281a2c53b884d0c2b etc. > They have to be packed anyway. Not if they are naturally aligned. >>>> +}; >>>> +_Static_assert(sizeof(struct ip6hdr) == 40, "unexpected padding in ip6hdr"); >>> >>> This hurts my eyes. >> >> Really? Would it be better if it was wrapped in a nice macro? > > This one at least checks the whole struct size but the other one was > checking an offset, that one did hurt. A programming language should > provide guarantees of this kind of alignments and (packed) does exactly > this. No. "packed" guarantees that there is no *padding* between the struct members, but it does not make any guarantees about *alignments*. Even worse, "packed" structs do not need any alignment at all. You could assigned an uneven address to a "packed struct" pointer, and it is supposed to work in all cases, even on architectures like Arm, 68000 and Sparc. What is not supposed to be working is to cast this pointer to a non-packed pointer, which is what we are currently doing in SLOF. >> But your patch didn't fix all warnings yet. These are left: > > Sure, I have not tried to fix that, this is just a direction, you are > welcome to finish the task :) Or I can do that when I get time to > compile gcc9, recover my ipv6 setup for testing and try myself. Hmm, ok. Maybe I'll look into it again when I've got some spare time this week. >> lib/libnet/ipv6.c: In function ‘ip6_checksum’: >> lib/libnet/ipv6.c:452:2: warning: converting a packed ‘struct ip6hdr’ >> pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) >> may result in an unaligned pointer value [-Waddress-of-packed-member] >> 452 | pip6h = (unsigned short *) &pseudo_ip6h; >> | ^~~~~ >> In file included from lib/libnet/ipv6.c:21: >> lib/libnet/ipv6.h:86:8: note: defined here >> 86 | struct ip6hdr { >> | ^~~~~~ >> CC pc-bios/s390-ccw/ndp.o >> lib/libnet/ipv6.c: In function ‘send_ipv6’: >> lib/libnet/ipv6.c:519:8: warning: converting a packed ‘struct icmp6hdr’ >> pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) >> may result in an unaligned pointer value [-Waddress-of-packed-member] >> 519 | ip6h->pl >> 1); >> | ^~~~ >> In file included from lib/libnet/ipv6.c:22: >> lib/libnet/icmpv6.h:123:8: note: defined here >> 123 | struct icmp6hdr { >> | ^~~~~~~~ >> lib/libnet/ipv6.c:527:32: warning: taking address of packed member of >> ‘struct ip6hdr’ may result in an unaligned pointer value >> [-Waddress-of-packed-member] >> 527 | gw = ipv6_get_default_router(&ip6h->src); >> | ^~~~~~~~~~ >> lib/libnet/icmpv6.c: In function ‘handle_ra’: >> lib/libnet/icmpv6.c:171:11: warning: taking address of packed member of >> ‘struct ip6hdr’ may result in an unaligned pointer value >> [-Waddress-of-packed-member] >> 171 | rtr_ip = (ip6_addr_t *) &ip6h->src; >> | ^~~~~~~~~~~~~~~~~~~~~~~~~ >> >> At least the ip6_checksum()-related warnings can not be silenced that >> easily with your approach of passing parameters by value instead of >> pointer... > > Then use unions: > https://github.com/aik/SLOF/commit/db37446ad > > ip6_checksum() is weird btw. Yeah, code like that also forced us to compile with -fno-strict-aliasing in the past already, IIRC. >>> Or disable this particular warning. What exactly >>> does it protect against? >> If I've googled it right, assigning an unaligned pointer value is >> undefined behavior according to the C standard. So the compiler could do >> weird stuff if we simply ignore it. > > Huh. Well. There we go - no taking references of struct members. s/references of struct members/references of *packed* struct memebers/ Thomas
On Mon, Sep 23, 2019 at 08:16:21AM +0200, Thomas Huth wrote: > > Or disable this particular warning. What exactly > > does it protect against? > If I've googled it right, assigning an unaligned pointer value is > undefined behavior according to the C standard. So the compiler could do > weird stuff if we simply ignore it. It is UB indeed, see C11 6.3.2.3/7: A pointer to an object type may be converted to a pointer to a different object type. If the resulting pointer is not correctly aligned for the referenced type, the behavior is undefined. I like to point people to https://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html for how to avoid causing such silly problems in the first place. Segher
On Mon, Sep 23, 2019 at 09:29:24AM +0200, Thomas Huth wrote: > No. "packed" guarantees that there is no *padding* between the struct > members, (Except between bitfield and non-bitfield members). > but it does not make any guarantees about *alignments*. Actually, it does: it is what it does directly, everything *else* is a side-effect: 'packed' The 'packed' attribute specifies that a structure member should have the smallest possible alignment--one bit for a bit-field and one byte otherwise, unless a larger value is specified with the 'aligned' attribute. The attribute does not apply to non-member objects. > Yeah, code like that also forced us to compile with -fno-strict-aliasing > in the past already, IIRC. /me makes signs warding off evil :-/ (If you need this option, you ignored too many compiler warnings). > >>> Or disable this particular warning. What exactly > >>> does it protect against? > >> If I've googled it right, assigning an unaligned pointer value is > >> undefined behavior according to the C standard. So the compiler could do > >> weird stuff if we simply ignore it. > > > > Huh. Well. There we go - no taking references of struct members. Taking the address of one is fine. Converting it to a pointer to another object type is not. Pointer to char (or void) is fine. Segher
diff --git a/lib/libnet/icmpv6.h b/lib/libnet/icmpv6.h index 41b0c70..ba51524 100644 --- a/lib/libnet/icmpv6.h +++ b/lib/libnet/icmpv6.h @@ -130,6 +130,8 @@ struct icmp6hdr { struct router_solicitation router_solicit; struct router_advertisement ra; } icmp6body; -} __attribute((packed)); +}; +_Static_assert((long)&(((struct icmp6hdr *)NULL)->icmp6body) == 4, + "unexpected padding in icmp6hdr"); #endif diff --git a/lib/libnet/ipv6.h b/lib/libnet/ipv6.h index 7b71b50..5fb718e 100644 --- a/lib/libnet/ipv6.h +++ b/lib/libnet/ipv6.h @@ -90,7 +90,8 @@ struct ip6hdr { uint8_t hl; /**< Hop limit */ ip6_addr_t src; /**< IPv6 source address */ ip6_addr_t dst; /**< IPv6 destination address */ -} __attribute((packed)); +}; +_Static_assert(sizeof(struct ip6hdr) == 40, "unexpected padding in ip6hdr"); /** \struct packeth * Struct with pointers to headers within a packet
When compiling the libnet code with GCC 9, there are some new compiler warnings popping up now: ipv6.c: In function ‘handle_ipv6’: ipv6.c:152:21: warning: taking address of packed member of ‘struct ip6hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] 152 | if (! find_ip6addr(&(ip6->dst))) | ^~~~~~~~~~~ ipv6.c: In function ‘ip6_checksum’: ipv6.c:455:2: warning: converting a packed ‘struct ip6hdr’ pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result in an unaligned pointer value [-Waddress-of-packed-member] 455 | pip6h = (unsigned short *) &pseudo_ip6h; | ^~~~~ In file included from ipv6.c:21: ipv6.h:86:8: note: defined here 86 | struct ip6hdr { | ^~~~~~ ipv6.c:522:8: warning: converting a packed ‘struct icmp6hdr’ pointer (alignment 1) to a ‘short unsigned int’ pointer (alignment 2) may result in an unaligned pointer value [-Waddress-of-packed-member] 522 | ip6h->pl >> 1); | ^~~~ In file included from ipv6.c:22: icmpv6.h:123:8: note: defined here 123 | struct icmp6hdr { | ^~~~~~~~ etc. The entries in struct ip6hdr are naturally aligned, so we can simply drop the __attribute__ ((packed)) here and use a _Static_assert() for the correct size instead. icmp6hdr is a more complex struct since it contains a union of packed structs, but the entries before the union are naturally aligned, too, so we can silence the compiler warning by dropping the "packed" attribute from the struct icmp6hdr and just asserting that the union is at the correct offset. Signed-off-by: Thomas Huth <thuth@redhat.com> --- lib/libnet/icmpv6.h | 4 +++- lib/libnet/ipv6.h | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-)