[SLOF] [PATCH] libnet: Fix compiler warnings with GCC 9

[Alexey Kardashevskiy]

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 at 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.



-- 
Alexey