patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6521, lwip doesn't compile in 64_bit...

 
 

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

patch #6521: lwip doesn't compile in 64_bit computers

Submitter:  Rishi Khan <rishi>
Submitted:  Fri 23 May 2008 07:03:50 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  kieranm Open/Closed:  Closed
Planned Release:  1.3.1

Jump to the original submission

Thu 25 Jun 2009 12:57:59 PM UTC, comment #12: 

I've checked this on x86_64 using the unix port and it seems to compile.  There were a couple of warnings from the unix port itself, but I've fixed those, and the core lwIP code showed no problems.


Kieran Mansley <kieranm>
Group Member
Fri 01 May 2009 12:29:38 PM UTC, comment #11: 

I should be able to test that the unix port compiles on an x86_64

Kieran Mansley <kieranm>
Group Member
Tue 21 Apr 2009 06:46:24 PM UTC, comment #10: 

I really need some input on this to know whether it is done or not...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Apr 2009 05:44:22 PM UTC, comment #9: 

As I don't have a 64-bit target, unfortunately, I can only compile the stack using the win32 and unix port and check for 64-bit compatibility warnings. I could try to get gcc & make running on my mac, though, never tried that...

Anyway, I guess the formatters are mostly correct now, but there might be other issues. I'd like to leave this open until someone runs the stack successfully on a 64-bit system with all options on.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 03 Apr 2009 11:14:58 AM UTC, comment #8: 

Can anyone comment on the status of this?

Kieran Mansley <kieranm>
Group Member
Wed 18 Feb 2009 09:16:32 PM UTC, comment #7: 

If we're concerned about portability with the use of %d for u8_t we could always explicitly cast the argument to an 'int'.

C99 says you can use %zd for something of type size_t, but since it's not in C90 I wouldn't recommend it. That implies a new define (perhaps defaulting to %zd though?).

On GCC, format strings are checked if using -Wformat (which is implied by -Wall). If porters are using printf() and friends, then this checking would just happen with those arguments. For any other function, their prototype for it would need to include _attribute_((format(....))). Essentially it's a port issue.

Jonathan Larmour <jifl>
Group Member
Wed 18 Feb 2009 09:15:08 PM UTC, comment #6: 

I've worked my way through some of the files (only 3 yet:). I hope I didn't make it worse, but I think it's quite straightforward.

Added SZT_F in cc.h for size_t (that will have to go in to new ports; to prevent breaking ports I defined it in arch.h for now).

Comments are welcome!

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Feb 2009 08:31:41 PM UTC, comment #5: 

Oops, it appears Jared posted a comment on the lwip-devel mailing list rather than here, in comments to this patch.
Anyway, I agree with Jared:
- %d is good for u8_t: ANSI C requires that u8_t is converted to int when passed to printf and friends
- size_t needs a new define: I haven't dug deep into that, but it sounds like a safe option
As for automated checking of format string consistency, shouldn't lint-like tools do this? Splint, maybe? And I know for a fact that the RealView compiler for ARM emits warnings in these cases.

- mike

Mike Kleshov <kleshov>
Group Member
Wed 18 Feb 2009 07:48:49 PM UTC, comment #4: 

Looking through this, what's the suggested format string for
a) size_t
b) u8_t
? I guess for size_t we need a new define. Is %d OK for u8_t?

Apart from that, I've had a look through the patch: most of the changes are just wrong, but there are many places that need attention. I'm in the middle of this but it will take some days until I find time for it again.

Oh, and does anybody know if there any good compiler setting to warn me about wrong format arguments? I don't want to search the whole code...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 12 Jun 2008 07:12:35 PM UTC, comment #3: 

I'll see what I can do.

Rishi Khan <rishi>
Fri 30 May 2008 12:27:32 PM UTC, comment #2: 

I agree this isn't really usable in the current state, but would welcome something that fixed it to address Jonathan's comments.

Kieran Mansley <kieranm>
Group Member
Mon 26 May 2008 01:06:20 AM UTC, comment #1: 

Thanks for the patch.

I see what you are trying to do, but a global search and replace of %d with %"S_32F" is not right. You say they should be u32_t or s32_t but that's not true. We don't need to lay down a size for most of these, and for many of the user-supplied data with the sockets interface (such as socket descriptors) we must stay compatible with the well-known standard types of the BSD interface. Arguments declared as int should stay with %d in debug output, and so on for similar arguments. It is only arguments of u16_t or s16_t etc. which should be changed if they need to be.

Also this change in lwip_select:
-  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_select(%d, %p, %p, %p, tvsec=%ld tvusec=%ld)\n",
+  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_select(%"S32_F", %p, %p, %p, tvsec=%lu tvusec=%lu)\n",
                   maxfdp1, (void )readset, (void ) writeset, (void *) exceptset,
-                  timeout ? timeout->tv_sec : -1L, timeout ? timeout->tv_usec : -1L));
+                  timeout ? timeout->tv_sec : -1, timeout ? timeout->tv_usec : -1));

The second change won't be right (although what was there before wasn't perfect either). Telling it -1 is an unsigned value won't be good. And on some platforms where sizeof(int) != sizeof(long) and struct timeval's tv fields aren't longs, the stack could get mangled. It should probably instead stay with %ld format specifiers and use this for args:
 timeout ? (long)timeout->tv_sec : -1L, timeout ? (long)timeout->tv_usec : -1L)
This allows it to adapt somewhat to systems which set LWIP_TIMEVAL_PRIVATE to 0.

Your changes to _IOR and _IOW aren't really the right answer, although long wasn't necessarily ideal either: they should probably be u32_t casts to explicitly guarantee the bitfield is large enough (consider targets with 16-bit ints).

If you could rework the patch to address these issues, that would be great, thanks!

Jonathan Larmour <jifl>
Group Member
Fri 23 May 2008 07:03:50 PM UTC, original submission:  

I edited many files (mostly sockets.c) to fix the long/int problem. These really should be u32_t and s32_t. There were many places in sockets.c that used "%d" instead of "%"S_32F. I fixed all of these. I also edited all int calls to s32_t or u32_t (for unsigned int).

Just a note, I fixed everything BUT PPP. There are many declarations of int's and many "%d" calls to LWIP_DEBUG. I don't use PPP, so I left these alone.


lwip now should compile on 64-bit platforms with PPP_SUPPORT off. (I didn't break PPP support, I just didn't fix it for the 64_bit cases).

Rishi Khan <rishi>

 

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

Attached Files
file #15718:  64_bit.patch added by rishi (31KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kleshov (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by rishi (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-06-25 kieranm StatusNeed Info Done
        Open/ClosedOpen Closed
    2009-05-01 kieranm Assigned togoldsimon kieranm
    2009-04-21 goldsimon StatusIn Progress Need Info
    2009-04-03 kieranm Planned ReleaseNone 1.3.1
    2009-02-18 goldsimon Assigned toNone goldsimon
    2009-02-18 goldsimon StatusNone In Progress
    2008-05-23 rishi Attached File- Added 64_bit.patch, #15718

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code