RE: [lwip-users] [PATCH] fix warning for gcc and possible unaligned access

[Curt McDowell]

struct ip_addr2 is only used as a hack in etharp.c and should be removed.
etharp_arp_input() should use simple memcpy()s to copy the struct ip_addr,
because that code isn't in a critical path.

  memcpy(&sipaddr, &hdr->sipaddr, sizeof(sipaddr));
  memcpy(&dipaddr, &hdr->dipaddr, sizeof(dipaddr));

The following comment in etharp_arp_input() is wrong:

  /* these are aligned properly, whereas the ARP header fields might not be */
  struct ip_addr sipaddr, dipaddr;

Because struct ip_addr is packed, sipaddr and dipaddr have an alignment
requirement of 1.  This booby trap results in really erratic behavior, say if
the "u8_t i" is moved up.  I was using struct ip_addr in my code and this
problem cost me a few hours.  struct ip_addr should not be packed.  Only the
packet format structures that contain it need to be packed.

Regards,
Curt McDowell
Broadcom Corp.

> -----Original Message-----
> From: address@hidden 
> [mailto:address@hidden On 
> Behalf Of Pedro Alves
> Sent: Friday, April 21, 2006 3:22 AM
> To: Mailing list for lwIP users
> Subject: Re: [lwip-users] [PATCH] fix warning for gcc and 
> possible unaligned access
> 
> Timmy Brolin wrote:
> 
> > Pedro Alves wrote:
> >
> >> address@hidden wrote:
> >>
> >>> Hi,
> >>> Why not just replace the lines
> >>>
> >>>     *(struct ip_addr2 *)&sipaddr = hdr->sipaddr;
> >>>     *(struct ip_addr2 *)&dipaddr = hdr->dipaddr; with
> >>>     sipaddr = *(struct ip_addr *)&hdr->sipaddr;
> >>>     dipaddr = *(struct ip_addr *)&hdr->dipaddr;
> >>
> >>
> >> Because if hdr->sipaddr is unaligned and the architecture doesn't 
> >> support unaligned accesses, like many RISCs do, the result 
> is undefined.
> >
> >
> > Are you sure that it can become unaligned?
> > There is code in place in lwip to guarantee header alignment...
> 
> Sure they can, that's why the copying is beeing made.
> 
> I missed the fact that ip_addr and ip_addr2 are both pack_struct'ed.
> The first time I looked I got the impression that struct 
> ip_addr2 was packed and struct ip_addr wasn't.
> 
> Beach's suggestion removes the gcc warning and is used 
> throughout lwip, but is it really safe?
> 
> -*(struct ip_addr2 *)&sipaddr = hdr->sipaddr; -*(struct 
> ip_addr2 *)&dipaddr = hdr->dipaddr;
> +sipaddr = *(struct ip_addr *)&hdr->sipaddr; dipaddr = 
> *(struct ip_addr 
> +*)&hdr->dipaddr;
> 
> The only safe way I've heard that ensures correct conversion 
> is using unions like this:
> 
> /* These tell the
> * compiler that the type punning between the two pointer 
> classes needs to
> * be taken into account when optimizing. 
> */
> #define IP_ADDR_TO_IP_ADDR2(FROM, TO) \
>   do { \
>     union ip_add_pun { \
>       struct ip_addr ip; \
>       struct ip_addr2 ip2; \
>     } d; \
>     d.ip = (FROM); \
>     (TO) = d.ip2; \
>   } while (0)
> 
> #define IP_ADDR2_TO_IP_ADDR(FROM, TO) \
>   do { \
>     union ip_add_pun { \
>       struct ip_addr ip; \
>       struct ip_addr2 ip2; \
>     } d; \
>     d.ip2 = (FROM); \
>     (TO) = d.ip; \
>   } while (0)
> 
>   IP_ADDR2_TO_IP_ADDR(hdr->sipaddr, sipaddr);
>   IP_ADDR2_TO_IP_ADDR(hdr->dipaddr, dipaddr);
> 
> Can anyone comment on why we need struct ip_addr and struct ip_addr2?
> 
> Cheers,
> Pedro Alves
> 
> 
> 
> _______________________________________________
> lwip-users mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/lwip-users
>