From: Jakob Botsch Nielsen Date: Tue, 17 Aug 2021 17:48:38 +0000 (+0200) Subject: Add checks for missing SSA numbers (#57274) X-Git-Tag: accepted/tizen/unified/20220110.054933~324 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=57839c4c8a28006aacae49a1ddf8e9239fdc7edb;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Add checks for missing SSA numbers (#57274) 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. --- diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 917aa94..52246f8 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -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. @@ -2991,8 +2991,8 @@ GenTree* Compiler::optCopyAssertionProp(AssertionDsc* curAssertion, return nullptr; } - tree->SetSsaNum(copySsaNum); tree->SetLclNum(copyLclNum); + tree->SetSsaNum(copySsaNum); #ifdef DEBUG if (verbose) diff --git a/src/coreclr/jit/earlyprop.cpp b/src/coreclr/jit/earlyprop.cpp index f30b33e..70eeea7 100644 --- a/src/coreclr/jit/earlyprop.cpp +++ b/src/coreclr/jit/earlyprop.cpp @@ -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(); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 6bea38f..fd17d74 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -6340,7 +6340,7 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* 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( @@ -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(); @@ -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()) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d4033ef..0e92407 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -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) @@ -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); @@ -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; @@ -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; } @@ -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. @@ -7955,6 +7956,7 @@ void Compiler::fgValueNumberTree(GenTree* tree) wasLocal = true; + bool wasInSsa = false; if (lvaInSsa(lclNum)) { FieldSeqNode* fieldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]); @@ -7962,12 +7964,11 @@ void Compiler::fgValueNumberTree(GenTree* tree) // 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()); @@ -7988,6 +7989,8 @@ void Compiler::fgValueNumberTree(GenTree* tree) rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet())); } + unsigned lclDefSsaNum; + ValueNumPair newLhsVNPair; if (isEntire) { newLhsVNPair = rhsVNPair; @@ -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 @@ -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); } @@ -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)); @@ -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(); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.cs b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.cs new file mode 100644 index 0000000..29004d9 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.cs @@ -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; +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.csproj new file mode 100644 index 0000000..f3e1cbd --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_2.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_3.cs b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_3.cs new file mode 100644 index 0000000..c779981 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_3.cs @@ -0,0 +1,85 @@ +// 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 22:20:52 +// Run on .NET 6.0.0-dev on X86 Windows +// Seed: 15888848110967612195 +// Reduced from 235.4 KiB to 1.4 KiB in 00:22:05 +// Crashes the runtime +using System.Runtime.CompilerServices; + +class C0 +{ + public ushort F0; + public short F1; + public ulong F2; + public short F3; + public C0(ushort f0, ulong f2, short f3) + { + F0 = f0; + F2 = f2; + F3 = f3; + } +} + +class C1 +{ + public uint F1; +} + +struct S0 +{ + public byte F0; + public C0 F3; +} + +public class Runtime_57061_3 +{ + static long s_2; + static S0[][][] s_3; + static S0 s_4; + public static int Main() + { + C1 vr3 = default(C1); + for (int vr4 = 0; vr4 < Bound(); vr4++) + { + try + { + System.Console.WriteLine(vr3.F1); + } + finally + { + s_2 = s_2; + } + + bool vr5 = false; + if (vr5) + { + try + { + vr5 &= 0 == s_4.F3.F1; + } + finally + { + var vr6 = new C0(1, 1, 0); + var vr7 = new C0(0, 0, 0); + S0 vr10 = s_3[0][0][0]; + byte vr11 = s_4.F0++; + uint vr12 = default(uint); + var vr8 = (byte)vr12; + M4(ref s_3[0][0][0], vr8); + } + } + } + + return 100; + } + + static uint M4(ref S0 arg2, byte arg3) + { + return default(uint); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Bound() => 0; +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_3.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_3.csproj new file mode 100644 index 0000000..f3e1cbd --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_57061/Runtime_57061_3.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + +