Guard out-of-range shift counts in integer opcodes#559
Conversation
|
gentle ping |
There was a problem hiding this comment.
Pull request overview
This PR makes integer shift opcodes deterministic and NumPy-compatible when the per-element shift count is negative or exceeds the operand bit width, avoiding undefined behavior in the C++ interpreter loop.
Changes:
- Guard
OP_LSHIFT_*to return0when the shift count is out-of-range. - Guard
OP_RSHIFT_*to clamp out-of-range counts towidth-1to preserve sign-fill behavior. - Add a regression test covering out-of-range shift counts for
i4andi8.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
numexpr/interp_body.cpp |
Adds per-element range checks for 32-bit and 64-bit integer shift opcodes to avoid UB and match NumPy semantics. |
numexpr/tests/test_numexpr.py |
Adds a regression test asserting NumExpr matches NumPy for out-of-range shift counts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for dtype in ('i4', 'i8'): | ||
| x = array([5, -5, 0], dtype=dtype) | ||
| for count in (-1, 64, 200): | ||
| y = array([count] * len(x), dtype=dtype) | ||
| assert_array_equal(evaluate("x << y"), x << y) | ||
| assert_array_equal(evaluate("x >> y"), x >> y) |
There was a problem hiding this comment.
Good point. Derived the width from the dtype and added count == width and width+1, so the >= width edge is covered for both i4 and i8.
uwezkhan
force-pushed
the
shift-count-guard
branch
from
de3e6d7 to
caa29dc
Compare
|
Rebased onto master. The only red jobs were the free-threaded 3.13 wheel builds, and those were failing while building numpy from source rather than on this change; master already dropped them in bd99ef3, so they no longer run. Also extended the test to hit the exact width boundary (32 for i4, 64 for i8) that Copilot flagged, not just the far out-of-range counts. |
The integer shift opcodes hand the per-element shift count straight to C++
<</>>. Whena<<bora>>bruns with a count that is negative or at least the operand width (32 for int, 64 for long long) the shift is undefined behavior. On arm64 `evaluate("a<<b")" with b=100 returns 80 because the hardware masks the count, while NumPy returns 0, and a UBSAN build traps right at the shift. Both int and long long, left and right, hit this.After the change an out-of-range left shift yields 0 and an out-of-range right shift clamps the count to width-1 so the sign bit fills, which is the result NumPy gives. The guard sits in the opcode next to the existing div/mod guards because the count is only known per element at run time, so a caller-side check could not cover array shift counts. Tradeoff is one extra unsigned compare per element; in-range counts keep the same value and the branch is well predicted.