From 45ca5446f2afcec0e1dda1f1ac4fe4963f0f4e06 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Fri, 4 Oct 2019 16:04:33 -0700 Subject: [PATCH] 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 https://github.com/dotnet/coreclr/commit/f58ce06e619e607d8e297d57f10afdc15cfce232 --- src/coreclr/src/jit/gentree.cpp | 34 +++++++++++++++- src/coreclr/src/jit/gentree.h | 6 +-- src/coreclr/src/jit/morph.cpp | 24 +++++------ .../JitBlue/GitHub_27027/GitHub_27027.cs | 40 +++++++++++++++++++ .../JitBlue/GitHub_27027/GitHub_27027.csproj | 13 ++++++ 5 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.csproj diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index ef95417288c..231ab70e3ed 100644 --- 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 index 6bc6a519324..33bc1f0cbfc 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -2101,11 +2101,7 @@ public: 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 index 89721522c10..33fa62db631 100644 --- 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 new file mode 100644 index 00000000000..c18dea58bc1 --- /dev/null +++ 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 new file mode 100644 index 00000000000..656bedde102 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27027/GitHub_27027.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + + True + + + + + -- 2.34.1