Restrict morph add rearrangement. (#23984)

Morph has transformations
((x + const) + y) => ((x + y) + const)
and
((x + const1) + (y + const2)) => ((x + y) + (const1 + const2))

If x or y is a GC pointer and one of the int operands may be negative,
this may result in a byref temp that points outside of the ref object.
If the code is in a non-interruptible region and a GC happens, the byref
won't be updated.

This change disallows the transformations if one of the non-const
operands is a GC pointer.

Fixes #23792.

diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index 83e069c..478a404 100644 (file)
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -12991,24 +12991,31 @@ DONE_MORPHING_CHILDREN:
                     op1->gtOp.gtOp2->gtOper == GT_CNS_INT && op2->gtOp.gtOp2->gtOper == GT_CNS_INT &&
                     !op1->gtOverflow() && !op2->gtOverflow())
                 {
-                    cns1 = op1->gtOp.gtOp2;
-                    cns2 = op2->gtOp.gtOp2;
-                    cns1->gtIntCon.gtIconVal += cns2->gtIntCon.gtIconVal;
-#ifdef _TARGET_64BIT_
-                    if (cns1->TypeGet() == TYP_INT)
+                    // Don't create a byref pointer that may point outside of the ref object.
+                    // If a GC happens, the byref won't get updated. This can happen if one
+                    // of the int components is negative. It also requires the address generation
+                    // be in a fully-interruptible code region.
+                    if (!varTypeIsGC(op1->gtOp.gtOp1->TypeGet()) && !varTypeIsGC(op2->gtOp.gtOp1->TypeGet()))
                     {
-                        // we need to properly re-sign-extend or truncate after adding two int constants above
-                        cns1->AsIntCon()->TruncateOrSignExtend32();
-                    }
+                        cns1 = op1->gtOp.gtOp2;
+                        cns2 = op2->gtOp.gtOp2;
+                        cns1->gtIntCon.gtIconVal += cns2->gtIntCon.gtIconVal;
+#ifdef _TARGET_64BIT_
+                        if (cns1->TypeGet() == TYP_INT)
+                        {
+                            // we need to properly re-sign-extend or truncate after adding two int constants above
+                            cns1->AsIntCon()->TruncateOrSignExtend32();
+                        }
 #endif //_TARGET_64BIT_
 
-                    tree->gtOp.gtOp2 = cns1;
-                    DEBUG_DESTROY_NODE(cns2);
+                        tree->gtOp.gtOp2 = cns1;
+                        DEBUG_DESTROY_NODE(cns2);
 
-                    op1->gtOp.gtOp2 = op2->gtOp.gtOp1;
-                    op1->gtFlags |= (op1->gtOp.gtOp2->gtFlags & GTF_ALL_EFFECT);
-                    DEBUG_DESTROY_NODE(op2);
-                    op2 = tree->gtOp.gtOp2;
+                        op1->gtOp.gtOp2 = op2->gtOp.gtOp1;
+                        op1->gtFlags |= (op1->gtOp.gtOp2->gtFlags & GTF_ALL_EFFECT);
+                        DEBUG_DESTROY_NODE(op2);
+                        op2 = tree->gtOp.gtOp2;
+                    }
                 }
 
                 if (op2->IsCnsIntOrI() && varTypeIsIntegralOrI(typ))
