From 3ed53a21fdea4805e6191a502238356fd06d17ff Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 26 Jul 2018 17:07:07 -0700 Subject: [PATCH 1/2] JIT: add extra check to struct of struct of x promotion Avoid promoting structs that contain struct fields that themselves wrap single simple fields, if those single simple fields are smaller than their enclosing struct. Otherwise we run the risk of losing track of the "extra" bytes in the innner struct. Addresses #19149. --- src/jit/lclvars.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/jit/lclvars.cpp b/src/jit/lclvars.cpp index 9e83d11776ca..9fee611bc398 100644 --- a/src/jit/lclvars.cpp +++ b/src/jit/lclvars.cpp @@ -1656,6 +1656,18 @@ void Compiler::lvaCanPromoteStructType(CORINFO_CLASS_HANDLE typeHnd, // natural boundary. if (fieldSize == 0 || fieldSize != TARGET_POINTER_SIZE || varTypeIsFloating(fieldVarType)) { + JITDUMP("Promotion blocked: struct contains struct field with one field," + " but that field has invalid size or type"); + return; + } + + // Insist this wrapped field occupy all of its parent storage. + unsigned innerStructSize = info.compCompHnd->getClassSize(pFieldInfo->fldTypeHnd); + + if (fieldSize != innerStructSize) + { + JITDUMP("Promotion blocked: struct contains struct field with one field," + "but that field is not the same size as its parent."); return; } From c922e69c80a6addc759d622cf5ae376afdb1c4b6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 26 Jul 2018 19:07:27 -0700 Subject: [PATCH 2/2] add test case; fix typo --- src/jit/lclvars.cpp | 2 +- .../JitBlue/GitHub_19022/GitHub_19022.cs | 4 + .../JitBlue/GitHub_19149/GitHub_19149.cs | 249 ++++++++++++++++++ .../JitBlue/GitHub_19149/GitHub_19149.csproj | 35 +++ 4 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.csproj diff --git a/src/jit/lclvars.cpp b/src/jit/lclvars.cpp index 9fee611bc398..96aa09021359 100644 --- a/src/jit/lclvars.cpp +++ b/src/jit/lclvars.cpp @@ -1667,7 +1667,7 @@ void Compiler::lvaCanPromoteStructType(CORINFO_CLASS_HANDLE typeHnd, if (fieldSize != innerStructSize) { JITDUMP("Promotion blocked: struct contains struct field with one field," - "but that field is not the same size as its parent."); + " but that field is not the same size as its parent."); return; } diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19022/GitHub_19022.cs b/tests/src/JIT/Regression/JitBlue/GitHub_19022/GitHub_19022.cs index cd4593256ba0..c5d1a8958615 100644 --- a/tests/src/JIT/Regression/JitBlue/GitHub_19022/GitHub_19022.cs +++ b/tests/src/JIT/Regression/JitBlue/GitHub_19022/GitHub_19022.cs @@ -1,3 +1,7 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + using System; using System.Numerics; diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.cs b/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.cs new file mode 100644 index 000000000000..e62ee0bb1e0f --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.cs @@ -0,0 +1,249 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +// Bug report thanks to @mgravell +// +// JIT bug affecting how fixed buffers are handled +// +// Affects: netcoreapp2.1, debug and release +// Does not seem to affect: netcoreapp2.0, net47 +// +// the idea behind CommandBytes is that it is a fixed-sized string-like thing +// used for matching commands; it is *implemented* as a fixed buffer +// of **longs**, but: the first byte of the first element is coerced into +// a byte and used to store the length; the actual text payload (ASCII) +// starts at the second byte of the first element +// +// as far as I can tell, it is all validly implemented, and it works fine +// in isolation, however: when used in a dictionary, it goes bad; +// - items not being found despite having GetHashCode and Equals match +// - items over 1 chunk size becoming corrupted (see: ToInnerString) +// +// however, if I replace the fixed buffer with the same number of +// regular fields (_c0,_c1,_c2) and use *all the same code*, it +// all works correctly! +// +// The "Main" method populates a dictionary in the expected way, +// then attempts to find things - either via TryGetValue or manually; +// it then compares the contents +// +// Yes, this code is evil; it is for a very specific optimized scenario. + +using System; +using System.Collections.Generic; +using System.Text; + +unsafe struct CommandBytes : IEquatable +{ + private const int ChunkLength = 3; + public const int MaxLength = (ChunkLength * 8) - 1; + + fixed long _chunks[ChunkLength]; + + public override int GetHashCode() + { + fixed (long* lPtr = _chunks) + { + var hashCode = -1923861349; + long* x = lPtr; + for (int i = 0; i < ChunkLength; i++) + { + hashCode = hashCode * -1521134295 + (*x++).GetHashCode(); + } + return hashCode; + } + } + + public override string ToString() + { + fixed (long* lPtr = _chunks) + { + var bPtr = (byte*)lPtr; + return Encoding.ASCII.GetString(bPtr + 1, bPtr[0]); + } + } + public int Length + { + get + { + fixed (long* lPtr = _chunks) + { + var bPtr = (byte*)lPtr; + return bPtr[0]; + } + } + } + public byte this[int index] + { + get + { + fixed (long* lPtr = _chunks) + { + byte* bPtr = (byte*)lPtr; + int len = bPtr[0]; + if (index < 0 || index >= len) throw new IndexOutOfRangeException(); + return bPtr[index + 1]; + } + } + } + + public CommandBytes(string value) + { + value = value.ToLowerInvariant(); + var len = Encoding.ASCII.GetByteCount(value); + if (len > MaxLength) throw new ArgumentOutOfRangeException("Maximum command length exceeed"); + + fixed (long* lPtr = _chunks) + { + Clear(lPtr); + byte* bPtr = (byte*)lPtr; + bPtr[0] = (byte)len; + fixed (char* cPtr = value) + { + Encoding.ASCII.GetBytes(cPtr, value.Length, bPtr + 1, len); + } + } + } + public override bool Equals(object obj) => obj is CommandBytes cb && Equals(cb); + + public string ToInnerString() + { + fixed (long* lPtr = _chunks) + { + long* x = lPtr; + var sb = new StringBuilder(); + for (int i = 0; i < ChunkLength; i++) + { + if (sb.Length != 0) sb.Append(','); + sb.Append(*x++); + } + return sb.ToString(); + } + } + public bool Equals(CommandBytes value) + { + fixed (long* lPtr = _chunks) + { + long* x = lPtr; + long* y = value._chunks; + for (int i = 0; i < ChunkLength; i++) + { + if (*x++ != *y++) return false; + } + return true; + } + } + private static void Clear(long* ptr) + { + for (int i = 0; i < ChunkLength; i++) + { + *ptr++ = 0L; + } + } +} + +static class Program +{ + static int Main() + { + var lookup = new Dictionary(); + void Add(string val) + { + var cb = new CommandBytes(val); + // prove we didn't screw up + if (cb.ToString() != val) + throw new InvalidOperationException("oops!"); + lookup.Add(cb, val); + } + Add("client"); + Add("cluster"); + Add("command"); + Add("config"); + Add("dbsize"); + Add("decr"); + Add("del"); + Add("echo"); + Add("exists"); + Add("flushall"); + Add("flushdb"); + Add("get"); + Add("incr"); + Add("incrby"); + Add("info"); + Add("keys"); + Add("llen"); + Add("lpop"); + Add("lpush"); + Add("lrange"); + Add("memory"); + Add("mget"); + Add("mset"); + Add("ping"); + Add("quit"); + Add("role"); + Add("rpop"); + Add("rpush"); + Add("sadd"); + Add("scard"); + Add("select"); + Add("set"); + Add("shutdown"); + Add("sismember"); + Add("spop"); + Add("srem"); + Add("strlen"); + Add("subscribe"); + Add("time"); + Add("unlink"); + Add("unsubscribe"); + + bool HuntFor(string lookFor) + { + Console.WriteLine($"Looking for: '{lookFor}'"); + var hunt = new CommandBytes(lookFor); + bool result = lookup.TryGetValue(hunt, out var found); + + if (result) + { + Console.WriteLine($"Found via TryGetValue: '{found}'"); + } + else + { + Console.WriteLine("**NOT FOUND** via TryGetValue"); + } + + Console.WriteLine("looking manually"); + foreach (var pair in lookup) + { + if (pair.Value == lookFor) + { + Console.WriteLine($"Found manually: '{pair.Value}'"); + var key = pair.Key; + void Compare(string caption, Func func) + { + T x = func(hunt), y = func(key); + Console.WriteLine($"{caption}: {EqualityComparer.Default.Equals(x, y)}, '{x}' vs '{y}'"); + } + Compare("GetHashCode", _ => _.GetHashCode()); + Compare("ToString", _ => _.ToString()); + Compare("Length", _ => _.Length); + Compare("ToInnerString", _ => _.ToInnerString()); + Console.WriteLine($"Equals: {key.Equals(hunt)}, {hunt.Equals(key)}"); + var eq = EqualityComparer.Default; + + Console.WriteLine($"EqualityComparer: {eq.Equals(key, hunt)}, {eq.Equals(hunt, key)}"); + Compare("eq GetHashCode", _ => eq.GetHashCode(_)); + } + } + Console.WriteLine(); + + return result; + } + + bool result1 = HuntFor("ping"); + bool result2 = HuntFor("subscribe"); + + return (result1 && result2) ? 100 : -1; + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.csproj new file mode 100644 index 000000000000..f8ba4995e0d7 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + False + + + + + true + True + + + + + + + + + +