Skip to content

Commit

Permalink
JIT: fix bug where a gc struct is not zero initialized (#67825)
Browse files Browse the repository at this point in the history
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].

Addresses #65694.
  • Loading branch information
AndyAyersMS committed Apr 14, 2022
1 parent 73665cc commit b13513a
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 14 deletions.
46 changes: 33 additions & 13 deletions src/coreclr/jit/liveness.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
73 changes: 73 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.cs
Original file line number Diff line number Diff line change
@@ -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<Key, Problem> _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<Key, Problem> d = new Dictionary<Key, Problem>();
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;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit b13513a

Please sign in to comment.