Skip to content

Commit

Permalink
Add checks for missing SSA numbers (#57274)
Browse files Browse the repository at this point in the history
In several places there were cases where we were checking lvaInSsa for
the lcl num of a local tree node, but then proceeded to use the ssa
number directly after. It is possible for a local to be in SSA without
tree nodes themselves having SSA form built for them, for example if
unreachable code makes it to SSA building. This adds some additional
check for missing SSA numbers.
  • Loading branch information
jakobbotsch authored Aug 17, 2021
1 parent f7cd0c6 commit 57839c4
Show file tree
Hide file tree
Showing 8 changed files with 226 additions and 37 deletions.
4 changes: 2 additions & 2 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2963,7 +2963,7 @@ GenTree* Compiler::optCopyAssertionProp(AssertionDsc* curAssertion,

// Extract the matching lclNum and ssaNum.
const unsigned copyLclNum = (op1.lcl.lclNum == lclNum) ? op2.lcl.lclNum : op1.lcl.lclNum;
unsigned copySsaNum = BAD_VAR_NUM;
unsigned copySsaNum = SsaConfig::RESERVED_SSA_NUM;
if (!optLocalAssertionProp)
{
// Extract the ssaNum of the matching lclNum.
Expand Down Expand Up @@ -2991,8 +2991,8 @@ GenTree* Compiler::optCopyAssertionProp(AssertionDsc* curAssertion,
return nullptr;
}

tree->SetSsaNum(copySsaNum);
tree->SetLclNum(copyLclNum);
tree->SetSsaNum(copySsaNum);

#ifdef DEBUG
if (verbose)
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/earlyprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,8 @@ GenTree* Compiler::optPropGetValueRec(unsigned lclNum, unsigned ssaNum, optPropK

GenTree* treeRhs = ssaDefAsg->gtGetOp2();

if (treeRhs->OperIsScalarLocal() && lvaInSsa(treeRhs->AsLclVarCommon()->GetLclNum()))
if (treeRhs->OperIsScalarLocal() && lvaInSsa(treeRhs->AsLclVarCommon()->GetLclNum()) &&
treeRhs->AsLclVarCommon()->HasSsaName())
{
// Recursively track the Rhs
unsigned rhsLclNum = treeRhs->AsLclVarCommon()->GetLclNum();
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6340,7 +6340,7 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
// To be invariant a LclVar node must not be the LHS of an assignment ...
bool isInvariant = !user->OperIs(GT_ASG) || (user->AsOp()->gtGetOp1() != tree);
// and the variable must be in SSA ...
isInvariant = isInvariant && m_compiler->lvaInSsa(lclNum);
isInvariant = isInvariant && m_compiler->lvaInSsa(lclNum) && lclVar->HasSsaName();
// and the SSA definition must be outside the loop we're hoisting from ...
isInvariant = isInvariant &&
!m_compiler->optLoopTable[m_loopNum].lpContains(
Expand Down Expand Up @@ -7244,7 +7244,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
{
// If it's a local byref for which we recorded a value number, use that...
GenTreeLclVar* argLcl = arg->AsLclVar();
if (lvaInSsa(argLcl->GetLclNum()))
if (lvaInSsa(argLcl->GetLclNum()) && argLcl->HasSsaName())
{
ValueNum argVN =
lvaTable[argLcl->GetLclNum()].GetPerSsaData(argLcl->GetSsaNum())->m_vnPair.GetLiberal();
Expand Down Expand Up @@ -7334,7 +7334,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
if (rhsVN != ValueNumStore::NoVN)
{
rhsVN = vnStore->VNNormalValue(rhsVN);
if (lvaInSsa(lhsLcl->GetLclNum()))
if (lvaInSsa(lhsLcl->GetLclNum()) && lhsLcl->HasSsaName())
{
lvaTable[lhsLcl->GetLclNum()]
.GetPerSsaData(lhsLcl->GetSsaNum())
Expand Down
71 changes: 40 additions & 31 deletions src/coreclr/jit/valuenum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7047,15 +7047,14 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)

unsigned lclNum = lclVarTree->GetLclNum();

unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);
// Ignore vars that we excluded from SSA (for example, because they're address-exposed). They don't have
// SSA names in which to store VN's on defs. We'll yield unique VN's when we read from them.
if (lvaInSsa(lclNum))
if (lvaInSsa(lclNum) && lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM)
{
// Should not have been recorded as updating ByrefExposed.
assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree));

unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);

ValueNum initBlkVN = ValueNumStore::NoVN;
GenTree* initConst = rhs;
if (isEntire && initConst->OperGet() == GT_CNS_INT)
Expand Down Expand Up @@ -7114,16 +7113,15 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
// Should not have been recorded as updating the GC heap.
assert(!GetMemorySsaMap(GcHeap)->Lookup(tree));

unsigned lhsLclNum = lclVarTree->GetLclNum();
FieldSeqNode* lhsFldSeq = nullptr;
unsigned lhsLclNum = lclVarTree->GetLclNum();
FieldSeqNode* lhsFldSeq = nullptr;
unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);
// If it's excluded from SSA, don't need to do anything.
if (lvaInSsa(lhsLclNum))
if (lvaInSsa(lhsLclNum) && lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM)
{
// Should not have been recorded as updating ByrefExposed.
assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree));

unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);

if (lhs->IsLocalExpr(this, &lclVarTree, &lhsFldSeq))
{
noway_assert(lclVarTree->GetLclNum() == lhsLclNum);
Expand Down Expand Up @@ -7170,7 +7168,8 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
{
unsigned rhsLclNum = rhsLclVarTree->GetLclNum();
rhsVarDsc = &lvaTable[rhsLclNum];
if (!lvaInSsa(rhsLclNum) || rhsFldSeq == FieldSeqStore::NotAField())
if (!lvaInSsa(rhsLclNum) || !rhsLclVarTree->HasSsaName() ||
rhsFldSeq == FieldSeqStore::NotAField())
{
rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, rhsLclVarTree->TypeGet()));
isNewUniq = true;
Expand Down Expand Up @@ -7199,7 +7198,8 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
{
unsigned rhsLclNum = rhsLclVarTree->GetLclNum();
rhsVarDsc = &lvaTable[rhsLclNum];
if (!lvaInSsa(rhsLclNum) || rhsFldSeq == FieldSeqStore::NotAField())
if (!lvaInSsa(rhsLclNum) || !rhsLclVarTree->HasSsaName() ||
rhsFldSeq == FieldSeqStore::NotAField())
{
isNewUniq = true;
}
Expand Down Expand Up @@ -7576,7 +7576,8 @@ void Compiler::fgValueNumberTree(GenTree* tree)
LclVarDsc* varDsc = &lvaTable[lclNum];

var_types indType = tree->TypeGet();
if ((lclFld->GetFieldSeq() == FieldSeqStore::NotAField()) || !lvaInSsa(lclFld->GetLclNum()))
if ((lclFld->GetFieldSeq() == FieldSeqStore::NotAField()) || !lvaInSsa(lclFld->GetLclNum()) ||
!lclFld->HasSsaName())
{
// This doesn't represent a proper field access or it's a struct
// with overlapping fields that is hard to reason about; return a new unique VN.
Expand Down Expand Up @@ -7955,19 +7956,19 @@ void Compiler::fgValueNumberTree(GenTree* tree)

wasLocal = true;

bool wasInSsa = false;
if (lvaInSsa(lclNum))
{
FieldSeqNode* fieldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]);

// Either "arg" is the address of (part of) a local itself, or else we have
// a "rogue" PtrToLoc, one that should have made the local in question
// address-exposed. Assert on that.
GenTreeLclVarCommon* lclVarTree = nullptr;
bool isEntire = false;
unsigned lclDefSsaNum = SsaConfig::RESERVED_SSA_NUM;
ValueNumPair newLhsVNPair;
GenTreeLclVarCommon* lclVarTree = nullptr;
bool isEntire = false;

if (arg->DefinesLocalAddr(this, genTypeSize(lhs->TypeGet()), &lclVarTree, &isEntire))
if (arg->DefinesLocalAddr(this, genTypeSize(lhs->TypeGet()), &lclVarTree, &isEntire) &&
lclVarTree->HasSsaName())
{
// The local #'s should agree.
assert(lclNum == lclVarTree->GetLclNum());
Expand All @@ -7988,6 +7989,8 @@ void Compiler::fgValueNumberTree(GenTree* tree)
rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet()));
}

unsigned lclDefSsaNum;
ValueNumPair newLhsVNPair;
if (isEntire)
{
newLhsVNPair = rhsVNPair;
Expand All @@ -8005,24 +8008,30 @@ void Compiler::fgValueNumberTree(GenTree* tree)
vnStore->VNPairApplySelectorsAssign(oldLhsVNPair, fieldSeq, rhsVNPair,
lhs->TypeGet(), compCurBB);
}
lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair = newLhsVNPair;

if (lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM)
{
lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair = newLhsVNPair;
wasInSsa = true;
#ifdef DEBUG
if (verbose)
{
printf("Tree ");
Compiler::printTreeID(tree);
printf(" assigned VN to local var V%02u/%d: VN ", lclNum, lclDefSsaNum);
vnpPrint(newLhsVNPair, 1);
printf("\n");
}
#endif // DEBUG
}
}
else
{
unreached(); // "Rogue" PtrToLoc, as discussed above.
}
#ifdef DEBUG
if (verbose)
{
printf("Tree ");
Compiler::printTreeID(tree);
printf(" assigned VN to local var V%02u/%d: VN ", lclNum, lclDefSsaNum);
vnpPrint(newLhsVNPair, 1);
printf("\n");
}
#endif // DEBUG
}
else if (lvaVarAddrExposed(lclNum))

if (!wasInSsa && lvaVarAddrExposed(lclNum))
{
// Need to record the effect on ByrefExposed.
// We could use MapStore here and MapSelect on reads of address-exposed locals
Expand Down Expand Up @@ -8300,7 +8309,7 @@ void Compiler::fgValueNumberTree(GenTree* tree)
{
FieldSeqNode* fieldSeq = nullptr;
ValueNum newVN = ValueNumStore::NoVN;
if (!lvaInSsa(arg->AsLclVarCommon()->GetLclNum()))
if (!lvaInSsa(arg->AsLclVarCommon()->GetLclNum()) || !arg->AsLclVarCommon()->HasSsaName())
{
newVN = vnStore->VNForExpr(compCurBB, TYP_BYREF);
}
Expand All @@ -8315,7 +8324,6 @@ void Compiler::fgValueNumberTree(GenTree* tree)
}
if (newVN == ValueNumStore::NoVN)
{
assert(arg->AsLclVarCommon()->GetSsaNum() != ValueNumStore::NoVN);
newVN = vnStore->VNForFunc(TYP_BYREF, VNF_PtrToLoc,
vnStore->VNForIntCon(arg->AsLclVarCommon()->GetLclNum()),
vnStore->VNForFieldSeq(fieldSeq));
Expand Down Expand Up @@ -8518,7 +8526,8 @@ void Compiler::fgValueNumberTree(GenTree* tree)
VNFuncApp funcApp;

// Is it a local or a heap address?
if (addr->IsLocalAddrExpr(this, &lclVarTree, &localFldSeq) && lvaInSsa(lclVarTree->GetLclNum()))
if (addr->IsLocalAddrExpr(this, &lclVarTree, &localFldSeq) && lvaInSsa(lclVarTree->GetLclNum()) &&
lclVarTree->HasSsaName())
{
unsigned lclNum = lclVarTree->GetLclNum();
unsigned ssaNum = lclVarTree->GetSsaNum();
Expand Down
70 changes: 70 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Generated by Fuzzlyn v1.2 on 2021-08-16 20:49:50
// Run on .NET 6.0.0-dev on X86 Windows
// Seed: 5524807387112568570
// Reduced from 260.9 KiB to 0.8 KiB in 00:20:16
// Crashes the runtime
using System.Runtime.CompilerServices;

struct S0
{
public uint F0;
}

public class Runtime_57061_2
{
static S0 s_2;
static uint[] s_13;
static sbyte[][] s_110;
static int[] s_111;
private static int Main()
{
s_2 = s_2;
return Foo();
}
public static int Foo()
{
if (M34())
{
ref S0 vr1 = ref s_2;
try
{
M33();
}
finally
{
vr1.F0 = s_13[0];
}
}

for (int vr2 = 0; vr2 < Bound(); vr2++)
{
int[] vr3 = s_111;
try
{
sbyte vr4 = s_110[0][0];
}
finally
{
int vr5 = vr3[0];
}
}

return 100;
}

static bool M33()
{
return default(bool);
}

static bool M34()
{
return default(bool);
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Bound() => 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
</PropertyGroup>
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
Loading

0 comments on commit 57839c4

Please sign in to comment.