buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26273, etharp.c : fix alignment issue in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #26273: etharp.c : fix alignment issue in etharp_raw

Submitted by:  Guillaume du PONTAVICE <gdupontavice>
Submitted on:  Tue 21 Apr 2009 04:49:48 PM UTC  
 
Category:  ARP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

( Jump to the original submission)

Thu 23 Apr 2009 05:39:36 PM UTC, comment #16: 

This is the right one:

Since this is definitively not our bug, I'm closing it as invalid.

As to struct copying: I think the current implementation is good enough, but
we could discuss about checking where we use it (especially regarding pointer
casting).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 23 Apr 2009 12:27:28 PM UTC, comment #15: 

Simon, I agree with you and was a bit appalled that GCC is using memcpy to copy just a 4 byte struct.

Bill Auerbach <billauerbach>
Thu 23 Apr 2009 08:38:05 AM UTC, comment #14: 

ok guys,
I checked with ST, I was not using the latest compiler.
this was a known issue, unfortunately compiler release are not public & are only given by ST support.

with the latest compiler, the assembly output looks like this

..1463: -- Line 1080 (lwip/src/netif/etharp.c)
ldl 11 -- ipsrc_addr
ldl 2 -- hdr
ldnlp 7
ldc 4
move
..1464: -- Line 1081 (lwip/src/netif/etharp.c)
ldl 13 -- ipdst_addr
ldl 2 -- hdr
adc 38
ldc 4
move

move : This interprets AREG as an unsigned integer representing the number of bytes to be transferred, BREG as the destination address, and CREG as the source address.

everything is working fine now.

anyway I agree that compiler behaviour on struct assignement shall be reviewed as well, to minimize memcpy call overhead. (even if it looks like ST20 has a specific opcode to perform memcpy, it could be useful for other target/compiler)

thanks for your support, guys !

Guillaume du PONTAVICE <gdupontavice>
Thu 23 Apr 2009 05:46:14 AM UTC, comment #13: 

> Don't change the code to work around compiler faults, but do change to code for less ambiguous C.


I'm getting the opinion assigning structs isn't that portable, so maybe we should remove that code...

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka3934.html


That link talks about how the ARM compiler optimizes memcpy. However in this case it produced a memcpy out of a structure assignment. That's different thing. However, I don't know how structure assignments are usually compiled, but I would have thought there is a similar algorithm (like copying members for small structs, using memcpy for large structs).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 22 Apr 2009 09:24:52 PM UTC, comment #12: 

Whether or not the compiler chooses to do inline copying or a call to memcpy for structures is presumably a heuristic of the compiler.

For example, the ARM tools will look for structures less than 64 bytes and a multiple of 4 long and that are word aligned and convert to load/store multiple:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka3934.html

Don't change the code to work around compiler faults, but do change to code for less ambiguous C.

Robert Sprowson <sprow>
Wed 22 Apr 2009 08:30:14 PM UTC, comment #11: 

>would it be possible to use the SMEMCPY alternative ?


The following will work for you and might be more efficient than memcpy (SMEMCPY).

  hdr->sipaddr.addrw[0] = ((struct ip_addr2 *)ipsrc_addr)->addrw[0];
  hdr->sipaddr.addrw[1] = ((struct ip_addr2 *)ipsrc_addr)->addrw[1];
  hdr->dipaddr.addrw[0] = ((struct ip_addr2 *)ipdst_addr)->addrw[0];
  hdr->dipaddr.addrw[1] = ((struct ip_addr2 *)ipdst_addr)->addrw[1];

Because my compiler cannot determine the alignment it generated 2 calls to memcpy to copy these 8 bytes.  Because of this, maybe the code above is better for all?

Simon, is the original code bad because it can (does in my case) make a memcpy function call?

I agree about changes to get around compiler bugs, but there's a lot to be said about coding something simpler to avoid an unknown way the compiler will handle something.  This could mean that all struct assignments are converted to memcpy calls.  This is something I don't want to see - the runtime overhead is enormous.

I'm inclined now to agree with you and Simon that the ST compiler is wrong - it should have done a byte copy or called memcpy.  This bug for ST might only present itself with packing enabled though (not that that helps you).

Should a task be considered for removing struct copies?  Not for the ST compiler, but because of the memcpy calls that might (or might not) result?

Bill Auerbach <billauerbach>
Wed 22 Apr 2009 07:05:24 PM UTC, comment #10: 

> Simon, the point is that a compiler won't do that (my example) correctly. It's wrong on some platforms!


