lwip-users
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Timmy Brolin
Subject: Re: [lwip-users] [PATCH] fix warning for gcc and possible unaligned access
Date: Mon, 24 Apr 2006 19:24:07 +0200
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)

Leon Woestenberg wrote:

Hello all,

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.


I think Curt's idea looks good / best. Do I summarize the actions to be taken OK as follows?

- remove packing from "struct ip_addr" globally.
- Byte by byte memory copies (using memcpy() or inline for loop) from packet to/from structure.
- Get rid of ip_addr2.

Regards,

Leon.

I agree that struct ip_addr should not be packed, but I don't see why memcpy is a better solution than ip_addr2?
In my opinion this:

*(struct ip_addr2 *)&sipaddr = hdr->sipaddr;

Is easier to read than this:

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

Add to that the fact that memcpy is less efficient than the ip_addr2 solution.
"Don't fix it if it ain't broken"?

Regards,
Timmy Brolin





reply via email to

[Prev in Thread] Current Thread [Next in Thread]