Skip to content

bpo-29816: Shift operation now has less opportunity to raise OverflowError. #680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 30, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions Lib/test/test_long.py
Original file line number Diff line number Diff line change
Expand Up @@ -906,20 +906,48 @@ def test_correctly_rounded_true_division(self):
self.check_truediv(-x, y)
self.check_truediv(-x, -y)

def test_negative_shift_count(self):
with self.assertRaises(ValueError):
42 << -3
with self.assertRaises(ValueError):
42 << -(1 << 1000)
with self.assertRaises(ValueError):
42 >> -3
with self.assertRaises(ValueError):
42 >> -(1 << 1000)

def test_lshift_of_zero(self):
self.assertEqual(0 << 0, 0)
self.assertEqual(0 << 10, 0)
with self.assertRaises(ValueError):
0 << -1
self.assertEqual(0 << (1 << 1000), 0)
with self.assertRaises(ValueError):
0 << -(1 << 1000)

@support.cpython_only
def test_huge_lshift_of_zero(self):
# Shouldn't try to allocate memory for a huge shift. See issue #27870.
# Other implementations may have a different boundary for overflow,
# or not raise at all.
self.assertEqual(0 << sys.maxsize, 0)
with self.assertRaises(OverflowError):
0 << (sys.maxsize + 1)
self.assertEqual(0 << (sys.maxsize + 1), 0)

@support.cpython_only
@support.bigmemtest(sys.maxsize + 1000, memuse=2/15 * 2, dry_run=False)
def test_huge_lshift(self, size):
self.assertEqual(1 << (sys.maxsize + 1000), 1 << 1000 << sys.maxsize)

def test_huge_rshift(self):
self.assertEqual(42 >> (1 << 1000), 0)
self.assertEqual((-42) >> (1 << 1000), -1)

@support.cpython_only
@support.bigmemtest(sys.maxsize + 500, memuse=2/15, dry_run=False)
def test_huge_rshift_of_huge(self, size):
huge = ((1 << 500) + 11) << sys.maxsize
self.assertEqual(huge >> (sys.maxsize + 1), (1 << 499) + 5)
self.assertEqual(huge >> (sys.maxsize + 1000), 0)

def test_small_ints(self):
for i in range(-5, 257):
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ What's New in Python 3.7.0 alpha 1?
Core and Builtins
-----------------

- bpo-29816: Shift operation now has less opportunity to raise OverflowError.
ValueError always is raised rather than OverflowError for negative counts.
Shifting zero with non-negative count always returns zero.

- bpo-28856: Fix an oversight that %b format for bytes should support objects
follow the buffer protocol.

Expand Down
72 changes: 56 additions & 16 deletions Objects/longobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -4294,6 +4294,12 @@ long_rshift(PyLongObject *a, PyLongObject *b)

CHECK_BINOP(a, b);

if (Py_SIZE(b) < 0) {
PyErr_SetString(PyExc_ValueError,
"negative shift count");
return NULL;
}

