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

[Thomas Huth]

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 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?

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