From b46f8e8358f9cee049eda7eda3707d4ccdc960f9 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Tue, 13 Mar 2018 17:16:44 -0700 Subject: [PATCH] Fix GH Issue 16892 - GC hole due to GT_INDEX_ADDR Added test case --- src/jit/compiler.cpp | 10 +- src/jit/flowgraph.cpp | 2 + src/jit/gentree.h | 2 +- src/jit/morph.cpp | 2 + .../JitBlue/GitHub_16892/GitHub_16892.cs | 104 ++++++++++++++++++ .../JitBlue/GitHub_16892/GitHub_16892.csproj | 41 +++++++ 6 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.csproj diff --git a/src/jit/compiler.cpp b/src/jit/compiler.cpp index dc68ce0af540..bc0159ca2e2b 100644 --- a/src/jit/compiler.cpp +++ b/src/jit/compiler.cpp @@ -9546,10 +9546,6 @@ int cTreeFlagsIR(Compiler* comp, GenTree* tree) case GT_INDEX: - if (tree->gtFlags & GTF_INX_RNGCHK) - { - chars += printf("[INX_RNGCHK]"); - } if (tree->gtFlags & GTF_INX_REFARR_LAYOUT) { chars += printf("[INX_REFARR_LAYOUT]"); @@ -9558,6 +9554,12 @@ int cTreeFlagsIR(Compiler* comp, GenTree* tree) { chars += printf("[INX_STRING_LAYOUT]"); } + __fallthrough; + case GT_INDEX_ADDR: + if (tree->gtFlags & GTF_INX_RNGCHK) + { + chars += printf("[INX_RNGCHK]"); + } break; case GT_IND: diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index ccf70dcddfbe..b1a4fef8e20f 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -23615,6 +23615,8 @@ Compiler::fgWalkResult Compiler::fgChkThrowCB(GenTree** pTree, fgWalkData* data) break; case GT_INDEX: + case GT_INDEX_ADDR: + // These two call CORINFO_HELP_RNGCHKFAIL for Debug code if (tree->gtFlags & GTF_INX_RNGCHK) { return Compiler::WALK_ABORT; diff --git a/src/jit/gentree.h b/src/jit/gentree.h index 35c215cf21ff..c33cb51d588c 100644 --- a/src/jit/gentree.h +++ b/src/jit/gentree.h @@ -893,7 +893,7 @@ struct GenTree #define GTF_FLD_VOLATILE 0x40000000 // GT_FIELD/GT_CLS_VAR -- same as GTF_IND_VOLATILE #define GTF_FLD_INITCLASS 0x20000000 // GT_FIELD/GT_CLS_VAR -- field access requires preceding class/static init helper -#define GTF_INX_RNGCHK 0x80000000 // GT_INDEX -- the array reference should be range-checked. +#define GTF_INX_RNGCHK 0x80000000 // GT_INDEX/GT_INDEX_ADDR -- the array reference should be range-checked. #define GTF_INX_REFARR_LAYOUT 0x20000000 // GT_INDEX #define GTF_INX_STRING_LAYOUT 0x40000000 // GT_INDEX -- this uses the special string array layout diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index aeca2feb80d1..1e2b1cbc526a 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -1590,6 +1590,7 @@ void fgArgInfo::ArgsComplete() // This means unnesting, sorting, etc. Technically this is overly // conservative, but I want to avoid as much special-case debug-only code // as possible, so leveraging the GTF_CALL flag is the easiest. + // if (!(argx->gtFlags & GTF_CALL) && (argx->gtFlags & GTF_EXCEPT) && (argCount > 1) && compiler->opts.compDbgCode && (compiler->fgWalkTreePre(&argx, Compiler::fgChkThrowCB) == Compiler::WALK_ABORT)) @@ -6063,6 +6064,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) indexAddr->gtFlags |= (array->gtFlags | index->gtFlags) & GTF_ALL_EFFECT; // Mark the indirection node as needing a range check if necessary. + // Note this will always be true unless JitSkipArrayBoundCheck() is used if ((indexAddr->gtFlags & GTF_INX_RNGCHK) != 0) { fgSetRngChkTarget(indexAddr); diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.cs b/tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.cs new file mode 100644 index 000000000000..3621f6f927be --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.cs @@ -0,0 +1,104 @@ +using System; +using System.Runtime.CompilerServices; +// +// Test case for a GC Stress 4 failure +// +// This test was failing during a GC Stress 4 run in the method Test(...) +// +// The failure requires that this test be built with Debug codegen +// +// The GC Stress failure will occur if the JIT +// 1. has evaluated and stored the two outgoing stack based arguments: a5, a6 +// 2. and then performs a call to the helper CORINFO_HELP_RNGCHKFAIL +// +// With the fix the JIT will evaluate the arr[3] with the rangecheck +// into a new compiler temp, before storing any outgoing arguments. +// + +class Item +{ + int _value; + + [MethodImpl(MethodImplOptions.NoInlining)] + public Item(int value) { _value = value; } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int GetValue() { return _value; } +} + +class Program +{ + public Item[] itemArray; + + [MethodImpl(MethodImplOptions.NoInlining)] + void Init() + { + itemArray = new Item[11]; + for (int i=0; i<11; i++) + { + itemArray[i] = new Item(i); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Compute(Item r1, Item r2, Item r3, Item r4, Item s5, Item s6) + { + int result = r1.GetValue(); + result += r2.GetValue(); + result += r3.GetValue(); + result += r4.GetValue(); + result += s5.GetValue(); + result += s6.GetValue(); + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + int Test(Item a1, Item a2, Item a4, Item a5, Item a6) + { + Item[] arr = itemArray; + int result = 0; + + // Insure that we have to generate fully interruptible GC information + // Form a possible infinte loop that the JIT believes could execute + // without encountering a GC safe point. + // + do { + if (result < 5) + { + result = Compute(a1, a2, arr[3], a4, a5, a6); + } + } while (result < 0); + + return result; + + } + + static int Main(string[] args) + { + Program prog = new Program(); + + prog.Init(); + + Item[] arr = prog.itemArray; + + Item obj1 = arr[1]; + Item obj2 = arr[2]; + Item obj3 = arr[3]; + Item obj4 = arr[4]; + Item obj5 = arr[5]; + Item obj6 = arr[6]; + + int result = prog.Test(obj1, obj2, obj4, obj5, obj6); + + if (result == 21) + { + Console.WriteLine("Passed"); + return 100; + } + else + { + Console.WriteLine("Failed"); + return -1; + } + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.csproj new file mode 100644 index 000000000000..0ee4f4aaa2fa --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_16892/GitHub_16892.csproj @@ -0,0 +1,41 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {76E69AA0-8C5A-4F76-8561-B8089FFA8D79} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + + + + False + + + + Full + False + True + + + + + + + + + + + $(JitPackagesConfigFileDirectory)benchmark\obj\project.assets.json + +