From c18d00cc64c42e56d00e3fa40cbca0387b494f9d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 26 Mar 2025 18:44:14 +0100 Subject: [PATCH 1/6] RangeCheck: Don't create invalid ranges --- src/coreclr/jit/rangecheck.cpp | 11 +++++++++++ src/coreclr/jit/rangecheck.h | 27 +++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 277ec31f6fb2e6..4eabf46024cd35 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -143,6 +143,7 @@ int RangeCheck::GetArrLength(ValueNum vn) bool RangeCheck::BetweenBounds(Range& range, GenTree* upper, int arrSize) { #ifdef DEBUG + assert(range.IsValid() || !"BetweenBounds: range is invalid"); if (m_pCompiler->verbose) { printf("%s BetweenBounds <%d, ", range.ToString(m_pCompiler), 0); @@ -354,6 +355,8 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* Range arrLenRange = GetRangeWorker(block, bndsChk->GetArrayLength(), false DEBUGARG(0)); if (arrLenRange.LowerLimit().IsConstant()) { + assert(arrLenRange.IsValid() || !"OptimizeRangeCheck: range is invalid"); + // Lower known limit of ArrLen: const int lenLowerLimit = arrLenRange.LowerLimit().GetConstant(); @@ -394,6 +397,8 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* return; } + assert(range.IsValid() || !"OptimizeRangeCheck: range is invalid"); + // If upper or lower limit is found to be unknown (top), or it was found to // be unknown because of over budget or a deep search, then return early. if (range.UpperLimit().IsUnknown() || range.LowerLimit().IsUnknown()) @@ -1003,6 +1008,12 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, unreached(); }; + if (!assertedRange.IsValid()) + { + JITDUMP("assertedRange is invalid: [%s] - bail out\n", assertedRange.ToString(comp)); + return; + } + JITDUMP("Tightening pRange: [%s] with assertedRange: [%s] into ", pRange->ToString(comp), assertedRange.ToString(comp)); diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 8df2fc19ea791c..298137b8819ebd 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -217,7 +217,7 @@ struct Limit return false; } #ifdef DEBUG - const char* ToString(Compiler* comp) + const char* ToString(Compiler* comp) const { switch (type) { @@ -283,10 +283,30 @@ struct Range } #ifdef DEBUG - const char* ToString(Compiler* comp) + const char* ToString(Compiler* comp) const { return comp->printfAlloc("<%s, %s>", lLimit.ToString(comp), uLimit.ToString(comp)); } + + bool IsValid() const + { + // Lower limit should be less than or equal to upper limit. + // In case of Checked Bounds, we have assumptions that a Checked Bound is never negative, + // e.g. <$bnd + 10, 5> is not a valid range. + if ((lLimit.IsConstant() && uLimit.IsConstant()) || (lLimit.IsBinOpArray() && uLimit.IsConstant()) || + (lLimit.IsConstant() && uLimit.IsBinOpArray())) + { + return lLimit.GetConstant() <= uLimit.GetConstant(); + } + + // When both limits are BinOpArray, we check if their offsets are valid + if (lLimit.IsBinOpArray() && uLimit.IsBinOpArray() && lLimit.vn == uLimit.vn) + { + return lLimit.GetConstant() <= uLimit.GetConstant(); + } + + return true; + } #endif }; @@ -452,6 +472,9 @@ struct RangeOps // then ignore the dependent variables for the lower bound but not for the upper bound. static Range Merge(const Range& r1, const Range& r2, bool monIncreasing) { + assert(r1.IsValid() || !"Merge: r1 is invalid"); + assert(r2.IsValid() || !"Merge: r2 is invalid"); + const Limit& r1lo = r1.LowerLimit(); const Limit& r1hi = r1.UpperLimit(); const Limit& r2lo = r2.LowerLimit(); From ae19fa0908b6fa306699ab927e5d4effafefe979 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 26 Mar 2025 19:09:52 +0100 Subject: [PATCH 2/6] fix build --- src/coreclr/jit/rangecheck.cpp | 6 +++--- src/coreclr/jit/rangecheck.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 4eabf46024cd35..ff2902fd48c657 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -143,7 +143,7 @@ int RangeCheck::GetArrLength(ValueNum vn) bool RangeCheck::BetweenBounds(Range& range, GenTree* upper, int arrSize) { #ifdef DEBUG - assert(range.IsValid() || !"BetweenBounds: range is invalid"); + assert(range.IsValid()); if (m_pCompiler->verbose) { printf("%s BetweenBounds <%d, ", range.ToString(m_pCompiler), 0); @@ -355,7 +355,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* Range arrLenRange = GetRangeWorker(block, bndsChk->GetArrayLength(), false DEBUGARG(0)); if (arrLenRange.LowerLimit().IsConstant()) { - assert(arrLenRange.IsValid() || !"OptimizeRangeCheck: range is invalid"); + assert(arrLenRange.IsValid()); // Lower known limit of ArrLen: const int lenLowerLimit = arrLenRange.LowerLimit().GetConstant(); @@ -397,7 +397,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* return; } - assert(range.IsValid() || !"OptimizeRangeCheck: range is invalid"); + assert(range.IsValid()); // If upper or lower limit is found to be unknown (top), or it was found to // be unknown because of over budget or a deep search, then return early. diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 298137b8819ebd..db10a325a6a9a3 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -287,6 +287,7 @@ struct Range { return comp->printfAlloc("<%s, %s>", lLimit.ToString(comp), uLimit.ToString(comp)); } +#endif bool IsValid() const { @@ -307,7 +308,6 @@ struct Range return true; } -#endif }; // Helpers for operations performed on ranges @@ -472,8 +472,8 @@ struct RangeOps // then ignore the dependent variables for the lower bound but not for the upper bound. static Range Merge(const Range& r1, const Range& r2, bool monIncreasing) { - assert(r1.IsValid() || !"Merge: r1 is invalid"); - assert(r2.IsValid() || !"Merge: r2 is invalid"); + assert(r1.IsValid()); + assert(r2.IsValid()); const Limit& r1lo = r1.LowerLimit(); const Limit& r1hi = r1.UpperLimit(); From fa42bedfe425acbb3c5b92d1b4c26e2ae1db7636 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 26 Mar 2025 20:04:32 +0100 Subject: [PATCH 3/6] fix build --- src/coreclr/jit/rangecheck.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index db10a325a6a9a3..02a978b307beb6 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -294,8 +294,7 @@ struct Range // Lower limit should be less than or equal to upper limit. // In case of Checked Bounds, we have assumptions that a Checked Bound is never negative, // e.g. <$bnd + 10, 5> is not a valid range. - if ((lLimit.IsConstant() && uLimit.IsConstant()) || (lLimit.IsBinOpArray() && uLimit.IsConstant()) || - (lLimit.IsConstant() && uLimit.IsBinOpArray())) + if ((lLimit.IsConstant() && uLimit.IsConstant()) || (lLimit.IsBinOpArray() && uLimit.IsConstant())) { return lLimit.GetConstant() <= uLimit.GetConstant(); } From 44c71c3b6305337400e24300958398e221d10ac8 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 26 Mar 2025 23:34:56 +0100 Subject: [PATCH 4/6] fix one more case --- src/coreclr/jit/rangecheck.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index ff2902fd48c657..5c1140d2b70c33 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1017,10 +1017,20 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, JITDUMP("Tightening pRange: [%s] with assertedRange: [%s] into ", pRange->ToString(comp), assertedRange.ToString(comp)); - pRange->lLimit = tightenLimit(assertedRange.lLimit, pRange->lLimit, preferredBoundVN, true); - pRange->uLimit = tightenLimit(assertedRange.uLimit, pRange->uLimit, preferredBoundVN, false); + Range copy = *pRange; + copy.lLimit = tightenLimit(assertedRange.lLimit, copy.lLimit, preferredBoundVN, true); + copy.uLimit = tightenLimit(assertedRange.uLimit, copy.uLimit, preferredBoundVN, false); - JITDUMP("[%s]\n", pRange->ToString(comp)); + JITDUMP("[%s]\n", copy.ToString(comp)); + if (copy.IsValid()) + { + *pRange = copy; + } + else + { + JITDUMP("invalid range after tightening\n"); + return; + } } } From 06e2c00747ff029fb66e0c797529e9550fbfe174 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 27 Mar 2025 01:04:27 +0100 Subject: [PATCH 5/6] fix tests --- src/coreclr/jit/rangecheck.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 5c1140d2b70c33..e061c43fe932ca 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -643,6 +643,7 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP bool RangeCheck::TryGetRangeFromAssertions(Compiler* comp, ValueNum num, ASSERT_VALARG_TP assertions, Range* pRange) { MergeEdgeAssertions(comp, num, ValueNumStore::NoVN, assertions, pRange, false); + assert(pRange->IsValid()); return !pRange->LowerLimit().IsUnknown() || !pRange->UpperLimit().IsUnknown(); } @@ -1208,6 +1209,9 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool op2Range = *op2RangeCached; } + assert(op1Range.IsValid()); + assert(op2Range.IsValid()); + Range r = Range(Limit::keUnknown); if (binop->OperIs(GT_ADD)) { @@ -1235,6 +1239,13 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool JITDUMP("Right shift range: %s >> %s = %s\n", op1Range.ToString(m_pCompiler), op2Range.ToString(m_pCompiler), r.ToString(m_pCompiler)); } + + // Some binops may produce invalid ranges, e.g. <0, 1> * <-1, -1> = <0, -1> + if (!r.IsValid()) + { + JITDUMP("BinOp range is invalid: %s\n", r.ToString(m_pCompiler)); + return Range(Limit::keUnknown); + } return r; } @@ -1705,6 +1716,7 @@ bool RangeCheck::TryGetRange(BasicBlock* block, GenTree* expr, Range* pRange) ClearSearchPath(); Range range = GetRangeWorker(block, expr, false DEBUGARG(0)); + assert(range.IsValid()); if (range.UpperLimit().IsUnknown() && range.LowerLimit().IsUnknown()) { JITDUMP("Range is completely unknown.\n"); From 90a315cab853725894468f63b5b861214a0d581c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 12 Jun 2025 19:04:22 +0200 Subject: [PATCH 6/6] small clean up in IsValid --- src/coreclr/jit/rangecheck.h | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 02a978b307beb6..45e88633f14d88 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -291,10 +291,8 @@ struct Range bool IsValid() const { - // Lower limit should be less than or equal to upper limit. - // In case of Checked Bounds, we have assumptions that a Checked Bound is never negative, - // e.g. <$bnd + 10, 5> is not a valid range. - if ((lLimit.IsConstant() && uLimit.IsConstant()) || (lLimit.IsBinOpArray() && uLimit.IsConstant())) + // A valid range must have lower limit <= upper limit. + if (lLimit.IsConstant() && uLimit.IsConstant()) { return lLimit.GetConstant() <= uLimit.GetConstant(); } @@ -305,6 +303,12 @@ struct Range return lLimit.GetConstant() <= uLimit.GetConstant(); } + // e.g. <$bnd + 10, 5> is not a valid range since $bnd is expected to be >= 0 + if (lLimit.IsBinOpArray() && uLimit.IsConstant()) + { + return lLimit.GetConstant() <= uLimit.GetConstant(); + } + return true; } }; @@ -661,6 +665,9 @@ struct RangeOps // static RelationKind EvalRelop(const genTreeOps relop, bool isUnsigned, const Range& x, const Range& y) { + assert(x.IsValid()); + assert(y.IsValid()); + const Limit& xLower = x.LowerLimit(); const Limit& yLower = y.LowerLimit(); const Limit& xUpper = x.UpperLimit();