Do not promote struct field with type mistmatch. (dotnet/coreclr#20085)

* Add a test that reads int field as double.

It fails in `void CodeGen::genCodeForStoreInd(GenTreeStoreInd* tree)`  on `assert(!varTypeIsFloating(targetType) || (targetType == data->TypeGet()));`.

* Fix for the previous test.

No asm diffs.

* Add a test that reads struct field as another struct type.

It fails with assert `Compiler::StructPromotionHelper::CheckFakedType` `assert(fakedFieldsMap.Lookup(fieldHnd));`.

* Fix the previous test.

Check that we promote with the same class.

* Add a test case for accessing an invalid offset.

It fails in `fgMorphStructField` with `noway_assert(fieldLclIndex != BAD_VAR_NUM);`.

* Fix the previous test.

Reject field promotion if offset is invalid. No asm diffs.

Commit migrated from https://github.com/dotnet/coreclr/commit/13dc153a7718a6d9297b757f5969c403856ef618

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
index f928479..080a0fb 100644 (file)
--- a/src/coreclr/src/jit/morph.cpp
+++ b/src/coreclr/src/jit/morph.cpp
@@ -17123,7 +17123,13 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent)
                 // Promoted struct
                 unsigned fldOffset     = field->gtFldOffset;
                 unsigned fieldLclIndex = lvaGetFieldLocal(varDsc, fldOffset);
-                noway_assert(fieldLclIndex != BAD_VAR_NUM);
+
+                if (fieldLclIndex == BAD_VAR_NUM)
+                {
+                    // Access a promoted struct's field with an offset that doesn't correspond to any field.
+                    // It can happen if the struct was cast to another struct with different offsets.
+                    return;
+                }
 
                 const LclVarDsc* fieldDsc  = &lvaTable[fieldLclIndex];
                 var_types        fieldType = fieldDsc->TypeGet();
@@ -17135,11 +17141,24 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent)
                     {
                         // This is going to be an incorrect instruction promotion.
                         // For example when we try to read int as long.
-                        // Tolerate this case now to keep no asm difs in this PR.
-                        // TODO make a return.
+                        return;
+                    }
+
+                    if (field->gtFldHnd != fieldDsc->lvFieldHnd)
+                    {
+                        CORINFO_CLASS_HANDLE fieldTreeClass = nullptr, fieldDscClass = nullptr;
+
+                        CorInfoType fieldTreeType = info.compCompHnd->getFieldType(field->gtFldHnd, &fieldTreeClass);
+                        CorInfoType fieldDscType = info.compCompHnd->getFieldType(fieldDsc->lvFieldHnd, &fieldDscClass);
+                        if (fieldTreeType != fieldDscType || fieldTreeClass != fieldDscClass)
+                        {
+                            // Access the promoted field with a different class handle, can't check that types match.
+                            return;
+                        }
+                        // Access the promoted field as a field of a non-promoted struct with the same class handle.
                     }
 #ifdef DEBUG