@@ -14036,39 +14043,36 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree)
     if (fgGlobalMorph && (oper == GT_ADD) && !tree->gtOverflow() && (op1->gtOper == GT_ADD) && !op1->gtOverflow() &&
         varTypeIsIntegralOrI(typ))
     {
+        GenTree* ad1 = op1->gtOp.gtOp1;
         GenTree* ad2 = op1->gtOp.gtOp2;
 
-        if (op2->OperIsConst() == 0 && ad2->OperIsConst() != 0)
-        {
-            // This takes
-            //       + (tree)
-            //      / \.
-            //     /   \.
-            //    /     \.
-            //   + (op1) op2
-            //  / \.
-            //     \.
-            //     ad2
-            //
-            // And it swaps ad2 and op2.  If (op2) is varTypeIsGC, then this implies that (tree) is
-            // varTypeIsGC.  If (op1) is not, then when we swap (ad2) and (op2), then we have a TYP_INT node
-            // (op1) with a child that is varTypeIsGC.  If we encounter that situation, make (op1) the same
-            // type as (tree).
+        if (!op2->OperIsConst() && ad2->OperIsConst())
+        {
+            //  This takes
+            //        + (tree)
+            //       / \.
+            //      /   \.
+            //     /     \.
+            //    + (op1) op2
+            //   / \.
+            //  /   \.
+            // ad1  ad2
             //
-            // Also, if (ad2) is varTypeIsGC then (tree) must also be (since op1 is), so no fixing is
-            // necessary
+            // and it swaps ad2 and op2.
 
-            if (varTypeIsGC(op2->TypeGet()))
+            // Don't create a byref pointer that may point outside of the ref object.
+            // If a GC happens, the byref won't get updated. This can happen if one
+            // of the int components is negative. It also requires the address generation
+            // be in a fully-interruptible code region.
+            if (!varTypeIsGC(ad1->TypeGet()) && !varTypeIsGC(op2->TypeGet()))
             {
-                noway_assert(varTypeIsGC(typ));
-                op1->gtType = typ;
-            }
-            tree->gtOp2 = ad2;
+                tree->gtOp2 = ad2;
 
-            op1->gtOp.gtOp2 = op2;
-            op1->gtFlags |= op2->gtFlags & GTF_ALL_EFFECT;
+                op1->gtOp.gtOp2 = op2;
+                op1->gtFlags |= op2->gtFlags & GTF_ALL_EFFECT;
 
-            op2 = tree->gtOp2;
+                op2 = tree->gtOp2;
+            }
         }
     }
 

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23792/GitHub_23792.il b/tests/src/JIT/Regression/JitBlue/GitHub_23792/GitHub_23792.il
new file mode 100644 (file)
index 0000000..9c63524
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_23792/GitHub_23792.il
@@ -0,0 +1,163 @@
+
+// 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.
+
+// This tests that 
+// ((x+icon)+y) => ((x+y)+icon)
+// and
+// ((x+icon1)+(y+icon2)) => ((x+y)+(icon1+icon2))
+// transformations in morph don't create byrefs pointing outside of the ref object.
+// Method Run1 calculates (theRef - 8 + a -8 + b).
+// Before the fix morph transformed this to ((theRef + a) + b -16).
+// theRef + a may point outside of the object.
+// Method Run2 calculates ((theRef - 8) + (a - 8) + b).
+// Before the fix morph transformed this to ((theRef + a) + b -16).
+// theRef + a may point outside of the object.
+
+.assembly extern mscorlib{}
+.assembly extern System.Console{}
+.assembly a {}
+
+.class private auto ansi beforefieldinit Test
+       extends [mscorlib]System.Object
+{
+
+  //public static int Main()
+  //{
+  //    byte size = 9;
+  //    byte[] arr = new byte[9];
+  //    for (byte i = 0; i < size; ++i)
+  //    {
+  //         arr[i] = i;
+  //    }
+  //    ref byte newRef1 = ref Run1(ref arr[8], (IntPtr)8, (IntPtr)8);
+  //    ref byte newRef2 = ref Run2(ref arr[8], (IntPtr)8, (IntPtr)8);
+  //    int result = 84 + newRef1 + newRef2;
+  //    if (result == 100)
+  //    {
+  //        Console.WriteLine("PASS");
+  //    }
+  //    else
+  //    {
+  //        Console.WriteLine("FAIL");
+  //    }            
+  //    return result;
+  //}
+
+  .method public hidebysig static int32  Main() cil managed
+  {
+        .entrypoint
+  // Code size       119 (0x77)
+  .maxstack  3
+  .locals init (uint8 V_0,
+           uint8[] V_1,
+           uint8& V_2,
+           uint8& V_3,
+           uint8 V_4)
+  IL_0000:  ldc.i4.s   9
+  IL_0002:  stloc.0
+  IL_0003:  ldc.i4.s   9
+  IL_0005:  newarr     [mscorlib]System.Byte
+  IL_000a:  stloc.1
+  IL_000b:  ldc.i4.0
+  IL_000c:  stloc.s    V_4
+  IL_000e:  br.s       IL_001d
+  IL_0010:  ldloc.1
+  IL_0011:  ldloc.s    V_4
+  IL_0013:  ldloc.s    V_4
+  IL_0015:  stelem.i1
+  IL_0016:  ldloc.s    V_4
+  IL_0018:  ldc.i4.1
+  IL_0019:  add
+  IL_001a:  conv.u1
+  IL_001b:  stloc.s    V_4
+  IL_001d:  ldloc.s    V_4
+  IL_001f:  ldloc.0
+  IL_0020:  blt.s      IL_0010
+  IL_0022:  ldloc.1
+  IL_0023:  ldc.i4.8
+  IL_0024:  ldelema    [mscorlib]System.Byte
+  IL_0029:  ldc.i4.8
+  IL_002a:  call       native int [mscorlib]System.IntPtr::op_Explicit(int32)
+  IL_002f:  ldc.i4.8
+  IL_0030:  call       native int [mscorlib]System.IntPtr::op_Explicit(int32)
+  IL_0035:  call       uint8& Test::Run1(uint8&,
+                                        native int,
+                                        native int)
+  IL_003a:  stloc.2
+  IL_003b:  ldloc.1
+  IL_003c:  ldc.i4.8
+  IL_003d:  ldelema    [mscorlib]System.Byte
+  IL_0042:  ldc.i4.8
+  IL_0043:  call       native int [mscorlib]System.IntPtr::op_Explicit(int32)
+  IL_0048:  ldc.i4.8
+  IL_0049:  call       native int [mscorlib]System.IntPtr::op_Explicit(int32)
+  IL_004e:  call       uint8& Test::Run2(uint8&,
+                                        native int,
+                                        native int)
+  IL_0053:  stloc.3
+  IL_0054:  ldc.i4.s   84
+  IL_0056:  ldloc.2
+  IL_0057:  ldind.u1
+  IL_0058:  add
+  IL_0059:  ldloc.3
+  IL_005a:  ldind.u1
+  IL_005b:  add
+  IL_005c:  dup
+  IL_005d:  ldc.i4.s   100
+  IL_005f:  bne.un.s   IL_006c
+  IL_0061:  ldstr      "PASS"
+  IL_0066:  call       void [System.Console]System.Console::WriteLine(string)
+  IL_006b:  ret
+  IL_006c:  ldstr      "FAIL"
+  IL_0071:  call       void [System.Console]System.Console::WriteLine(string)
+  IL_0076:  ret
+} // end of method Test::Main
+
+
+  .method private hidebysig static uint8& 
+          Run1(uint8& theRef,
+              native int a,
+              native int b) cil managed noinlining
+  {
+    ldarg.0
+    ldc.i4    8
+    sub
+    ldarg.1
+    ldc.i4    8
+    sub
+    add
+    ldc.i4    8
+    add
+    ret
+  } // end of method Test::Run1
+
+    .method private hidebysig static uint8& 
+          Run2(uint8& theRef,
+              native int a,
+              native int b) cil managed noinlining
+  {
+    ldarg.0
+    ldc.i4    -8
+    add
+    ldarg.1
+    ldc.i4    -8
+    add
+    add
+    ldarg.2
+    add
+    ret
+  } // end of method Test::Run2
+
+  .method public hidebysig specialname rtspecialname 
+          instance void  .ctor() cil managed
+  {
+    // Code size       7 (0x7)
+    .maxstack  8
+    IL_0000:  ldarg.0
+    IL_0001:  call       instance void [mscorlib]System.Object::.ctor()
+    IL_0006:  ret
+  } // end of method Test::.ctor
+
+} // end of class Test

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23792/GitHub_23792.ilproj b/tests/src/JIT/Regression/JitBlue/GitHub_23792/GitHub_23792.ilproj
new file mode 100644 (file)
index 0000000..1e33a02
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_23792/GitHub_23792.ilproj
@@ -0,0 +1,37 @@
+<?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>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="GitHub_23792.il" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup> 
+</Project>