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: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] 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: > > > >> [EMAIL PROTECTED] 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 > [email protected] > http://lists.nongnu.org/mailman/listinfo/lwip-users > _______________________________________________ lwip-users mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/lwip-users
