From da83d0e502bc5acd630c849c02116aceca420ac0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 12 May 2026 03:47:50 +0200 Subject: [PATCH 1/3] JIT: Fix correctness issues in RangeOps 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> --- src/coreclr/jit/rangecheck.h | 84 ++++++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index ea399f334edbeb..60ca38b7b7b919 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -391,6 +391,17 @@ struct RangeOps return Limit(Limit::keUnknown); } + // Even when the unsigned overflow check above passes, the signed multiplications + // below can still overflow (which is undefined behavior on signed int). Re-check + // with signed semantics to keep the int min/max computation well-defined. + if (unsignedMul && (CheckedOps::MulOverflows(r1lo, r2lo, /* unsignedMul */ false) || + CheckedOps::MulOverflows(r1lo, r2hi, /* unsignedMul */ false) || + CheckedOps::MulOverflows(r1hi, r2lo, /* unsignedMul */ false) || + CheckedOps::MulOverflows(r1hi, r2hi, /* unsignedMul */ false))) + { + return Limit(Limit::keUnknown); + } + int lo = min(min(r1lo * r2lo, r1lo * r2hi), min(r1hi * r2lo, r1hi * r2hi)); int hi = max(max(r1lo * r2lo, r1lo * r2hi), max(r1hi * r2lo, r1hi * r2hi)); assert(hi >= lo); @@ -431,21 +442,67 @@ struct RangeOps return Multiply(r1, convertedOp2Range); } + // Compute a safe upper bound for the bitwise OR / XOR of two non-negative + // ranges with upper bounds 'b' and 'd': the smallest 2^k - 1 mask that is + // >= max(b, d). Bits in the result can be at most as high as the top bit + // of max(b, d). + // + // NOTE: max(b, d) must be non-negative (i.e., fit in a 31-bit signed int). + // + static int BitwiseUpperBoundForNonNegative(int b, int d) + { + unsigned m = static_cast(max(b, d)); + m |= m >> 1; + m |= m >> 2; + m |= m >> 4; + m |= m >> 8; + m |= m >> 16; + // m is now of the form 2^k - 1 with m >= max(b, d). Since max(b, d) <= INT32_MAX, + // the result m <= INT32_MAX, so the cast back to signed is safe. + return static_cast(m); + } + static Range Or(const Range& r1, const Range& r2) { - // For OR we require both operands to be constant to produce a constant result. - // No useful information can be derived if only one operand is constant. + int r1ConstVal; + int r2ConstVal; + bool r1IsConstVal = r1.IsSingleValueConstant(&r1ConstVal); + bool r2IsConstVal = r2.IsSingleValueConstant(&r2ConstVal); + + // Both single constants - compute the exact result. + if (r1IsConstVal && r2IsConstVal) + { + return Range(Limit(Limit::keConstant, r1ConstVal | r2ConstVal)); + } + + if (!r1.IsConstantRange() || !r2.IsConstantRange()) + { + return Range(Limit(Limit::keUnknown)); + } + + int a = r1.LowerLimit().GetConstant(); + int b = r1.UpperLimit().GetConstant(); + int c = r2.LowerLimit().GetConstant(); + int d = r2.UpperLimit().GetConstant(); + + // Signed cases get hairy. + if ((a < 0) || (c < 0)) + { + return Range(Limit(Limit::keUnknown)); + } + + // For non-negative ranges: + // Lower bound: x | y >= max(x, y), so result >= max(a, c). + // Upper bound: x | y can have at most as many bits set as the highest bit of + // max(b, d). Conservatively round up to the next "all ones" mask. // - // Example: [0..3] | [1..255] = [1..255] - // [X..Y] | [1..255] = [unknown..unknown] + // NOTE: a naive `b | d` is unsound here. For example, [3..4] | [4..4] would + // give upper = 4|4 = 4, but 3|4 = 7 is also reachable. // - return ApplyRangeOp(r1, r2, [](const Limit& a, const Limit& b) { - if (a.IsConstant() && b.IsConstant() && (a.GetConstant() >= 0) && (b.GetConstant() >= 0)) - { - return Limit(Limit::keConstant, a.GetConstant() | b.GetConstant()); - } - return Limit(Limit::keUnknown); - }); + // Example: [3..5] | [4..7] = [max(3,4) .. mask(max(5,7))] = [4..7] + // + return Range(Limit(Limit::keConstant, max(a, c)), + Limit(Limit::keConstant, BitwiseUpperBoundForNonNegative(b, d))); } static Range And(const Range& r1, const Range& r2) @@ -623,10 +680,11 @@ struct RangeOps return Limit(Limit::keUnknown); } - // Keep it simple for now, check if 0 <= C < 31 + // Allow shift counts in [1..30]. Shift count 31 would produce 1 << 31 == INT_MIN + // (or UB pre-C++20), which the subsequent Multiply cannot use safely. int r1loConstant = r1lo.GetConstant(); int r1hiConstant = r1hi.GetConstant(); - if (r1loConstant <= 0 || r1loConstant > 31 || r1hiConstant <= 0 || r1hiConstant > 31) + if ((r1loConstant <= 0) || (r1loConstant > 30) || (r1hiConstant <= 0) || (r1hiConstant > 30)) { return Limit(Limit::keUnknown); } From c916cdcbc4202c96e47c02275fb85f283b30503e Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 12 May 2026 04:01:42 +0200 Subject: [PATCH 2/3] Reword 1 << 31 comment per review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/rangecheck.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 60ca38b7b7b919..90202b3b3be643 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -680,8 +680,9 @@ struct RangeOps return Limit(Limit::keUnknown); } - // Allow shift counts in [1..30]. Shift count 31 would produce 1 << 31 == INT_MIN - // (or UB pre-C++20), which the subsequent Multiply cannot use safely. + // Allow shift counts in [1..30]. Shift count 31 can produce implementation-defined + // codegen or UB for `1 << 31`, and the resulting INT_MIN cannot be used safely by + // the subsequent Multiply. int r1loConstant = r1lo.GetConstant(); int r1hiConstant = r1hi.GetConstant(); if ((r1loConstant <= 0) || (r1loConstant > 30) || (r1hiConstant <= 0) || (r1hiConstant > 30)) From e42cdb61a359da60e7c00334cfed3ed9c6425f06 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 12 May 2026 04:19:03 +0200 Subject: [PATCH 3/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/coreclr/jit/rangecheck.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 90202b3b3be643..6a5c1330c2ef14 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -394,10 +394,10 @@ struct RangeOps // Even when the unsigned overflow check above passes, the signed multiplications // below can still overflow (which is undefined behavior on signed int). Re-check // with signed semantics to keep the int min/max computation well-defined. - if (unsignedMul && (CheckedOps::MulOverflows(r1lo, r2lo, /* unsignedMul */ false) || - CheckedOps::MulOverflows(r1lo, r2hi, /* unsignedMul */ false) || - CheckedOps::MulOverflows(r1hi, r2lo, /* unsignedMul */ false) || - CheckedOps::MulOverflows(r1hi, r2hi, /* unsignedMul */ false))) + if (unsignedMul && (CheckedOps::MulOverflows(r1lo, r2lo, CheckedOps::Signed) || + CheckedOps::MulOverflows(r1lo, r2hi, CheckedOps::Signed) || + CheckedOps::MulOverflows(r1hi, r2lo, CheckedOps::Signed) || + CheckedOps::MulOverflows(r1hi, r2hi, CheckedOps::Signed))) { return Limit(Limit::keUnknown); }