diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index d053476b69c905..7ae3e2118ad165 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1677,6 +1677,13 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp else { JITDUMP("invalid range after tightening\n"); + // The tightened range is empty, i.e. the assertions contradict the range we computed. + // If the assertion set really did hold here the block would be unreachable, but we get + // here with sets that do not hold at this point (e.g. assertions of a use block merged + // into the range of a definition that lives in another block). Returning while leaving + // the previously tightened range in place lets a fact that has just been disproven + // escape to the caller, which then folds branches with it. Bail out to Unknown instead. + *pRange = Range(Limit(Limit::keUnknown)); return; } } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_132879/Runtime_132879.cs b/src/tests/JIT/Regression/JitBlue/Runtime_132879/Runtime_132879.cs new file mode 100644 index 00000000000000..5d1d64c2dcd452 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_132879/Runtime_132879.cs @@ -0,0 +1,330 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Regression test for https://github.com/dotnet/runtime/issues/132879 +// +// RangeCheck::MergeEdgeAssertionsWorker tightens a range with the incoming assertions. When +// tightening produced an empty range it returned early and left the previously tightened +// range in place, so a fact the assertions had just disproven escaped to the caller. +// Assertion propagation then folded a branch with it: here the range of "op" was computed as +// [5..7] even though "op" is EqualUserExternalId (78), so the "op > 69" test guarding the +// operator switch folded to false and execution fell into the default case. +// +// The method needs to be compiled with full opts and no PGO data (which is what crossgen2 +// does), hence AggressiveOptimization; with tier1 profile data the bad range is not formed. + +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using System.Text; +using Xunit; + +#nullable enable + +namespace Runtime_132879 +{ + public enum FilterConditionOperator + { + Equal = 2, + NotEqual = 3, + GreaterThan = 4, + LessThan = 5, + GreaterEqual = 6, + LessEqual = 7, + Like = 8, + NotLike = 9, + Between = 12, + NotBetween = 13, + Null = 14, + NotNull = 15, + EqualUserId = 43, + NotEqualUserId = 44, + BeginsWith = 64, + DoesNotBeginWith = 65, + DoesNotEndWith = 66, + EndsWith = 67, + Contains = 68, + DoesNotContain = 69, + EqualUserExternalId = 78, + NotEqualUserExternalId = 79, + EqualDeviceId = 86, + NotEqualDeviceId = 87, + } + + // Minimal stand-ins for the types the original method depended on. They exist only to keep + // the control flow of ConditionBuilder intact. + + public enum MobileObjectType { Text, DateTime, Date, Other } + public enum SqlSyntaxType { MicrosoftSQLServer, SQLite } + public enum SqlCommandType { Select, Insert, Update, Delete } + + public class ObjectAlias + { + public string? Alias; + public string ObjectName = ""; + public string SqlAlias = ""; + } + + public class FilterCondition + { + public string PropertyName = ""; + public string? Alias; + public FilterConditionOperator Operator; + public string[]? Values; + + // In the original, returns a date range, or null if the operator is not a date operator. + public Tuple? GetDateRange(DateTime prepareDate, object? storageTimeZone) => null; + } + + public class DateTimeTypeInfo + { + public object? StorageTimeZone; + } + + public class MobileObjectProperty + { + + public MobileObjectType Type; + public string? ExternalType; + public DateTimeTypeInfo? TypeInfoAsDateTime; + + public static object DeserializeValue(MobileObjectType type, string s) => (object)DateTime.Parse(s); + public static string SerializeValue(MobileObjectType type, DateTime dt) => dt.ToString("O"); + } + public class MobileHostInfo + { + public string DeviceId = "DEV-1"; + } + + public class MetadataBindingInfo + { + public MobileHostInfo MobileHost = new(); + public string UserId = "U-1"; + public string UserExternalId = "EXT-1"; + public string GetSqlPropertyName(string objectName, string propertyName) => propertyName; + } + + public class MobileException : Exception + { + private MobileException(string msg) : base(msg) { } + public static MobileException CreateError(string msg, string trace) => new MobileException(msg + " | TRACE: " + trace); + public static Exception CreateException(string msg, Exception inner) => new MobileException(msg + " -> " + inner.Message); + } + + public class Repro + { + public MetadataBindingInfo? MetadataBinding = new MetadataBindingInfo(); + public SqlSyntaxType SqlSyntax = SqlSyntaxType.MicrosoftSQLServer; + public SqlCommandType CommandType = SqlCommandType.Select; + private readonly DateTime _prepareDate = DateTime.UtcNow; + + // Stand-in for a helper method used by the original method (defined elsewhere in the + // real class) - for our test scenario it always returns a valid object. + private MobileObjectProperty? GetMobileObjectProperty(string objectName, List validPropertyNames, Dictionary? propertyBinding, ref string propertyName) + { + return new MobileObjectProperty { Type = MobileObjectType.Other, ExternalType = "varchar" }; + } + + private void AppendField(StringBuilder sb, string tabAlias, string sqlPropertyName, string? targetType) + { + sb.Append('[').Append(tabAlias).Append('.').Append(sqlPropertyName).Append(']'); + } + + private bool ConditionBuilder_DateRange(StringBuilder sb, MobileObjectProperty mop, Tuple? dateRange, Action appendField) + { + // Not exercised in our test scenario (op is not a date operator) - always false. + return false; + } + + private string InsertParameter(object value) => "@p_" + value; + private string InsertSerializedParameter(MobileObjectProperty mop, string? val) => "@p_" + val; + [MethodImpl(MethodImplOptions.AggressiveOptimization | MethodImplOptions.NoInlining)] + public string? ConditionBuilder(string objectName, IReadOnlyList aliases, string tabAlias, FilterCondition fc, List validPropertyNames, Dictionary? propertyBinding = null) + { + try + { + var sb = new StringBuilder(); + var propertyName = fc.PropertyName; + if (MetadataBinding != null) + { + var op = fc.Operator; + var vals = fc.Values; + var mop = GetMobileObjectProperty(objectName, validPropertyNames, propertyBinding, ref propertyName); + if (mop == null) return null; + bool isNull = false; + bool isNotNull = false; + switch (op) + { + case FilterConditionOperator.Null: isNull = true; break; + case FilterConditionOperator.NotNull: isNotNull = true; break; + default: + if (vals != null && vals.Length == 1 && (vals[0] == null || mop.Type != MobileObjectType.Text && string.IsNullOrEmpty(vals[0]))) + { + // if the value stands for the null operator + // if '=' then check for NULL + // TODO: option to skip the operator for this comparison + if (op == FilterConditionOperator.Equal) isNull = true; + else return null; // skip this fragment + } + break; + } + string? targetType = null; + void AppendCurrentField(StringBuilder sbForField) + { + AppendField(sbForField, tabAlias, MetadataBinding.GetSqlPropertyName(objectName, propertyName), targetType); + } + + AppendCurrentField(sb); + var matched = true; + if (isNull) sb.Append(" IS NULL"); + else if (isNotNull) sb.Append(" IS NOT NULL"); + else matched = false; + if (!matched) + { + var dateRange = fc.GetDateRange(_prepareDate, mop.TypeInfoAsDateTime?.StorageTimeZone); + if (dateRange != null) + { + matched = ConditionBuilder_DateRange(sb, mop, dateRange, AppendCurrentField); + } + } + if (!matched) + { + // range correction may be needed + if (mop.Type == MobileObjectType.DateTime && vals != null) + { + if (op == FilterConditionOperator.LessEqual && vals.Length == 1) + { + // if only a date on the right-hand side + if (vals[0].Length == 10) // YYYY-MM-DD - date without time + { + var dt = (DateTime)MobileObjectProperty.DeserializeValue(MobileObjectType.Date, vals[0]); + op = FilterConditionOperator.LessThan; + vals = new[] { MobileObjectProperty.SerializeValue(MobileObjectType.Date, dt.AddDays(1)) }; + } + } + else if ((op == FilterConditionOperator.Between || op == FilterConditionOperator.NotBetween) && vals.Length == 2) + { + if (vals[1].Length == 10) // YYYY-MM-DD - date without time + { + vals = new[] { vals[0], vals[1] + "T23:59:59.9999999" }; + } + } + } + } + if (!matched) + { + matched = true; + switch (op) + { + case FilterConditionOperator.EqualUserId: + case FilterConditionOperator.EqualUserExternalId: + case FilterConditionOperator.Equal: + case FilterConditionOperator.EqualDeviceId: + sb.Append('='); break; + case FilterConditionOperator.NotEqual: + case FilterConditionOperator.NotEqualUserId: + case FilterConditionOperator.NotEqualUserExternalId: + case FilterConditionOperator.NotEqualDeviceId: + sb.Append("<>"); break; + case FilterConditionOperator.GreaterThan: sb.Append('>'); break; + case FilterConditionOperator.LessThan: sb.Append('<'); break; + case FilterConditionOperator.GreaterEqual: sb.Append(">="); break; + case FilterConditionOperator.LessEqual: sb.Append("<="); break; + case FilterConditionOperator.Like: + case FilterConditionOperator.BeginsWith: + case FilterConditionOperator.EndsWith: + case FilterConditionOperator.Contains: + sb.Append(" LIKE "); + break; + case FilterConditionOperator.NotLike: + case FilterConditionOperator.DoesNotBeginWith: + case FilterConditionOperator.DoesNotEndWith: + case FilterConditionOperator.DoesNotContain: + sb.Append(" NOT LIKE "); + break; + default: matched = false; break; + } + if (matched) + { + switch (op) + { + case FilterConditionOperator.EqualUserId: + case FilterConditionOperator.NotEqualUserId: + sb.Append(InsertParameter(MetadataBinding.UserId)); + break; + case FilterConditionOperator.EqualUserExternalId: + case FilterConditionOperator.NotEqualUserExternalId: + sb.Append(InsertParameter(MetadataBinding.UserExternalId)); + break; + case FilterConditionOperator.BeginsWith: + case FilterConditionOperator.DoesNotBeginWith: + sb.Append(InsertSerializedParameter(mop, vals![0] + "%")); + break; + case FilterConditionOperator.EndsWith: + case FilterConditionOperator.DoesNotEndWith: + sb.Append(InsertSerializedParameter(mop, "%" + vals![0])); + break; + case FilterConditionOperator.Contains: + case FilterConditionOperator.DoesNotContain: + sb.Append(InsertSerializedParameter(mop, "%" + vals![0] + "%")); + break; + case FilterConditionOperator.EqualDeviceId: + case FilterConditionOperator.NotEqualDeviceId: + sb.Append(InsertParameter(MetadataBinding.MobileHost.DeviceId)); + break; + default: + sb.Append(InsertSerializedParameter(mop, vals![0])); + break; + } + } + } + if (!matched) + { + throw MobileException.CreateError($"Unknown FilterConditionOperator: {op.ToString()} ({(int)op}), partial condition={sb}", ""); + } + } + else + { + sb.Append(propertyName); + sb.Append(' '); + sb.Append(fc.Operator); // quick workaround - sufficient for comparison + sb.Append(' '); + if (fc.Values != null) + { + foreach (var v in fc.Values) + { + sb.Append('\''); + sb.Append(v.Replace("'", "''")); + sb.Append('\''); + } + } + } + return sb.ToString(); + } + catch (Exception ex) + { + throw MobileException.CreateException($"Condition: '{objectName}'.'{fc?.PropertyName}' {fc?.Operator}", ex); + } + } + } + + public class Runtime_132879 + { + [Fact] + public static void TestEntryPoint() + { + var repro = new Repro(); + var fc = new FilterCondition + { + PropertyName = "ExternalId", + Alias = null, + Operator = FilterConditionOperator.EqualUserExternalId, + Values = null + }; + + string? result = repro.ConditionBuilder("MyObject", new List(), "t0", fc, new List { "ExternalId" }); + + Assert.Equal("[t0.ExternalId]=@p_EXT-1", result); + } + } +} diff --git a/src/tests/JIT/Regression/Regression_ro_2.csproj b/src/tests/JIT/Regression/Regression_ro_2.csproj index f45592f1d391a5..f74bd0a2cd534e 100644 --- a/src/tests/JIT/Regression/Regression_ro_2.csproj +++ b/src/tests/JIT/Regression/Regression_ro_2.csproj @@ -132,6 +132,7 @@ +