JIT: Fix correctness issues in RangeOps#128063
Closed
EgorBo wants to merge 3 commits into
Closed
Conversation
Three correctness bugs in src/coreclr/jit/rangecheck.h:
1. RangeOps::Or - unsound upper bound
Or used ApplyRangeOp, which pairs (r1.lo, r2.lo) and (r1.hi, r2.hi).
That works for monotonic operators but is wrong for bitwise OR:
[3..5] | [4..7] -> lo = 3|4 = 7, hi = 5|7 = 7 (claims [7..7])
The actual minimum is max(3, 4) = 4 (achieved at x=4, y=4), so the
computed lower bound is too large and the range is too narrow.
Downstream folding could then mis-prove relops, e.g. (x|y) != 7 was
folded to false in:
static int Test(int x, int y)
{
if (x < 3 || x > 5) return -1;
if (y < 4 || y > 7) return -1;
int r = x | y;
if (r != 7) return 1; // folded to false -> Test(4, 4) returned 0
return 0;
}
Replaced with a direct constant-range computation for non-negative
ranges:
lo = max(a, c) // OR can only set more bits, so result >= max
hi = bit-fill mask of max(b, d), i.e. (1 << (top_bit + 1)) - 1
// bits in result can be at most as high as
// the top bit of max(b, d)
Note that a naive `b | d` would also be unsound: e.g. [3..4] | [4..4]
would give upper = 4|4 = 4, but 3|4 = 7 is also reachable.
2. RangeOps::Multiply - signed-overflow UB on the unsigned path
When unsignedMul = true, the four corner products use plain signed
`int` multiplication after only an *unsigned* overflow check passes.
For example [0..50000] * [0..50000] with unsignedMul = true: all four
MulOverflows(_, _, /*unsigned*/ true) checks pass (2.5e9 fits unsigned),
but `50000 * 50000` as int*int is signed-overflow UB. Added a second
overflow check using signed semantics whenever unsignedMul is set.
3. RangeOps::ConvertShiftToMultiply - 1 << 31
The guard allowed shift count = 31, producing `1 << 31`. That is
defined to INT_MIN in C++20, undefined behavior pre-C++20, and either
way the subsequent Multiply cannot use it usefully. Tightened the
upper bound to 30.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT range analysis (RangeOps in rangecheck.h) to avoid unsound range computations and undefined behavior in a few integer operations that can feed downstream assertion propagation and folding.
Changes:
- Fix
RangeOps::Orto compute a conservative, sound result range for non-negative constant ranges (instead of applying a monotonic-style pairing that is invalid for OR). - Prevent signed-overflow UB in
RangeOps::Multiplywhen the operation is being analyzed withunsignedMul=trueby re-checking with signed overflow semantics before performingintmultiplications. - Tighten
ConvertShiftToMultiplyto disallow shift count 31 to avoid problematic1 << 31behavior and keep subsequent multiplication reasoning safe.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three correctness bugs in
src/coreclr/jit/rangecheck.h:1.
RangeOps::Or— unsound upper boundOrusedApplyRangeOp, which pairs(r1.lo, r2.lo)and(r1.hi, r2.hi). That works for monotonic operators but is wrong for bitwise OR:The actual minimum is
max(3, 4) = 4(achieved atx=4, y=4), so the computed lower bound is too large and the range is too narrow. Downstream folding could then mis-prove relops, e.g.(x|y) != 7was folded to false in:Replaced with a direct constant-range computation for non-negative ranges:
lo = max(a, c)— OR can only set more bits, so result>= max(x, y)hi= bit-fill mask ofmax(b, d), i.e.(1 << (top_bit + 1)) - 1— bits in result can be at most as high as the top bit ofmax(b, d)Note that a naive
b | dwould also be unsound: e.g.[3..4] | [4..4]would give upper =4|4 = 4, but3|4 = 7is also reachable.2.
RangeOps::Multiply— signed-overflow UB on the unsigned pathWhen
unsignedMul = true, the four corner products use plain signedintmultiplication after only an unsigned overflow check passes. For example[0..50000] * [0..50000]withunsignedMul = true: all fourMulOverflows(_, _, /*unsigned*/ true)checks pass (2.5e9 fits unsigned), but50000 * 50000asint*intis signed-overflow UB. Added a second overflow check using signed semantics wheneverunsignedMulis set.3.
RangeOps::ConvertShiftToMultiply—1 << 31The guard allowed shift count = 31, producing
1 << 31. That is defined toINT_MINin C++20, undefined behavior pre-C++20, and either way the subsequentMultiplycannot use it usefully. Tightened the upper bound to 30.