I agree with you there. However, this is not the point in this bug. As I said, it's the other way round here and alignment is correct (as you can see in the disassembly Guillaume posted).

In comment #6 you say it's not a compiler bug. Maybe I'm dumb, but I still fail to see what that explanation (your example) has to do with this bug?

I can't speak for the rest of the lwIP developers, but if what we are doing there is a non-portable construct, I'm willing to change it - if it's a compiler bug, I'd rather not.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 22 Apr 2009 05:16:18 PM UTC, comment #9: 

> then either sipaddr or dipaddr is not four-byte aligned (MAC addresses are 6-byte long). The question appears, how the statement
>
>hdr->dipaddr = hdr->sipaddr;
>
>(struct to struct copy) is translated by a compiler.


Because there is no cast! The compiler has all the information it needs to generate correct code.  It knows the base of the struct is aligned and that a sub-structure isn’t (because of packing).

A cast tells the compiler "I know what I’m doing".  Therefore, the code with the cast had better ensure alignment, because the compiler will not.

Bill

Bill Auerbach <billauerbach>
Wed 22 Apr 2009 05:11:05 PM UTC, comment #8: 

Simon, the point is that a compiler won't do that (my example) correctly.  It's wrong on some platforms! My point is, you can cast addresses to pointers and violate alignment constraints. The compiler can do nothing about that.

Bill Auerbach <billauerbach>
Wed 22 Apr 2009 04:06:09 PM UTC, comment #7: 

> The compiler assumes all pointers to structs are properly aligned.


If the struct is inside another packed struct, the compiler should not assume proper alignment but cope with any alignment!

> I disagree that what you're seeing is a compiler bug.


Your example is the other way round: the code in question does not cast from 2-byte-aligned to 4-byte-aligned but from 4-byte-aligned to 2-byte-aligned. In your example this would be:

u32_t buffer[10];
u8_t *ptr, i;

ptr = (u8_t *) &buffer[1];
i = *ptr;

