classification
Title: Objects/obmalloc.c:529: warning: comparison is always false due to limited range of data type
Type: compile error Stage:
Components: Build Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: loewis Nosy List: brett.cannon, christian.heimes, loewis
Priority: release blocker Keywords: 64bit, patch

Created on 2008-08-22 01:03 by christian.heimes, last changed 2008-09-11 06:57 by loewis.

Files
File name Uploaded Description Edit Remove
issue3642.diff brett.cannon, 2008-09-10 05:41
obmalloc.diff loewis, 2008-09-10 15:43
Messages
msg71712 (view) Author: Christian Heimes (christian.heimes) Date: 2008-08-22 01:02
I'm getting a compiler warning on Linux AMD64. It's most probably an
issue related to 64bit builds, because size_t > uint_t on 64bit systems.
Such warnings may hide overflow issues.

gcc -pthread -c -fno-strict-aliasing -g -Wall -Wstrict-prototypes  -I.
-IInclude -I./Include   -DPy_BUILD_CORE -o Objects/obmalloc.o
Objects/obmalloc.c
Objects/obmalloc.c: In function 'new_arena':
Objects/obmalloc.c:529: warning: comparison is always false due to
limited range of data type

I was able to silence the compiler by declaring numarenas as size_t
instead of uint.
msg71713 (view) Author: Christian Heimes (christian.heimes) Date: 2008-08-22 01:06
I'm seeing a similar issue in py3k:

Objects/bytesobject.c: In function '_PyBytes_FormatLong':
Objects/bytesobject.c:3203: warning: comparison is always false due to
limited range of data type
msg71772 (view) Author: Christian Heimes (christian.heimes) Date: 2008-08-22 19:51
The warning in obmalloc.c was silenced in r65975 (trunk) and r65976 (py3k)
msg71820 (view) Author: Brett Cannon (brett.cannon) Date: 2008-08-23 22:42
There is no patch here, so undoing the "needs review" flag.
msg71843 (view) Author: Christian Heimes (christian.heimes) Date: 2008-08-24 16:42
I've fixed both compiler warnings in trunk and py3k branch.
msg71893 (view) Author: Martin v. Löwis (loewis) Date: 2008-08-24 22:10
I think the patch is fairly incomplete. "i" still stays at uint, so if
numarenas would ever overflow uint, the loop would never reach
numarenas, and all arenas would get discarded.

Likewise, maxarenas still stays at uint, so when numarenas overflows
uint, the additional arenas get discarded.

The compiler was right observing that the condition is always false:
this was a security check to prevent overflow, but on this specific
system, overflow couldn't occur in the first place. 

If you want to silence the warning, try casting numarenas to size_t in
the test only. If you want to properly remove the unnecessary test, put
an ifdef around it, testing whether size_t is larger than uint.

With the current code, it might be that you have disabled the security
check: numarenas <= maxarenas will not occur anymore (since numarenas is
wider than maxarenas); as a consequence, you then see the silent
truncation later on instead of the exception.
msg71897 (view) Author: Christian Heimes (christian.heimes) Date: 2008-08-24 22:45
Good analysis Martin!

In my humble opinion it should be safe to limit the amount of arenas to
UINT_MAX instead of PY_SIZE_MAX. 4,294,967,295 arenas should be more
than sufficient for the next decade or two. Do you concur?
msg71909 (view) Author: Martin v. Löwis (loewis) Date: 2008-08-25 04:50
> In my humble opinion it should be safe to limit the amount of arenas to
> UINT_MAX instead of PY_SIZE_MAX. 4,294,967,295 arenas should be more
> than sufficient for the next decade or two. Do you concur?

It is certainly reasonable to limit arenas to uint. Still, the test
for SIZE_MAX must remain: it doesn't test whether numarenas overflows,
but whether numarenas*sizeof(*arenas) overflows as a parameter for
realloc. As the parameter for realloc is size_t (per C spec), the
overflow test needs to use PY_SIZE_MAX.
msg72942 (view) Author: Brett Cannon (brett.cannon) Date: 2008-09-10 05:41
Going with what Martin suggested, the attached patch reverts what
Christian did and adds an #ifdef sizeof(uint) <= sizeof(uint) around the
PY_SIZE_MAX check. Someone should obviously test on an AMD64 machine (I
have a Core 2 Mac, but I don't know what flags I would need to pass to
trigger the warning on my machine).
msg72943 (view) Author: Martin v. Löwis (loewis) Date: 2008-09-10 05:53
It doesn't work that way - you can't use sizeof() in the preprocessor.
Instead, the pyconfig.h constants should be used. I'll try to look into
this later today.
msg72968 (view) Author: Martin v. Löwis (loewis) Date: 2008-09-10 15:43
Here is my attempt for a patch (obmalloc.diff). Please review.
msg72994 (view) Author: Brett Cannon (brett.cannon) Date: 2008-09-10 22:40
Patch looks good to me and Martin's analysis of the test being useless
on a platform where size_t > uint_t makes sense.
msg73007 (view) Author: Martin v. Löwis (loewis) Date: 2008-09-11 06:57
I have now committed the new patch as r66383 and r66384
History
Date User Action Args
2008-09-11 06:57:06loewissetstatus: open -> closed
resolution: fixed
messages: + msg73007
2008-09-10 22:40:26brett.cannonsetkeywords: - needs review
messages: + msg72994
2008-09-10 15:43:31loewissetkeywords: + needs review
files: + obmalloc.diff
messages: + msg72968
2008-09-10 05:53:41loewissetmessages: + msg72943
2008-09-10 05:41:13brett.cannonsetfiles: + issue3642.diff
assignee: loewis
messages: + msg72942
keywords: + patch
2008-09-09 13:14:18barrysetpriority: deferred blocker -> release blocker
2008-09-04 01:11:32benjamin.petersonsetpriority: release blocker -> deferred blocker
2008-08-25 04:50:34loewissetmessages: + msg71909
2008-08-24 22:45:27christian.heimessetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg71897
2008-08-24 22:10:01loewissetnosy: + loewis
messages: + msg71893
2008-08-24 16:42:57christian.heimessetstatus: open -> closed
resolution: fixed
messages: + msg71843
2008-08-23 22:42:50brett.cannonsetkeywords: - needs review
nosy: + brett.cannon
messages: + msg71820
2008-08-22 19:51:32christian.heimessetmessages: + msg71772
2008-08-22 01:06:16christian.heimessetmessages: + msg71713
2008-08-22 01:03:01christian.heimescreate