JIT: port extra check to struct of struct of x promotion to relase/2.1

Port of #19156.

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.

diff --git a/src/jit/lclvars.cpp b/src/jit/lclvars.cpp
index 765a2ee..866b711 100644 (file)
--- a/src/jit/lclvars.cpp
+++ b/src/jit/lclvars.cpp
@@ -1626,6 +1626,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;
                 }
 

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 (file)
index 0000000..e62ee0b
--- /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<CommandBytes>
+{
+    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<CommandBytes, string>();
+        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<T>(string caption, Func<CommandBytes, T> func)
+                    {
+                        T x = func(hunt), y = func(key);
+                        Console.WriteLine($"{caption}: {EqualityComparer<T>.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<CommandBytes>.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 (file)
index 0000000..f8ba499
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.csproj
@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType></DebugType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>