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

[Pedro Alves]

This is me <address@hidden>, no access to the other mail from here.

Curt McDowell wrote:
> 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.
>
>   
Right!
Bet you were casting from u32_t to an unaligned struct ip_addr, no?

Seems like the right thing to do.
If the packing is removed from struct ip_addr, and struct ip_addr2 is 
removed, then there is no need to memcpy anymore.

A simple:

sipaddr = hdr->sipaddr;

will do.

And this should also trigger some optimizations that weren't possible because 
of the packing,
saving a few (not many) .text bytes.

I can only look at this again next week, I will provide a patch then if noone 
does it before.

Cheers,
Pedro Alves

> 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
> >
> >     
>
>
>
>
>
> _______________________________________________
> lwip-users mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/lwip-users
>
>