-                    if (tree->TypeGet() == TYP_STRUCT)
+                    else if (tree->TypeGet() == TYP_STRUCT)
                     {
                         // The field tree accesses it as a struct, but the promoted lcl var for the field
                         // says that it has another type. It can happen only if struct promotion faked

diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.cs b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.cs
new file mode 100644 (file)
index 0000000..2c03529
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.cs
@@ -0,0 +1,62 @@
+// 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.
+
+// The test came from #20085. It tries to access field from a promoted struct with an offset that 
+// is not valid for the promoted struct type.
+
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+using System.Diagnostics;
+using System;
+
+class TestStructAccessThroughRef
+{
+
+    [StructLayout(LayoutKind.Explicit)]
+    struct NotPromotedStruct
+    {
+
+        [FieldOffset(0)]
+        public long anotherField;
+        [FieldOffset(4)] // Offset doesn't correspond to a valid offset in PromotedStructSize.
+        public long overlappingField;
+
+        public static ref PromotedStruct AsPromotedStructSize20(ref NotPromotedStruct d) => ref Unsafe.As<NotPromotedStruct, PromotedStruct>(ref d);
+    }
+
+    [StructLayout(LayoutKind.Explicit)]
+    struct PromotedStruct
+    {
+        [FieldOffset(0)]
+        public long anotherField;
+        [FieldOffset(8)]
+        public int smallField;
+
+        public static ref NotPromotedStruct AsNotPromotedStruct(ref PromotedStruct d) => ref Unsafe.As<PromotedStruct, NotPromotedStruct>(ref d);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void TestStructCasts()
+    {
+        PromotedStruct a = new PromotedStruct(); // Addr-exposed, cannot be independent promoted.
+        a.anotherField = 5;
+        a.smallField = 6;
+
+        Debug.Assert(PromotedStruct.AsNotPromotedStruct(ref a).anotherField == 0x5);
+
+        // This access will ask LclVariable with type of `PromotedStruct` about the field with offset == 4, that doesn't exist there.
+        Debug.Assert(PromotedStruct.AsNotPromotedStruct(ref a).overlappingField == 0x600000000);
+        a.smallField = 6;
+        Debug.Assert(PromotedStruct.AsNotPromotedStruct(ref a).overlappingField == 0x600000000);
+        PromotedStruct.AsNotPromotedStruct(ref a).overlappingField = 0x700000000;
+        Debug.Assert(a.smallField == 0x7);
+    }
+
+    static int Main()
+    {
+        TestStructCasts();
+        return 100;
+    }
+
+}

diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.csproj b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.csproj
new file mode 100644 (file)
index 0000000..b25553e
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/AccessInvalidFieldOffset.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>{9B2CCDC2-D6CE-4568-A419-AC79613BC235}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+       <LangVersion>latest</LangVersion>
+  </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>None</DebugType>
+    <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>

diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.cs b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.cs
new file mode 100644 (file)
index 0000000..f81e879
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.cs
@@ -0,0 +1,88 @@
+// 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.
+
+// The test came from #20085. It tests that we do access overlapping fields with the correct types. 
+// Espessialy if the stuct was casted by 'Unsafe.As` from a promoted type
+// and the promoted type had another field on the same offset but with a different type/size.
+
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+using System.Diagnostics;
+using System;
+
+class TestReadIntAsDouble
+{
+       private struct Dec
+       {
+               public int uflags;
+               public int uhi;
+               public int ulo;
+               public int umid;
+       }
+
+       [StructLayout(LayoutKind.Explicit)]
+       private struct DecCalc1
+       {
+               [FieldOffset(0)]
+               public int uflags;
+               [FieldOffset(4)]
+               public int uhi;
+               [FieldOffset(8)]
+               public int ulo;
+               [FieldOffset(12)]
+               public int umid;
+               [FieldOffset(8)]
+               public double ulomidLE;
+       }
+
+       public struct Data
+       {
+               public int x, y, z;
+               public double m;
+       }
+
+
+       [MethodImpl(MethodImplOptions.NoInlining)]
+       public static void TestDoubleAssignment(Data d)
+       {
+               Dec p = default;
+               p.ulo = d.x;
+               p.umid = d.y;
+        // The jit gets field's type based on offset, so it will return `ulo` as int.
+        d.m = Unsafe.As<Dec, DecCalc1>(ref p).ulomidLE; 
+       }
+
+    [StructLayout(LayoutKind.Explicit)]
+    private struct DecCalc2
+    {
+        [FieldOffset(0)]
+        public int uflags;
+        [FieldOffset(4)]
+        public int uhi;
+        [FieldOffset(8)]
+        public double ulomidLE;
+        [FieldOffset(8)]
+        public int ulo;
+        [FieldOffset(12)]
+        public int umid;
+
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static void TestIntAssignment(Data d)
+    {
+        Dec p = default;
+        p.ulo = d.x;
+        p.umid = d.y;
+        // The jit gets field's type based on offset, so it will return `ulomidLE` as double.
+        d.x = Unsafe.As<Dec, DecCalc2>(ref p).ulo;
+    }
+
+    static int Main()
+    {
+        TestDoubleAssignment(default);
+        TestIntAssignment(default);
+               return 100;
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.csproj b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.csproj
new file mode 100644 (file)
index 0000000..b25553e
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadDoubleFromIntOffset.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>{9B2CCDC2-D6CE-4568-A419-AC79613BC235}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+       <LangVersion>latest</LangVersion>
+  </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>None</DebugType>
+    <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>

diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.cs b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.cs
new file mode 100644 (file)
index 0000000..b88af7f
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.cs
@@ -0,0 +1,85 @@
+// 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.
+
+// The test came from #20085. It tests that we do access overlapping fields with the correct types. 
+// Espessialy if the stuct was casted by 'Unsafe.As` from a promoted type
+// and the promoted type had another field on the same offset but with a different type/size.
+
+
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+using System.Diagnostics;
+using System;
+
+class TestAssignFieldsBetweenPromotedNotPromotedStructs
+{
+
+    struct PrimitiveStruct // a struct of single field of scalar types aligned at their natural boundary.
+    {
+        public long pointerSizedField;
+    }
+
+    struct NonPrimitiveStruct
+    {
+        public byte a;
+        public long b;
+    }
+
+    [StructLayout(LayoutKind.Explicit)]
+    struct NotPromotedStruct
+    {
+        [FieldOffset(0)]
+        public PrimitiveStruct notPromotedField;
+        [FieldOffset(0)]
+        public NonPrimitiveStruct anotherOverlappingStruct;
+
+        [FieldOffset(8)]
+        public long anotherField;
+
+
+        public static ref PromotedStruct AsPromotedStructSize20(ref NotPromotedStruct d) => ref Unsafe.As<NotPromotedStruct, PromotedStruct>(ref d);
+    }
+
+    [StructLayout(LayoutKind.Explicit)]
+    struct PromotedStruct
+    {
+        [FieldOffset(0)]
+        public PrimitiveStruct promotedField;
+        [FieldOffset(8)]
+        public long anotherField;
+
+
+        public static ref NotPromotedStruct AsNotPromotedStruct(ref PromotedStruct d) => ref Unsafe.As<PromotedStruct, NotPromotedStruct>(ref d);
+    }
+
+    // Some simple tests that check that lcl variables 
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void TestStructCasts()
+    {
+        PromotedStruct a = new PromotedStruct(); // Addr-exposed, cannot be promoted.promotedField.
+        a.promotedField.pointerSizedField = 4;
+        a.anotherField = 5;
+        NotPromotedStruct b = PromotedStruct.AsNotPromotedStruct(ref a);
+        // The cast can be inlined and the field handle will refer to the `PromotedStruct.pointerSizedField`,
+        // in this case we can promote it because `NotPromotedStruct.notPromotedField.pointerSizedField` has
+        // the same class handle.
+        Debug.Assert(b.notPromotedField.pointerSizedField == 0x4);
+
+        NotPromotedStruct c = PromotedStruct.AsNotPromotedStruct(ref a);
+        // The cast can be inlined and the field handle will refer to the `PromotedStruct.pointerSizedField`,
+        // in this case we cannot promote it because `NotPromotedStruct.anotherOverlappingStruct.a` has
+        // a different class handle (`NotPromotedStruct.anotherOverlappingStruct`).
+        Debug.Assert(c.anotherOverlappingStruct.a == 0x4);
+
+        Debug.Assert(c.anotherOverlappingStruct.b == 0x5);
+    }
+
+    public static int Main()
+    {
+        TestStructCasts();
+        return 100;
+    }
+
+}
+

diff --git a/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.csproj b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.csproj
new file mode 100644 (file)
index 0000000..b25553e
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Directed/StructPromote/Unsafe/ReadStructAsAnotherType.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>{9B2CCDC2-D6CE-4568-A419-AC79613BC235}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+       <LangVersion>latest</LangVersion>
+  </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>None</DebugType>
+    <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>