if (Py_SIZE(a) < 0) {
/* Right shifting negative numbers is harder */
PyLongObject *a1, *a2;
Expand All @@ -4309,18 +4315,34 @@ long_rshift(PyLongObject *a, PyLongObject *b)
}
else {
shiftby = PyLong_AsSsize_t((PyObject *)b);
if (shiftby == -1L && PyErr_Occurred())
return NULL;
if (shiftby < 0) {
PyErr_SetString(PyExc_ValueError,
"negative shift count");
return NULL;
if (shiftby >= 0) {
wordshift = shiftby / PyLong_SHIFT;
loshift = shiftby - wordshift * PyLong_SHIFT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not loshift = shiftby % PyLong_SHIFT;?

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left shift uses such code. I suppose this is because multiplication and substraction are faster than the "%" operation. If there is a reason to use it for left shift, there is a reason to use it for right shift.

The new code for left and right shifts are almost exact duplicates (there is additional optimization for right shift, but it is not very important and can be omitted). Is it worth to extract it in the helper function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is because multiplication and substraction are faster than the "%" operation.

This seems unlikely to me. In particular, I'd expect an optimising compiler to be able to recognise and do a good job of optimising the pair shiftby / PyLong_SHIFT and shiftby % PyLong_SHIFT if they appear next to each other in code. Rewriting the % would likely make it harder for the compiler to recognise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth to extract it in the helper function?

That sounds reasonable to me, if you want to do it; if you don't, that also sounds reasonable to me. :-)

}
wordshift = shiftby / PyLong_SHIFT;
newsize = Py_ABS(Py_SIZE(a)) - wordshift;
else {
/* PyLong_Check(b) is true and Py_SIZE(b) >= 0, so it must be that
PyLong_AsSsize_t raised an OverflowError. */
assert(PyErr_ExceptionMatches(PyExc_OverflowError));
PyErr_Clear();
/* With such large wordshift return PyLong_FromLong(0) below. */
wordshift = PY_SSIZE_T_MAX / sizeof(digit);
loshift = 0;
#if SIZE_MAX / 2 < ULLONG_MAX / 15
if (Py_SIZE(a) > PY_SSIZE_T_MAX / PyLong_SHIFT) {
int overflow;
long long lshiftby = PyLong_AsLongLongAndOverflow((PyObject *)b,
&overflow);
assert(!PyErr_Occurred());
if (lshiftby >= 0 && lshiftby / PyLong_SHIFT < wordshift) {
wordshift = lshiftby / PyLong_SHIFT;
loshift = lshiftby - wordshift * PyLong_SHIFT;
}
}
#endif
}
newsize = Py_SIZE(a) - wordshift;
if (newsize <= 0)
return PyLong_FromLong(0);
loshift = shiftby % PyLong_SHIFT;
hishift = PyLong_SHIFT - loshift;
lomask = ((digit)1 << hishift) - 1;
himask = PyLong_MASK ^ lomask;
Expand Down Expand Up @@ -4349,21 +4371,39 @@ long_lshift(PyObject *v, PyObject *w)

CHECK_BINOP(a, b);

shiftby = PyLong_AsSsize_t((PyObject *)b);
if (shiftby == -1L && PyErr_Occurred())
return NULL;
if (shiftby < 0) {
if (Py_SIZE(b) < 0) {
PyErr_SetString(PyExc_ValueError, "negative shift count");
return NULL;
}

if (Py_SIZE(a) == 0) {
return PyLong_FromLong(0);
}

shiftby = PyLong_AsSsize_t((PyObject *)b);
/* wordshift, remshift = divmod(shiftby, PyLong_SHIFT) */
wordshift = shiftby / PyLong_SHIFT;
remshift = shiftby - wordshift * PyLong_SHIFT;
if (shiftby >= 0) {
wordshift = shiftby / PyLong_SHIFT;
remshift = shiftby - wordshift * PyLong_SHIFT;
}
else {
/* PyLong_Check(b) is true and Py_SIZE(b) >= 0, so it must be that
PyLong_AsSsize_t raised an OverflowError. */
assert(PyErr_ExceptionMatches(PyExc_OverflowError));
PyErr_Clear();
/* With such large wordshift raise error in _PyLong_New() below. */
wordshift = PY_SSIZE_T_MAX / sizeof(digit);
remshift = 0;
#if SIZE_MAX / 2 < ULLONG_MAX / 15
int overflow;
long long lshiftby = PyLong_AsLongLongAndOverflow((PyObject *)b,
&overflow);
assert(!PyErr_Occurred());
if (lshiftby >= 0 && lshiftby / PyLong_SHIFT < wordshift) {
wordshift = lshiftby / PyLong_SHIFT;
remshift = lshiftby - wordshift * PyLong_SHIFT;
}
#endif
}

oldsize = Py_ABS(Py_SIZE(a));
newsize = oldsize + wordshift;
Expand Down