From ee346a18490175cd6051d08231a9521fa9ecb5f6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 8 Apr 2022 15:56:51 -0700 Subject: [PATCH] JIT: fix bug where a gc struct is not zero initialized This fixes an unexpected interaction between the zero-init optimization and dead stores. We have a local gc struct that is tracked but not promoted (and so on the frame) with an explicit zero init. The zero-init opt determines that in-prolog init is not needed because the local is tracked and has a live explicit initializer. So it marks the local as `lvHasExplicitInit`. But subsequent control flow optimizations end up making the explicit zero init dead, and it is removed by dead stores. Later on when we report GC info for the struct we report it as untracked. This leads to the GC seeing an uninitialized stack slot as a GC ref. The fix is to inhibit dead stores of zero initializers for `lvHasExplicitInit` (restricted to GC locals with multiple references). [some alternative fixes were considered, see notes in the PR]. Fixes #65694. --- src/coreclr/jit/liveness.cpp | 46 ++++++++---- src/coreclr/jit/optimizer.cpp | 2 +- .../JitBlue/Runtime_65694/Runtime_65694.cs | 73 +++++++++++++++++++ .../Runtime_65694/Runtime_65694.csproj | 9 +++ 4 files changed, 116 insertions(+), 14 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.csproj diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index ba1a653b93f2b..f516097c7f09e 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -1988,25 +1988,45 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR if (blockRange.TryGetUse(node, &addrUse) && (addrUse.User()->OperIs(GT_STOREIND, GT_STORE_BLK, GT_STORE_OBJ))) { - // Remove the store. DCE will iteratively clean up any ununsed operands. GenTreeIndir* const store = addrUse.User()->AsIndir(); - JITDUMP("Removing dead indirect store:\n"); - DISPNODE(store); + // If this is a zero init of an explicit zero init gc local + // that has at least one other reference, we will keep the zero init. + // + const LclVarDsc& varDsc = lvaTable[node->AsLclVarCommon()->GetLclNum()]; + const bool isExplicitInitLocal = varDsc.lvHasExplicitInit; + const bool isReferencedLocal = varDsc.lvRefCnt() > 1; + const bool isZeroInit = store->OperIsInitBlkOp(); + const bool isGCInit = varDsc.HasGCPtr(); - assert(store->Addr() == node); - blockRange.Delete(this, block, node); - - GenTree* data = - store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data(); - data->SetUnusedValue(); - - if (data->isIndir()) + if (isExplicitInitLocal && isReferencedLocal && isZeroInit && isGCInit) { - Lowering::TransformUnusedIndirection(data->AsIndir(), this, block); + // Yes, we'd better keep it around. + // + JITDUMP("Keeping dead indirect store -- explicit zero init of gc type\n"); + DISPNODE(store); } + else + { + // Remove the store. DCE will iteratively clean up any ununsed operands. + // + JITDUMP("Removing dead indirect store:\n"); + DISPNODE(store); + + assert(store->Addr() == node); + blockRange.Delete(this, block, node); - fgRemoveDeadStoreLIR(store, block); + GenTree* data = + store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data(); + data->SetUnusedValue(); + + if (data->isIndir()) + { + Lowering::TransformUnusedIndirection(data->AsIndir(), this, block); + } + + fgRemoveDeadStoreLIR(store, block); + } } } } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 8b8e7895a4f93..84c75a2cbb63c 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -9492,7 +9492,7 @@ void Compiler::optRemoveRedundantZeroInits() // the prolog and this explicit intialization. Therefore, it doesn't // require zero initialization in the prolog. lclDsc->lvHasExplicitInit = 1; - JITDUMP("Marking " FMT_LP " as having an explicit init\n", lclNum); + JITDUMP("Marking V%02u as having an explicit init\n", lclNum); } } break; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.cs b/src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.cs new file mode 100644 index 0000000000000..1dae258e5273c --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.cs @@ -0,0 +1,73 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; + +public struct Key +{ + public int a; + public string s; +} + +public struct Problem +{ + public int x; + public double d; + public string s0; + public int y; + public double e; + public string s1; +} + +public class Runtime_65694 +{ + public Dictionary _d; + + [MethodImpl(MethodImplOptions.NoInlining)] + public void D() + { + Problem p = new Problem { s0 = "hello", s1 = "world", x = 33 }; + Key k = new Key() { a = 0, s = "a" }; + Dictionary d = new Dictionary(); + d[k] = p; + + _d = d; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void F() + { + GC.Collect(); + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + public int G(Key k, bool b) + { + Problem p = default; + + F(); + + if (b) + { + if (_d?.TryGetValue(k, out p) == true && (p.x == 33)) + { + return 22; + } + } + + return 0; + } + + public static int Main() + { + var r = new Runtime_65694(); + r.D(); + int result = 0; + Key k = new Key() { a = 0, s = "a" }; + result += r.G(k, true); + return result + 78; + } +} + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.csproj new file mode 100644 index 0000000000000..f492aeac9d056 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + \ No newline at end of file