Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Samuel Thibault]

Richard Henderson, on mar. 09 janv. 2018 08:33:22 -0800, wrote:
> On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
> > Host: Mac OS 10.12.5
> > Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> > 
> >   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' 
> > of class or
> >         structure 'ip6' may result in an unaligned pointer value
> >         [-Waddress-of-packed-member]
> >       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
> >                                  ^~~~~~~~~~
> >   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 
> > 'IN6_IS_ADDR_MULTICAST'
> >   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
> 
> The fact that you replace a macro with a function of exactly the same code in
> order to avoid a diagnostic sure looks like a compiler bug to me.

The compiler could be smarter to realize that it's actually a byte access 
indeed.

There are two things for me:

- The OS-provided implementation of IN6_IS_ADDR_MULTICAST could be doing
  32bit accesses and whatnot, so we are not supposed to use it on packed
  fields.

- With the proposal patch, the compiler could still emit the warning,
  since we are basically still passing the address of the packed field
  to a function which the compiler might not see either.  We are however
  sure that the function makes an aligned access.

So I'd say we should still rather do it to be on the safe side. With
Thomas Huth's comments applied.

BTW,

Thomas Huth <address@hidden> wrote:
> I think this might also be a bug in the macro definitions.

> > > #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)

> On Linux, it is defined like this:
> 
> #define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)

Well, the standard says that IN6_IS_ADDR_MULTICAST takes a struct
in6_addr*, so it can use the s6_addr field.

Samuel