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:33:33 +0200
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)

This line:
printf ("tm.b = 0x%x\n", (unsigned long)tm.b);
Would most likely cause a crash.

The problem is that if the address of tm.a is for example 0x1000, then the address of tm.b will be 0x1001. If you try to perform a 32bit read or write operation on tm.b then it will crash if the CPU does not support unaligned accesses.

If you leave out the packed attribute, then the compiler will insert three dummy bytes between tm.a and tm.b to solve the unaligned access problem. This is of course not an option in the lwip case since we use structs to describe packet headers.

Regards,
Timmy Brolin

Pedro Alves wrote:

Timmy Brolin wrote:

Curt McDowell wrote:

Pedro Alves wrote
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.


memcpy may still be needed because hdr->sipaddr is misaligned at +2 due to the format of the ARP packet. My theory is that struct ip_addr2 was originally invented because the 4-byte struct/uint32 copy was crashing on someone's 2-byte
aligned CPU!

Regards,
Curt McDowell
Broadcom Corp.

Either ip_addr2 or memcpy is needed.
The code "sipaddr = hdr->sipaddr;" would crash on any CPU which do not support unaligned accesses.


huh? Not if struct ip_addr is NOT packed, hdr is an instance of a packed struct, and sipaddr is a struct ip_addr, not struct ip_addr2.

Are you saying that:


#include <stdio.h>

struct test_me
{
  unsigned char a;
  unsigned long b;
  unsigned char c;
} __attribute__ ((packed));

int main(int argc, char** argv)
{
 volatile char dummy_offset_alignment_for_next_var;
 volatile struct test_me tm;
 volatile unsigned long i;
 volatile unsigned long i2;

 tm.a = 0xdd;
 tm.c = 0xee;
 i = 0x55ffaa33;
 tm.b = i;
 i2 = tm.b;

 printf("sizeof(struct test_me) = 0x%x\n", sizeof(struct test_me));
 printf ("tm.a = 0x%x\n", (unsigned long)tm.a);
 printf ("tm.b = 0x%x\n", (unsigned long)tm.b);
 printf ("tm.c = 0x%x\n", (unsigned long)tm.c);
 printf ("i2 = 0x%x\n", i2);

 return 0;
}

Won't work as expected? That would beat the whole purpose of __attribute__((packed)), no?

P.S.: No use testing in x86, as that supports unaligned accesses anyway.

Cheers,
Pedro Alves

The current ip_addr2 solution is technically more efficient than memcpy since it copies the ip_addr with just two 16bit reads+writes. The only solution which would be slightly more efficient is to copy hdr->sipaddr to a 32bit register using two 16bit accesses, and then write the 32bit register to sipaddr. But since this is not a critical code path, it is more important to keep the code as clean as possible rather than trying to save one single memory access.
The ip_addr2 solution makes the code nice and clean.

Regards,
Timmy Brolin




_______________________________________________
lwip-users mailing list
address@hidden
http://lists.nongnu.org/mailman/listinfo/lwip-users







reply via email to

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