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

[Pedro Alves]

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