[ python-Bugs-1521947 ] possible bug in mystrtol.c with recent gcc

SourceForge.net noreply at sourceforge.net
Tue Oct 3 12:35:40 CEST 2006 

Bugs item #1521947, was opened at 2006-07-13 13:39
Message generated for change (Comment added) made by tim_one
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1521947&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Python Interpreter Core
Group: Python 2.4
Status: Open
Resolution: None
Priority: 5
Submitted By: Marien Zwart (marienz)
Assigned to: Nobody/Anonymous (nobody)
Summary: possible bug in mystrtol.c with recent gcc

Initial Comment:
python 2.5b2 compiled with gentoo's gcc 4.1.1 and -O2
fails test_unary_minus in test_compile.py. Some
investigating showed that the -ftree-vrp pass of that
gcc (implied by -O2) optimizes out the "result ==
-result" test on line 215 of PyOS_strtol, meaning
PyOS_strtol of the most negative long will incorrectly
overflow. 

Python deals with this in this case by constructing a
python long object instead of a python int object, so
at least in this case the problem is not serious. I
have no idea if there is a case where this is a "real"
problem.

At first I reported this as a gentoo compiler bug, but
got the reply that:

"""
I'm pretty sure it's broken. -LONG_MIN overflows when
LONG_MIN < -LONG_MAX, and in standard C as well as "GNU
C", behaviour after overflow on signed integer
operations is undefined.
"""

(see https://bugs.gentoo.org/show_bug.cgi?id=140133)

So now I'm reporting this here. There seems to be a
LONG_MIN constant in limits.h here that could checked
against instead, but I have absolutely no idea how
standard that is. Or this could be a compiler bug after
all, in which case I would appreciate information I Can
use to back that up :)

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

>Comment By: Tim Peters (tim_one)
Date: 2006-10-03 06:35

Message:
Logged In: YES 
user_id=31435

> Based on the other bug I guess that casting an arbitrary
> long to unsigned long is allowed.

Right, C defines the result of casting any integral type to
any unsigned integral type.

> If so, then maybe we could use the following test:
>
>  if (sign == '-' && uresult == 0-(unsigned
long)LONG_MIN) {
>      result = LONG_MIN;
>  }
>
> which states the intention a bit more clearly and
> without the assert().

We could.  It's not really clearer to me, given that the
current code is explained in a comment block before
PyOS_strtol(), and I couldn't care less about removing an
assert, so I'm not going to bother.  I wouldn't object to
changing it, although "0-" instead of plain unary "-" also
begs for an explanation lest someone delete the "0" because
it looks plain silly.

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

Comment By: Armin Rigo (arigo)
Date: 2006-10-03 06:10

Message:
Logged In: YES 
user_id=4771

Based on the other bug I guess that casting an arbitrary
long to unsigned long is allowed.  If so, then maybe we could
use the following test:

  if (sign == '-' && uresult == 0-(unsigned long)LONG_MIN) {
      result = LONG_MIN;
  }

which states the intention a bit more clearly and without the
assert().

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

Comment By: Tim Peters (tim_one)
Date: 2006-09-05 01:21

Message:
Logged In: YES 
user_id=31435

Well, I deliberately avoided using LONG_MIN in the patches
for the other bug, so that should answer your question ;-)

If someone wants to take the small risk of backporting it,
fine by me -- it's not going to break on Windows, and if it
doesn't break on your box either ...

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-09-05 01:12

Message:
Logged In: YES 
user_id=33168

Tim, how do you feel about backporting now?  (Not sure if
your opinion changed based on the other bug.)  That and I'm
cleaning out my mailbox, so I don't have to look at this any
more. :-)

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

Comment By: Tim Peters (tim_one)
Date: 2006-07-27 17:00

Message:
Logged In: YES 
user_id=31435

Neal, as I said in the checkin comment, I expect it's no
less likely that we'll get screwed by goofy platform values
for LONG_MIN and LONG_MAX than that we /got/ screwed by an
"over ambitious" compiler optimizing away "result ==
-result", so I'm not sure backporting is a good idea.  I
stuck in an assert that might fail if a platform is insane;
if there are no reports of that assert triggering, then I'd
feel better about backporting the change.

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

Comment By: Matt Fleming (splitscreen)
Date: 2006-07-27 06:49

Message:
Logged In: YES 
user_id=1126061

Fixed for me on NetBSD -current AMD64, gcc 4.1.2

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

Comment By: Nick Maclaren (nmm)
Date: 2006-07-27 04:31

Message:
Logged In: YES 
user_id=42444

Fixed for me, too, and the code looks solid.


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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-07-27 00:48

Message:
Logged In: YES 
user_id=33168

I reopened this bug as it still affects 2.4.  Tim's fix
should be backported.

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-07-27 00:35

Message:
Logged In: YES 
user_id=33168

Tim's fix corrected the problem on amd64 gentoo linux with
gcc 4.1.1.

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

Comment By: Tim Peters (tim_one)
Date: 2006-07-26 21:16

Message:
Logged In: YES 
user_id=31435

Made a non-heroic effort to repair this in rev 50855, but
have no platform on which it could make any difference (and
apparently no Python buildbot test platform cares either) .
 Would be nice if someone who has such a platform could
check against current Python trunk.

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

Comment By: Nick Maclaren (nmm)
Date: 2006-07-17 05:20

Message:
Logged In: YES 
user_id=42444

Yes, this is a duplicate of 1521726.

The compiler people's answer is correct, and mystrtoul.c is
incorrect. LONG_MIN has been in C since C90, so should be
safe, but its value may not always be what you expect.

However, the code breakage is worse that just that test, and
there are the following 3 cases of undefined behaviour:

    1) Casting and unsigned long to long above LONG_MAX.
    2) That test.
    3) result = -result.

The fix should be to make result unsigned long, and check
the range BEFORE any conversion.  Nothing else is safe.

It is a matter of taste whether to include a fiddle for the
number that can be held only when negated - I wouldn't, and
would let that number be converted to a Python long, but
others might disagree.


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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-07-16 21:13

Message:
Logged In: YES 
user_id=33168

Is this a duplicate of 1521726?

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

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1521947&group_id=5470

More information about the Python-bugs-list mailing list