patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5846, struct sockaddr_in has different...

 
 

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

patch #5846: struct sockaddr_in has different alignment than struct sockaddr

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Mon 02 Apr 2007 08:32:36 PM UTC
   
 
Category:  None Priority:  3 - Low
Status:  Wont Do Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Wed 11 Apr 2007 07:00:06 PM UTC, comment #8: 

OK, you convinced me not to use -Wcast-align on lwIP.

Closing this as 'Wont Do'.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 07 Apr 2007 08:20:09 PM UTC, comment #7: 

The reason I was concerned about unnecessary overhead (extra code and/or extra footprint) was because you had said earlier in the comment to which I replied: "In other words, I think if the one is packed, the other should be packed, also."

I was also extrapolating a bit - in your patch you, for example, align the struct sockaddr to 64-bits for MEM_ALIGNMENT==8, but MEM_ALIGNMENT has nothing to do with the alignment requirements of a struct in_addr which might only need alignment to 1, 2, or 4 bytes. So to guarantee they'd match to avoid any warning when the cast is made, since it happens both ways, you'd also need to add some alignment component to struct sockaddr_in. With that patch's approach anyway. The patch as it stands isn't right.

Anyway, the warning is irrelevant - as you say yourself, a struct sockaddr is never deferenced in itself. There is not, and will not be, any problem here - people don't create struct sockaddrs.

Although if/when someone finishes off ipv6, there'll be an AF_INET6 class to distinguish and a struct sockaddr_in6 will probably have different alignment constraints to a struct sockaddr_in.

Other stacks don't bother with this anyway: warnings, especially those generated by obscure warning flags, are guides, not problems to be fixed. If others think this really is a "problem", then the solution will have to involve unions.

Jonathan Larmour <jifl>
Group Member
Sat 07 Apr 2007 09:10:17 AM UTC, comment #6: 

OK, I can understand it's not a nice solution I brought up. What I can't understand is why aligning the struct sockaddr would slow down lwIP. It's just used for pointers anyway, its members are never accessed. Pushing around an aligned pointer is not different from an unaligned, is it?

After all, I think the warning is good, as those alignment faults (if they appear) are not easy to debug. And on changing the code, such a bug could easily appear, since some platforms support unaligned access and some don't.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 07 Apr 2007 03:26:23 AM UTC, comment #5: 

So you're building with -Wcast-align. That's your problem. lwIP is doing nothing wrong at all, and I would be against slowing done lwIP to pacify some warning about something that isn't a problem.

I believe in general that GCC has a principle that -Wall contains warnings that should not trigger for valid code, unless that valid code can be adjusted without a speed penalty. Other warnings may be different, and -Wcast-align is in that category.


Jonathan Larmour <jifl>
Group Member
Fri 06 Apr 2007 11:01:06 AM UTC, comment #4: 

Example of code that causes the warning:
lwip_bind(int s, struct sockaddr *name, socklen_t namelen)
{
...
  local_addr.addr = ((struct sockaddr_in *)name)->sin_addr.s_addr;

other places (always 'cast increases required alignment of target type'):
In function `lwip_bind':
lines 230, 232, 233
In function `lwip_connect':
lines 290, 292, 299, 300
In function `lwip_sendto':
lines  527, 532, 533

I'm using gcc version 3.4.1 (Altera Nios II 5.1 b93). The NIOS-II processor has 4 byte alignment requirement (much like ARM processors) and I have the following gcc options (some of them are for my C++ code:
-Wall
-ggdb
-fno-exceptions
-Wextra
-fverbose-asm
-Wparentheses
-Wswitch
-Wno-unused-parameter
-Wfloat-equal
-Wshadow
-Wcast-align
-Wno-unused-function
-Werror
-O0
-mhw-mul
-mno-hw-mulx
-mno-hw-div

Please tell me if you know a better way to remove this warning (personally, I can understnad the compiler complaining about the cast :)

Thanks,
Simon

Simon Goldschmidt <goldsimon>
Group administrator
Thu 05 Apr 2007 01:30:34 PM UTC, comment #3: 

This doesn't seem right. No-one should be declaring a "struct sockaddr". This means that we only ever declare sockaddr_in's, and cast them to sockaddr's, and that means the alignment is always fine.

If there is a compiler generating a warning for this, that's unfortunate, but we should not increase footprint or complexity just to silence a warning.

What code is causing the warning?

Jonathan Larmour <jifl>
Group Member
Tue 03 Apr 2007 01:48:16 PM UTC, comment #2: 

It's the whole struct which is unaligned. Casting a pointer from sockaddr_in to sockaddr leads to a warning. In Linux, it's the way we have it, but in my opinion, this would also lead to a warning, wouldn't it?

Using packed is not that good, I think: The struct is accessed byte by byte (using GCC) although that's not necessary, because sockaddr is only used to pass over sockaddr_in structs in lwIP. In other words, I think if the one is packed, the other should be packed, also.

I know what I submitted does not look nice and I'm open to any other solution, as long as I get rid of the warning!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 03 Apr 2007 01:42:19 PM UTC, comment #1: 

Is the problem the layout of the fields within the structure, or how an instance of the structure itself is aligned?

In general I prefer to rely on the compiler to sort out things like this (by e.g. using the "packed" directive to gcc).

Given that these types should be similar to those used on BSD, Linux, etc, how are they defined there to avoid the same problem?  Could we do something similar?


Kieran Mansley <kieranm>
Group Member
Mon 02 Apr 2007 08:32:36 PM UTC, original submission:  

Because struct sockaddr only includes u8_t values, it is byte-aligned, while struct sockaddr_in is 4-byte-aligned (because of in_addr).

In the attached patch, I've solved this by including different 'alignment' members in struct sockaddr depending on MEM_ALIGNMENT. Another (less cleaner) solution would be to just integrate a union including a struct ip_addr, this way alignment would always be the same.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #12381:  sockets_alignment.patch added by goldsimon (724B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-04-11 goldsimon StatusIn Progress Wont Do
        Open/ClosedOpen Closed
    2007-04-02 goldsimon Attached File- Added sockets_alignment.patch, #12381

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code