[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it ins
From: |
Samuel Thibault |
Subject: |
Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST() |
Date: |
Tue, 9 Jan 2018 22:18:28 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
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
- Re: [Qemu-devel] [PATCH 01/12] slirp: remove QEMU_PACKED from structures with don't require it, (continued)
- [Qemu-devel] [PATCH 02/12] slirp: struct icmp/ethhdr ARE packed, Philippe Mathieu-Daudé, 2018/01/08
- [Qemu-devel] [PATCH 03/12] slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero(), Philippe Mathieu-Daudé, 2018/01/08
- [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST(), Philippe Mathieu-Daudé, 2018/01/08
- [Qemu-devel] [PATCH 05/12] slirp: poison IN6_*_ADDR_*() macros to avoid them, Philippe Mathieu-Daudé, 2018/01/08
- [Qemu-devel] [PATCH 06/12] slirp: remove unused header, Philippe Mathieu-Daudé, 2018/01/08
- [Qemu-devel] [PATCH 07/12] slirp: remove unnecessary, Philippe Mathieu-Daudé, 2018/01/08
- [Qemu-devel] [PATCH 08/12] slirp: removed unused code, Philippe Mathieu-Daudé, 2018/01/08