[Top][All Lists]

[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: Pedro Alves
Subject: Re: [lwip-users] [PATCH] fix warning for gcc and possible unaligned access
Date: Fri, 21 Apr 2006 23:31:42 +0100
User-agent: Thunderbird 1.5 (Windows/20051201)

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.

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.

Pedro Alves

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:

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. */
  do { \
    union ip_add_pun { \
      struct ip_addr ip; \
      struct ip_addr2 ip2; \
    } d; \
    d.ip = (FROM); \
    (TO) = d.ip2; \
  } while (0)

  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?

Pedro Alves

lwip-users mailing list

lwip-users mailing list

reply via email to

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