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

[Thomas Huth]

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.

>> +};
>> +_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