gcc 4.2 exposes signed integer overflows
I discovered that gcc 4.2 exposes a flaw with signed integer overflows in python. This bug and the necessary fix has been discussed in detail on the gcc mailing list. I have filed a detailed bug report and the recommended patch proposed by the gcc developers. This problem should be addressed BEFORE python 2.5 is released. The bug report is... [ 1545668 ] gcc trunk (4.2) exposes a signed integer overflows in the python sourceforge bug tracker. Thanks in advance for attempting to fix this before Python 2.5 is released. Jack
On 8/26/06, Jack Howarth <howarth@bromo.msbb.uc.edu> wrote:
I discovered that gcc 4.2 exposes a flaw with signed integer overflows in python. This bug and the necessary fix has been discussed in detail on the gcc mailing list. I have filed a detailed bug report and the recommended patch proposed by the gcc developers. This problem should be addressed BEFORE python 2.5 is released. The bug report is...
[ 1545668 ] gcc trunk (4.2) exposes a signed integer overflows
in the python sourceforge bug tracker. Thanks in advance for attempting to fix this before Python 2.5 is released.
I'm not sure I follow why this isn't considered a regression in GCC. Clearly, on all current hardware, x == -x is also true for the most negative int (0x80000000 on a 32-bit box). Why is GCC attempting to break our code (and then blaming us for it!) by using the C standard's weaselwords that signed integer overflow is undefined, despite that it has had a traditional meaning on 2's complement hardware for many decades? If GCC starts to enforce everything that the C standard says is undefined then very few programs will still work... -- --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum wrote:
On 8/26/06, Jack Howarth <howarth@bromo.msbb.uc.edu> wrote:
I discovered that gcc 4.2 exposes a flaw with signed integer overflows in python. This bug and the necessary fix has been discussed in detail on the gcc mailing list. I have filed a detailed bug report and the recommended patch proposed by the gcc developers. This problem should be addressed BEFORE python 2.5 is released. The bug report is...
[ 1545668 ] gcc trunk (4.2) exposes a signed integer overflows
in the python sourceforge bug tracker. Thanks in advance for attempting to fix this before Python 2.5 is released.
I'm not sure I follow why this isn't considered a regression in GCC. Clearly, on all current hardware, x == -x is also true for the most negative int (0x80000000 on a 32-bit box). Why is GCC attempting to break our code (and then blaming us for it!) by using the C standard's weaselwords that signed integer overflow is undefined, despite that it has had a traditional meaning on 2's complement hardware for many decades? If GCC starts to enforce everything that the C standard says is undefined then very few programs will still work...
+1. It's clearly undefined behaviour, but that doesn't mean that it is a good idea for gcc to be making changes that could silently break a lot of code, in pursuit of a microoptimization that is unlikely to significantly help performance. CPython should be fixed anyway. The correct fix is "if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x)". -- David Hopwood <david.nospam.hopwood@blueyonder.co.uk>
On 8/26/06, David Hopwood <david.nospam.hopwood@blueyonder.co.uk> wrote:
CPython should be fixed anyway. The correct fix is "if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x)".
Why not just "... && x == LONG_MIN"? -- Thomas Wouters <thomas@python.org> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
[David Hopwood]
CPython should be fixed anyway. The correct fix is "if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x)".
Note that this was already suggested in the bug report. [Thomas Wouters]
Why not just "... && x == LONG_MIN"?
In full, if (y == -1 && x == LONG_MIN) "should work" too. In practice we try to avoid numeric symbols from platform header files because so many platforms have screwed these up over the centuries (search for LONG_BIT or HUGE_VAL ;-)), and because it's better (when possible) not to tie the code to that `x` was specifically declared as type "long" (e.g., just more stuff that will break if Python decides to make its short int of type PY_LONG_LONG instead). In this specific case, there may also have been a desire to avoid generating a memory load for a fat constant. However, since this is integer division, in real life (outside the test suite) we'll never go beyond the "y == -1" test.
Thomas Wouters wrote:
On 8/26/06, David Hopwood <david.nospam.hopwood@blueyonder.co.uk> wrote:
CPython should be fixed anyway. The correct fix is "if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x)".
Why not just "... && x == LONG_MIN"?
Because the intent is to check that x / y does not overflow a long, and x == LONG_MIN would not cause an overflow on 1's complement or sign-magnitude systems. (CPython has probably only been tested on 2's complement systems anyway, but if we're going to be pedantic about depending only on things in the C standard...) -- David Hopwood <david.nospam.hopwood@blueyonder.co.uk>
[David Hopwood]
(CPython has probably only been tested on 2's complement systems anyway,
Definitely so. Are there any boxes using 1's-comp or sign-magnitude integers anymore? Python assumes 2's-comp in many places.
but if we're going to be pedantic about depending only on things in the C standard...)
No, in that respect we're driven by the silliest decisions made by C compiler writers ;-)
On Sunday 27 August 2006 05:06, Jack Howarth wrote:
I discovered that gcc 4.2 exposes a flaw with signed integer overflows in python. This bug and the necessary fix has been discussed in detail on the gcc mailing list. I have filed a detailed bug report and the recommended patch proposed by the gcc developers. This problem should be addressed BEFORE python 2.5 is released. The bug report is...
[ 1545668 ] gcc trunk (4.2) exposes a signed integer overflows
in the python sourceforge bug tracker. Thanks in advance for attempting to fix this before Python 2.5 is released. Jack
Regardless of whether we consider gcc's behaviour to be correct or not, I do agree we need a fix for this in 2.5 final. That should also be backported to release24-maint for the 2.4.4 release, and maybe release23-maint, as Barry recently started talking about cutting a 2.3.6. Can I nominate Tim, with his terrifying knowledge of C compiler esoterica, as the person to pick the best fix? Thanks, Anthony
[Anthony Baxter]
Regardless of whether we consider gcc's behaviour to be correct or not,
It is correct, but more to the point it's, umm, /there/ ;-)
I do agree we need a fix for this in 2.5 final. That should also be backported to release24-maint for the 2.4.4 release, and maybe release23-maint, as Barry recently started talking about cutting a 2.3.6.
Can I nominate Tim, with his terrifying knowledge of C compiler esoterica, as the person to pick the best fix?
It's a bitch. Changing to if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x) is the obvious fix, but violates our "no warnings" policy: the MS compiler warns about applying unary minus to an unsigned operand -- it "looks insane" to /their/ compiler writers ;-). Elegant patch below -- LOL. Would be nice if someone verified it worked on a box where it matters. Would also be nice if people checked to see whether their compiler(s) warn about something else now. IIndex: Objects/intobject.c =================================================================== --- Objects/intobject.c (revision 51618) +++ Objects/intobject.c (working copy) @@ -564,8 +564,14 @@ "integer division or modulo by zero"); return DIVMOD_ERROR; } - /* (-sys.maxint-1)/-1 is the only overflow case. */ - if (y == -1 && x < 0 && x == -x) + /* (-sys.maxint-1)/-1 is the only overflow case. x is the most + * negative long iff x < 0 and, on a 2's-complement box, x == -x. + * However, -x is undefined (by C) if x /is/ the most negative long + * (it's a signed overflow case), and some compilers care. So we cast + * x to unsigned long first. However, then other compilers warn about + * applying unary minus to an unsigned operand. Hence the weird "0-". + */ + if (y == -1 && x < 0 && (unsigned long)x == 0-(unsigned long)x) return DIVMOD_OVERFLOW; xdivy = x / y; xmody = x - xdivy * y;
Hi Tim, On Sat, Aug 26, 2006 at 08:37:46PM -0400, Tim Peters wrote:
[Thomas Wouters]
Why not just "... && x == LONG_MIN"?
it's better (when possible) not to tie the code to that `x` was specifically declared as type "long" (e.g., just more stuff that will break if Python decides to make its short int of type PY_LONG_LONG instead).
The proposed "correct fix" breaks this goal too:
"if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x)".
^^^^^^^^^^^^^ ^^^^^^^^^^^^^ A bientot, Armin
[Thomas Wouters]
Why not just "... && x == LONG_MIN"?
[Tim Peters]
it's better (when possible) not to tie the code to that `x` was specifically declared as type "long" (e.g., just more stuff that will break if Python decides to make its short int of type PY_LONG_LONG instead).
[Armin Rigo]
The proposed "correct fix" breaks this goal too:
"if (y == -1 && x < 0 && (unsigned long)x == -(unsigned long)x)".
^^^^^^^^^^^^^ ^^^^^^^^^^^^^
Yup, although as noted before the proposed fix actually writes == 0-(unsigned long)x at the tail end instead (to avoid compiler warnings at least under MS C). It doesn't run afoul of the other criterion you snipped from the start of the quoted paragraph: In practice we try to avoid numeric symbols from platform header files because so many platforms have screwed these up over the centuries (search for LONG_BIT or HUGE_VAL ;-)), .... This is a wrong time in the release process to take on chance on discovering a flaky LONG_MIN on some box, so I want to keep the code as much as possible like what's already there (which worked fine for > 10 years on all known boxes) for now. Speaking of which, I saw no feedback on the proposed patch in http://mail.python.org/pipermail/python-dev/2006-August/068502.html so I'll just check that in tomorrow.
On Wednesday 30 August 2006 08:57, Tim Peters wrote:
Speaking of which, I saw no feedback on the proposed patch in
http://mail.python.org/pipermail/python-dev/2006-August/068502.html
so I'll just check that in tomorrow.
Fine with me! thanks, Anthony -- Anthony Baxter <anthony@interlink.com.au> It's never too late to have a happy childhood.
On Wednesday 30 August 2006 08:57, Tim Peters wrote:
Speaking of which, I saw no feedback on the proposed patch in
http://mail.python.org/pipermail/python-dev/2006-August/068502.html
so I'll just check that in tomorrow.
This should also be backported to release24-maint and release23-maint. Let me know if you can't do the backport... -- Anthony Baxter <anthony@interlink.com.au> It's never too late to have a happy childhood.
[Tim Peters]
Speaking of which, I saw no feedback on the proposed patch in
http://mail.python.org/pipermail/python-dev/2006-August/068502.html
so I'll just check that in tomorrow.
[Anthony Baxter]
This should also be backported to release24-maint and release23-maint. Let me know if you can't do the backport...
Done in rev 51711 on the 2.5 branch. Done in rev 51715 on the 2.4 branch. Done in rev 51716 on the trunk, although in the LONG_MIN way (which is less obscure, but a more "radical" code change). I don't care about the 2.3 branch, so leaving that to someone who does. Merge rev 51711 from the 2.5 branch. It will generate a conflict on Misc/NEWS. Easiest to revert Misc/NEWS then and just copy/paste the little blurb from 2.5 news at the appropriate place: """ - Overflow checking code in integer division ran afoul of new gcc optimizations. Changed to be more standard-conforming. """
participants (7)
-
Anthony Baxter -
Armin Rigo -
David Hopwood -
Guido van Rossum -
howarth＠bromo.msbb.uc.edu -
Thomas Wouters -
Tim Peters