Any compiler should doe that porperly! (Only our code in question is using structs, not u8/u32_t, but I don't see why that should make a difference)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 22 Apr 2009 01:50:40 PM UTC, comment #6: 

(Posted to lwip-devel - including it here now)

The compiler assumes all pointers to structs are properly aligned.  All local, static, and global struct allocations are going to be properly aligned.  They would not be aligned when casting to pointers to those structs.  Hence, pointer casting isn't portable or guaranteed to always work.

It would be curious to see how it is handled if a union were used and the casting removed.  I expect then with padding turned off that the compiler might do the right thing.

-----------------------------------------------

I disagree that what you're seeing is a compiler bug.

For example:

u8_t buffer[10];
u32_t *ptr, i;

ptr = (u32_t *) &buffer[1];
i = *ptr;

This will fail in the same way.  It's an implicit copy of a u32_t - but it's not a compiler bug.

Bill

Bill Auerbach <billauerbach>
Wed 22 Apr 2009 10:48:47 AM UTC, comment #5: 

> would it be possible to use the SMEMCPY alternative?


I'm afraid that normally, our attitued towards compiler bugs is: don't use bad code to work around them! In this, case, a function call instead of load/store is bad code to me! However, since we already do that in other places in etharp.c (in fact, the whole struct ip_addr2 seems to be a compiler-bug-workaround), I could live with it.

The only question is whether this is the only line in the code that does not work with that compiler, and I must say I'm not willing to adjust the complete lwIP stack (there are many instances of unaligned, packed structs). For example, what about the IP addresses in ethernet/IP packets? Since the ethernet header is 14 bytes, thus the IP addresses are always unaligned (but packed): does that work for you?

Couldn't it be that there exists another packing setting for that compiler which would generate correct code?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 22 Apr 2009 08:42:14 AM UTC, comment #4: 

ok here is the assembly dump

ldl n = (Load local) Load the value from n words above WPTR.
ldnlp n  = (Load non-local pointer) Load the value AREG + 4n.
dup = duplicate top of stack
stnl n = Store non-local Store a value to n words above AREG.
rev = reverse AREG & BREG value
adc n = add constant to AREG

-- Line 1081 (lwip/src/netif/etharp.c)
..2719: -- Line 1081 (lwip/src/netif/etharp.c)
ldl 5 -- hdr
ldnlp 7
dup
ldl 12 -- ipsrc_addr
ldnl 0
rev
stnl 0
-- Line 1082 (lwip/src/netif/etharp.c)
..2720: -- Line 1082 (lwip/src/netif/etharp.c)
ldl 5 -- hdr
adc 38
dup
ldl 14 -- ipdst_addr
ldnl 0
rev
stnl 0

as we can see, ipsrc_addr field is word aligned (instruction ldnlp 7 : offset = 7  words = 28 bytes) a load word / store word operation is used.

ipdst_addr field is not word aligned (instruction adc 38 : offset = 38 bytes), but the compiler still uses a load word / store word operation.

so it is a compiler bug.
I will report a bug to ST but unfortunately I doubt I can get a compiler fix in a short term ....

would it be possible to use the SMEMCPY alternative ?

Guillaume du PONTAVICE <gdupontavice>
Wed 22 Apr 2009 07:19:58 AM UTC, comment #3: 

> I will have a look at the assembly dump tomorrow to confirm this.


That would be great. If so, it would be a bug in the compiler, though.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 21 Apr 2009 09:45:28 PM UTC, comment #2: 

well, in my case
the beginning of the ARP packet is word aligned & hdr->sipaddr & hdr->dipaddr are two bytes aligned (not 4 bytes aligned), as their offset in an ARP packet is only two bytes aligned.

when writing on a non aligned address on ST20 chipset, no exception occurs, but this may result in corruption of adjacent memory.

I guess the ST compiler try to optimize the cast & then generate a load word / store word instruction instead of two load short / store short instructions.
I will have a look at the assembly dump tomorrow to confirm this.

Guillaume du PONTAVICE <gdupontavice>
Tue 21 Apr 2009 05:55:40 PM UTC, comment #1: 

I guess you have copied those lines from autoip.c:419 or etharp.c:667 ? In these places, it is understandable to use memcpy, since we copy from a 2-byte-aligned struct to a 4-byte-aligned.

However, in your patch it is the other way round: a 4-byte-aligned struct is casted to a 2-byte-aligned (which is in turn copied). How could that fail?

I don't have that strong an opinion against your patch, (since we already do it that way in other places), only, I would like to understand why it fails before changing it.

Someone know the answer?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 21 Apr 2009 04:49:48 PM UTC, original submission:  

I am working on ST5100 chipset (ST20 core / OS20, using ST toolset ST20 2.3.1, using structure packing option -falign1 for etharp.c)
unfortunately the compiler seems not being able to correctly handle the following affectations in etharp.c (function etharp_raw)

hdr->sipaddr = (struct ip_addr2 )ipsrc_addr;
hdr->dipaddr = (struct ip_addr2 )ipdst_addr;

the result is : the MAC address which is located just after the IP in the ARP header got corrupted :

ffffffffffff
001914c5f0b4
0806
0001
0800
0604
0001
001914c5f0b4   <= sender MAC
c0a800bc       <= sender IP
00000000c0a8   <= destination MAC (corrupted)
00eb0000       <= destination IP  (corrupted)

using the patch hereafter, everything is fine, I got the following ARP packet

ffffffffffff
001914c5f0b4
0806
0001
0800
0604
0001
001914c5f0b4   <= sender MAC
c0a800bc       <= sender IP

                        1.   <= destination MAC

c0a800eb       <= destination IP

Index: etharp.c
===================================================================
RCS file: /sources/lwip/lwip/src/netif/etharp.c,v
retrieving revision 1.150
diff -u -r1.150 etharp.c
--- etharp.c 18 Feb 2009 16:48:30 -0000 1.150
+++ etharp.c 21 Apr 2009 16:09:24 -0000
@@ -1077,8 +1077,8 @@
 #endif /* LWIP_AUTOIP */
     hdr->ethhdr.src.addr[k]  = ethsrc_addr->addr[k];
   }
-  hdr->sipaddr = (struct ip_addr2 )ipsrc_addr;
-  hdr->dipaddr = (struct ip_addr2 )ipdst_addr;
+  SMEMCPY(&hdr->sipaddr,ipsrc_addr,sizeof(hdr->sipaddr));
+  SMEMCPY(&hdr->dipaddr,ipdst_addr,sizeof(hdr->dipaddr));

   hdr->hwtype = htons(HWTYPE_ETHERNET);
   hdr->proto = htons(ETHTYPE_IP);

Guillaume du PONTAVICE <gdupontavice>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sprow (Posted a comment)
  • -email is unavailable- added by billauerbach (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by gdupontavice (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-04-23 goldsimon StatusNeed Info => Invalid
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2009-04-21 goldsimon StatusNone => Need Info

    Back to the top


    Powered by Savane 3.5