Jit: Fix SetIndirExceptionFlags.

SetIndirExceptionFlags should not set `GTF_IND_NONFAULTING` flag if the address has `GTF_EXCEPT` flag. The failing scenario was: We were setting `GTF_IND_NONFAULTING` on this indirection (since `ADDR` Node can't be null) ``` [000003] *--XG------- * IND int [000002] ---XG------- \--* ADDR byref Zero Fseq[i] [000001] ---XG------- \--* FIELD struct s [000000] ------------ \--* LCL_VAR ref V00 arg0 ``` this was then transformed to ``` [000003] *---G------- * IND int [000013] -----+------ \--* ADD byref [000000] -----+------ +--* LCL_VAR ref V00 arg0 [000012] -----+------ \--* CNS_INT long 8 field offset Fseq[s, i] ``` The `GTF_EXCEPT` flag was cleared on `IND` because it had `GTF_IND_NONFAULTING`set and the address no longer had `GTF_EXCEPT` flag. Fixes dotnet/coreclr#27027. Commit migrated from dotnet/coreclr@f58ce06
temfordump · Oct 11, 2019 · 45ca544 · 45ca544
1 parent e6c64c6
commit 45ca544
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 18 deletions.
diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp
@@ -7953,7 +7953,7 @@ void Compiler::gtUpdateNodeOperSideEffects(GenTree* tree)
         tree->gtFlags &= ~GTF_EXCEPT;
         if (tree->OperIsIndirOrArrLength())
         {
-            tree->gtFlags |= GTF_IND_NONFAULTING;
+            tree->SetIndirExceptionFlags(this);
         }
     }
 
@@ -9271,6 +9271,38 @@ bool GenTree::Precedes(GenTree* other)
     return false;
 }
 
+//------------------------------------------------------------------------------
+// SetIndirExceptionFlags : Set GTF_EXCEPT and GTF_IND_NONFAULTING flags as appropriate
+//                          on an indirection or an array length node.
+//
+// Arguments:
+//    comp  - compiler instance
+//
+
+void GenTree::SetIndirExceptionFlags(Compiler* comp)
+{
+    GenTree* addr = nullptr;
+    if (OperIsIndir())
+    {
+        addr = AsIndir()->Addr();
+    }
+    else
+    {
+        assert(gtOper == GT_ARR_LENGTH);
+        addr = AsArrLen()->ArrRef();
+    }
+
+    if (OperMayThrow(comp) || ((addr->gtFlags & GTF_EXCEPT) != 0))
+    {
+        gtFlags |= GTF_EXCEPT;
+    }
+    else
+    {
+        gtFlags &= ~GTF_EXCEPT;
+        gtFlags |= GTF_IND_NONFAULTING;
+    }
+}
+
 #ifdef DEBUG
 
 /* static */ int GenTree::gtDispFlags(unsigned flags, unsigned debugFlags)

diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h
@@ -2101,11 +2101,7 @@ struct GenTree
         gtFlags &= ~GTF_REUSE_REG_VAL;
     }
 
-    void SetIndirExceptionFlags(Compiler* comp)
-    {
-        assert(OperIsIndirOrArrLength());
-        gtFlags |= OperMayThrow(comp) ? GTF_EXCEPT : GTF_IND_NONFAULTING;
-    }
+    void SetIndirExceptionFlags(Compiler* comp);
 
 #if MEASURE_NODE_SIZE
     static void DumpNodeSizes(FILE* fp);

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
@@ -6188,7 +6188,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)
         tree->SetOper(GT_IND);
         tree->gtOp.gtOp1 = addr;
 
-        tree->gtFlags &= (~GTF_EXCEPT | addr->gtFlags);
         tree->SetIndirExceptionFlags(this);
 
         if (addExplicitNullCheck)
@@ -8859,7 +8858,6 @@ GenTree* Compiler::fgMorphOneAsgBlockOp(GenTree* tree)
                 tree->gtFlags |= GTF_GLOB_REF;
             }
 
-            dest->gtFlags &= (~GTF_EXCEPT | dest->AsIndir()->Addr()->gtFlags);
             dest->SetIndirExceptionFlags(this);
             tree->gtFlags |= (dest->gtFlags & GTF_EXCEPT);
         }
@@ -8906,7 +8904,6 @@ GenTree* Compiler::fgMorphOneAsgBlockOp(GenTree* tree)
                     src->gtFlags |= (GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
                 }
 
-                src->gtFlags &= (~GTF_EXCEPT | src->AsIndir()->Addr()->gtFlags);
                 src->SetIndirExceptionFlags(this);
             }
         }
@@ -11756,21 +11753,24 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
 
 DONE_MORPHING_CHILDREN:
 
-    if (tree->OperMayThrow(this))
+    if (tree->OperIsIndirOrArrLength())
     {
-        // Mark the tree node as potentially throwing an exception
-        tree->gtFlags |= GTF_EXCEPT;
+        tree->SetIndirExceptionFlags(this);
     }
     else
     {
-        if (tree->OperIsIndirOrArrLength())
+        if (tree->OperMayThrow(this))
         {
-            tree->gtFlags |= GTF_IND_NONFAULTING;
+            // Mark the tree node as potentially throwing an exception
+            tree->gtFlags |= GTF_EXCEPT;
         }
-        if (((op1 == nullptr) || ((op1->gtFlags & GTF_EXCEPT) == 0)) &&
-            ((op2 == nullptr) || ((op2->gtFlags & GTF_EXCEPT) == 0)))
+        else
         {
-            tree->gtFlags &= ~GTF_EXCEPT;
+            if (((op1 == nullptr) || ((op1->gtFlags & GTF_EXCEPT) == 0)) &&
+                ((op2 == nullptr) || ((op2->gtFlags & GTF_EXCEPT) == 0)))
+            {
+                tree->gtFlags &= ~GTF_EXCEPT;
+            }
         }
     }
 
@@ -14649,7 +14649,7 @@ GenTree* Compiler::fgMorphTree(GenTree* tree, MorphAddrContext* mac)
             tree->gtDynBlk.Addr()        = fgMorphTree(tree->gtDynBlk.Addr());
             tree->gtDynBlk.gtDynamicSize = fgMorphTree(tree->gtDynBlk.gtDynamicSize);
 
-            tree->gtFlags &= (~GTF_EXCEPT & ~GTF_CALL);
+            tree->gtFlags &= ~GTF_CALL;
             tree->SetIndirExceptionFlags(this);
 
             if (tree->OperGet() == GT_STORE_DYN_BLK)

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.cs
@@ -0,0 +1,40 @@
+// 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.Runtime.CompilerServices;
+
+public struct S
+{
+    public int i;
+    public int j;
+}
+
+public class Test
+{
+    public S s;
+
+    public static int Main()
+    {
+        // Test that the correct exception is thrown from Run.
+        // The bug was that the exceptions were reordered and DivideByZeroException
+        // was thrown instead of NullReferenceException.
+        try {
+            Run(null, 0);
+        }
+        catch (System.NullReferenceException)
+        {
+            return 100;
+        }
+
+        return -1;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static int Run(Test test, int j)
+    {
+        int k = test.s.i + 1/j;
+        return k;
+    }
+}
diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType />
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>