[issue1621] Do not assume signed integer overflow behavior

[Mark Dickinson]

Mark Dickinson <dickinsm at gmail.com> added the comment:

A few comments:

(1) I agree that signed overflows should be avoided
where possible.

(2) I think some of the changes in the latest patch
(fix-overflows-final.patch) are unnecessary, and
decrease readability a bit. An example is the following
chunk for the x_divrem function in Objects/longobject.c.

@@ -1799,7 +1799,7 @@ x_divrem(PyLongObject *v1, PyLongObject *w1, PyLongObject **prem)
 	k = size_v - size_w;
 	a = _PyLong_New(k + 1);
 
-	for (j = size_v; a != NULL && k >= 0; --j, --k) {
+	for (j = size_v; a != NULL && k >= 0; j = (unsigned)j - 1 , k = (unsigned)k - 1) {
 		digit vj = (j >= size_v) ? 0 : v->ob_digit[j];
 		twodigits q;
 		stwodigits carry = 0;

In this case it's easy to see from the code that j and k
will always be nonnegative, so replacing --j with j =
(unsigned)j - 1 is unnecessary.  (This chunk no longer
applies for 2.7 and 3.1, btw, since x_divrem got
rewritten recently.)  Similar comments apply to the
change:

-			min_gallop -= min_gallop > 1;
+			if (min_gallop > 1) min_gallop = (unsigned)min_gallop - 1;

in Objects/listobject.c.  Here it's even clearer that
the cast is unnecessary.

I assume these changes were made to silence warnings from
-Wstrict-overflow, but I don't think that should be a goal:
I'd suggest only making changes where there's a genuine
possibility of overflow (even if it's a remote one), and
leaving the code unchanged if it's reasonably easy to
see that overflow is impossible.


(3) unicode_hash also needs fixing, as do the lookup
algorithms for set and dict.  Both use overflowing
arithmetic on signed types as a matter of course.
Probably a good few of the hash algorithms for the
various object types in Objects/ are suspect.

----------
nosy: +marketdickinson

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue1621>
_______________________________________