patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6699, Fixing a couple of compilation...

 
 

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

patch #6699: Fixing a couple of compilation warnings(Paradigm C++)

Submitted by:  Tai-hwa Liang <atliang>
Submitted on:  Fri 19 Dec 2008 10:15:23 AM UTC  
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Simon Goldschmidt <goldsimon> Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Thu 26 Feb 2009 02:40:45 AM UTC, comment #16: 

Adding yet another patch to fix possible building warnings if users use IN_CLASS[ABCD]_{HOST,NET} directly.

(file #17535)

Tai-hwa Liang <atliang>
Mon 16 Feb 2009 07:38:47 PM UTC, comment #15: 

I just converted the length parameters to u32_t, so this should be fixed.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 12 Feb 2009 05:58:16 PM UTC, comment #14: 

> The type of the len field in struct api_msg_msg and the write
> offset in struct netconn should both be changed from int to u32_t.


Both socket and netconn API need to get their len/size arguments changed from int to size_t to have a clean solution for this. I'll do that on the weekend if there are no negative comments until then...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 22 Dec 2008 04:59:42 PM UTC, comment #13: 

> The int can't be 0xFFFF.

Now I see it... Still, I think the wording of this warning is a bit vague...

> Sure, it's always false, but it's also bad coding style

I suppose adding an explicit check for size of int would be more readable than relying on the if statement to reduce to 'always false' on 16-bit platforms. Something like this should work:
#include <limits.h>
#if UINT_MAX > 0xffffu
...
#endif

> That will break the code on many 8- and 16-bit compilers that are not C99 compatible

It won't if you define roll-your-own types like it's currently done with u8_t, u16_t and so on.

Mike Kleshov <kleshov>
Project Member
Mon 22 Dec 2008 04:43:16 PM UTC, comment #12: 

>Do we need to remove that warning?


On a compiler with sizeof int == 2, it's real.  The int can't be 0xFFFF.  Sure, it's always false, but it's also bad coding style when it can be done correctly for all platforms (which lwIP is quite good at).

>I believe the proper solution would be to use C99 types from stdint.h like uint_fast16_t or equivalent.


That will break the code on many 8- and 16-bit compilers that are not C99 compatible.

Bill

Bill Auerbach <billauerbach>
Mon 22 Dec 2008 04:30:00 PM UTC, comment #11: 

> Wouldn't it remove the warning that started this post?

Do we need to remove that warning? The warning says "Constant out of range in comparison", and I am struggling to understand what it actually means. I believe gcc would say "comparison is always false due to limited range of data type", which is exactly what we want from this code. It is wrong to silence every warning of every compiler. The cost of silencing a warning is not zero: code readability could suffer, and there is a risk of intruducing new bugs (which almost happened in this case.)

> It's also marginally more efficient to leave ints and unsigned ints...

I believe the proper solution would be to use C99 types from stdint.h like uint_fast16_t or equivalent.

Mike Kleshov <kleshov>
Project Member
Mon 22 Dec 2008 03:03:39 PM UTC, comment #10: 

Wouldn't it remove the warning that started this post?  It's also marginally more efficient to leave ints and unsigned ints as ints or unsigned ints and not as u16_t/u32_t s_16t/u_16t because int is the native type and most efficient type for the processor.  Why force 16/32 bits when there's no benefit gained from doing so?

Bill

Bill Auerbach <billauerbach>
Mon 22 Dec 2008 02:55:09 PM UTC, comment #9: 

> Why not make w.len unsigned int?


A quick search suggests that most instances of type int in lwip could actually be changed to unsigned int. So converting all of them would make more sense. But that would be a lot of work, and is likely to introduce new bugs in the working code. Besides, would it solve any problem?

Mike Kleshov <kleshov>
Project Member
Mon 22 Dec 2008 02:27:53 PM UTC, comment #8: 

Why not make w.len unsigned int?  It can't be negative.  0xffff could also be changed to 0xffffu or MAX_UINT in this case.  I generally find it to be a bad idea to mixed signed and unsigned integers.

Bill

Bill Auerbach <billauerbach>
Sat 20 Dec 2008 12:25:55 PM UTC, comment #7: 

Yeah, I reverted it. Sorry for the mess. The warning is back, though, but that's OK.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sat 20 Dec 2008 11:27:28 AM UTC, comment #6: 

On closer look, it seems that the original behavior (always false) is desireable:

if ((conn->write_msg->msg.w.len - conn->write_offset > 0xffff)) { /* max_u16_t */
  len = 0xffff;

Apparently, the intent of the code is to limit the 'len' argument (which is u16_t) passed to tcp_write(). When sizeof(int)==2, the amount of data is already limited by the integer data type. So 'always false' seems appropriate here. The easy solution is to revert the patch, then.

Mike Kleshov <kleshov>
Project Member
Sat 20 Dec 2008 10:38:21 AM UTC, comment #5: 

> ... double parentheses ...


I also don't have a clue why there are double parentheses... They seem to have been there for a while... :-)

As to the len/offset/(int)0xffff: I agree (int)0xffff is not a good idea if sizeof(int)==2, I missed that. But I'm also not sure converting these to u32_t is a good idea: The reason they are int is that both the socket and the netconn API have int as size in their send functions. If you change it in api_msg_msg, you have to change the send functions, too!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sat 20 Dec 2008 12:33:23 AM UTC, comment #4: 

Mike is correct, including the fix. The type of the len field in  struct api_msg_msg and the write offset in struct netconn should both be changed from int to u32_t.

Incidentally I've no idea about the reason for double parentheses. It's conventional to use them if the comparison expression includes an assignment (usually that silences a warning), but that's not the case here.

Jonathan Larmour <jifl>
Project Member
Fri 19 Dec 2008 08:39:24 PM UTC, comment #3: 

Hi,
I am concerned by this change in api_msg.c:

@@ -955 +955 @@
-  if ((conn->write_msg->msg.w.len - conn->write_offset > 0xffff)) { /* max_u16_t */
+  if ((conn->write_msg->msg.w.len - conn->write_offset > (int)0xffff)) { /* max_u16_t */

Short version:
It might silence the warning with this particular compiler but it keeps the bug in the code in case of sizeof(int)==2.

Long version:
The code should work fine if sizeof(int)==4. Let's consider the case of sizeof(int)==2.
In the original version of the code, the integer constant 0xffff has the type unsigned int which is u16_t. The left side of operator < will be converted to u16_t. The unsigned comparison will be pointless (always false.)
In the changed version of the code, the integer constant 0xffff is converted to int and becomes -1. The signed comparison will be pointless as well (always true.)
To fix the bug, we need to change the types of both conn->write_msg->msg.w.len and conn->write_offset. Obviously, the piece of code in question assumes that both of them are at least s32_t. At the moment they are int (s16_t).

Other thoughts:
1) In general, I would oppose changes to the code that only intend to silence questionable warnings of some particular compiler.
2) Why the double parentheses around the condition in this if statement?

- mike

Mike Kleshov <kleshov>
Project Member
Fri 19 Dec 2008 06:14:33 PM UTC, comment #2: 

I've fixed some of the errors, though not all: the "Conversion may lose significant digits" warnings in pbuf.c, mem.c and sockets.c:

lwIP relies on less stricter warnings (gcc switch -Wall only) and this warning is NOT shown there (although truncated from int to u16_t), you may define other warning options for your compiler.

> Is it true that some of the lwip code assumes that
> sizeof(int)==4? Can somebody please comment?


No, it's not true. lwIP should run on all platforms since 'int' should only be used where the size does not matter. Of course it might be that there are some places where this is not correctly implemented, but that would be a bug and should be reported here!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 19 Dec 2008 11:19:50 AM UTC, comment #1: 

It appears that about half of those warnings are caused by the fact that sizeof(int)==2 for this particular compiler. The other half is just the compiler being overly enthusiastic emitting warnings.
Is it true that some of the lwip code assumes that sizeof(int)==4? Can somebody please comment?

Mike Kleshov <kleshov>
Project Member
Fri 19 Dec 2008 10:15:23 AM UTC, original submission:  

Tested current CVS build with Paradigm C++ 6.00.045:

Warn :  udp.c(641,76):Ambiguous operators need parentheses
Warn :  udp.c(641,76):Ambiguous operators need parentheses
Warn :  udp.c(641,76):Ambiguous operators need parentheses
Warn :  udp.c(699,78):Ambiguous operators need parentheses
Warn :  udp.c(699,78):Ambiguous operators need parentheses
Warn :  udp.c(699,78):Ambiguous operators need parentheses

Warn :  inet_chksum.c(110,13):Constant is long

Warn :  tcpip.c(301,31):Constant is long

Warn :  tcp_in.c(1319,48):Ambiguous operators need parentheses

Warn :  inet.c(142,6):Conversion may lose significant digits

Warn :  tcp.c(694,42):Integral constant overflow with '*' operator
Warn :  tcp.c(745,40):Integral constant overflow with '*' operator

Warn :  api_msg.c(955,57):Constant out of range in comparison

Warn :  pbuf.c(312,77):Conversion may lose significant digits

Warn :  mem.c(241,45):Conversion may lose significant digits
Warn :  mem.c(252,44):Conversion may lose significant digits
Warn :  mem.c(332,60):Conversion may lose significant digits
Warn :  mem.c(391,8):Conversion may lose significant digits
Warn :  mem.c(518,15):Conversion may lose significant digits
Warn :  mem.c(571,68):Conversion may lose significant digits

Warn :  sockets.c(296,36):Conversion may lose significant digits
Warn :  sockets.c(563,38):Conversion may lose significant digits
Warn :  sockets.c(676,49):Constant out of range in comparison
Warn :  sockets.c(1136,32):Conversion may lose significant digits

Tai-hwa Liang <atliang>

 

Attached Files
file #17535:  ip_addr.h.patch added by atliang (2KiB - Fixing possible building warnings whilst using IN_CLASS[ABC]_{HOST,NET} directly)
file #17091:  lwip.patch added by atliang (10KiB - 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 billauerbach (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kleshov (Posted a comment)
  • -email is unavailable- added by atliang (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-02-26 atliang Attached File- => Added ip_addr.h.patch, #17535
    2009-02-16 goldsimon StatusIn Progress => Done
        Open/ClosedOpen => Closed
    2008-12-20 jifl StatusDone => In Progress
        Open/ClosedClosed => Open
    2008-12-19 goldsimon StatusNone => Done
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2008-12-19 atliang Attached File- => Added lwip.patch, #17091

    Back to the top


    Powered by Savane 3.5