Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commite2955fc

Browse files
[NativeAOT-LLVM] Fix a GC hole in the new LSSA implementation (#2531)
* Add tests* Fix a GC hole in the new LSSA implementationWe need to be careful about not depending on stale shadow stackstate when making decision not to spill a def derived from analready spilled value.This has some cost, but an overly large one, about 0.15% in termsof code size for HelloWasm.* Add some minimal GcStress coverageWithout the fix, this test quickly crashed under stress.With the fix, it passes without issue.
1 parent166b2ee commite2955fc

File tree

6 files changed

+432
-60
lines changed

6 files changed

+432
-60
lines changed

‎src/coreclr/jit/llvmlssa.cpp‎

Lines changed: 128 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,21 @@ class ShadowStackAllocator
239239
}
240240
};
241241

242+
// We perform queries on whether a given value is GC-exposed in the context of a 'current' node.
243+
// If the current value is derived from one already spilled and still live on the shadow stack,
244+
// we can skip re-spilling it. To pass the context of whether that latter condition is true, we
245+
// use this enum. In general, we can only ascertain with precision 'active'-ness of candidate
246+
// locals and so treat the rest quite conservatively.
247+
//
248+
enumclassDefStatus
249+
{
250+
Unknown,// The stack slot (if any) associated with the def may have been overwritten.
251+
Active// The stack slot (if any) associated with the def contains its value.
252+
};
253+
242254
staticconstunsigned GC_EXPOSED_NO =0;
243-
staticconstunsigned GC_EXPOSED_YES =1;
255+
staticconstunsigned GC_EXPOSED_SPILL =1;
256+
staticconstunsigned GC_EXPOSED_YES =2;
244257
staticconstunsigned GC_EXPOSED_UNKNOWN = ValueNumStore::NoVN;
245258
staticconstunsigned LAST_ACTIVE_USE_IS_LAST_USE_BIT =1 <<31;
246259

@@ -353,15 +366,11 @@ class ShadowStackAllocator
353366
SpillSdsuValue(blockRange, retBufNode, &spillLclNum);
354367
m_liveSdsuGcDefs.AddOrUpdate(retBufNode, spillLclNum);
355368
}
356-
elseif (retBufNode->OperIsLocalRead())
369+
elseif (varTypeIsGC(retBufNode) &&IsCandidateLocalNode(retBufNode))
357370
{
358371
unsigned lclNum = retBufNode->AsLclVarCommon()->GetLclNum();
359-
LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum);
360-
if (IsCandidateLocal(varDsc))
361-
{
362-
SpillLocalValue(lclNum, retBufNode->AsLclVarCommon()->GetSsaNum()
363-
DEBUGARG("is used as a return buffer"));
364-
}
372+
unsigned ssaNum = retBufNode->AsLclVarCommon()->GetSsaNum();
373+
SpillLocalValue(lclNum, ssaNumDEBUGARG("is used as a return buffer"));
365374
}
366375
}
367376

@@ -578,7 +587,7 @@ class ShadowStackAllocator
578587
GenTreeLclVarCommon* defNode = ssaDsc->GetDefNode();
579588

580589
INDEBUG(constchar* reason);
581-
if (!IsGcExposedLocalValue(ssaDscDEBUGARG(&reason)))
590+
if (!IsGcExposedLocalValue(varDsc, ssaNum, DefStatus::ActiveDEBUGARG(&reason)))
582591
{
583592
JITDUMP("V%02u/%d %s, but did not insert a spill: %s\n", lclNum, ssaNum, spillReason, reason);
584593
return;
@@ -620,51 +629,73 @@ class ShadowStackAllocator
620629
MarkLocalSpilled(varDsc, ssaDsc);
621630
}
622631

623-
boolIsGcExposedValue(GenTree* node)
632+
boolIsGcExposedValue(GenTree* node, DefStatus defStatus)
624633
{
625634
// TODO-LLVM-LSSA-CQ: add handling for PHIs here. Take note to handle cycles properly.
626635
assert(node->IsValue());
627-
if (node->OperIsLocalRead())
636+
LclVarDsc* varDsc;
637+
if (IsCandidateLocalNode(node, &varDsc))
628638
{
629-
LclVarDsc* varDsc = m_compiler->lvaGetDesc(node->AsLclVarCommon());
630-
if (IsCandidateLocal(varDsc))
631-
{
632-
LclSsaVarDsc* ssaDsc = varDsc->GetPerSsaData(node->AsLclVarCommon()->GetSsaNum());
633-
returnIsGcExposedLocalValue(ssaDsc);
634-
}
635-
636-
// Non-candidate locals are always on the shadow stack and so not exposed.
637-
returnfalse;
639+
returnIsGcExposedLocalValue(varDsc, node->AsLclVarCommon()->GetSsaNum(), defStatus);
638640
}
639641

640-
returnIsGcExposedSdsuValue(node);
642+
returnIsGcExposedSdsuValue(node, defStatus);
641643
}
642644

643-
boolIsGcExposedSdsuValue(GenTree* node)
645+
boolIsGcExposedSdsuValue(GenTree* node, DefStatus defStatus)
644646
{
645-
assert(node->IsValue() && !node->OperIsLocal());
647+
assert(node->IsValue() && !IsCandidateLocalNode(node));
646648

647649
// Addition and subtraction of byrefs follows convenient rules that allow us to consider only
648650
// whether the "base" is exposed or not. Namely, byref arithmetic must stay within the bounds
649651
// of the underlying object, or be performed on pinned values only.
650652
if (node->OperIs(GT_ADD, GT_SUB) && node->gtGetOp2()->TypeIs(TYP_I_IMPL))
651653
{
652-
returnIsGcExposedValue(node->gtGetOp1());
654+
returnIsGcExposedValue(node->gtGetOp1(), DefStatus::Unknown);
655+
}
656+
657+
if (node->OperIsLocalRead())
658+
{
659+
// For non-candidate locals, all definitions are spilled to the shadow stack. Thus, the only
660+
// way for a value to get exposed is if something modifies the backing location. This can
661+
// happen while the SDSU is still live. E. g.:
662+
// t1 = LCL_VAR V00
663+
// CALL(&V00) ; A safe point, does V00 = null
664+
// USE(t1)
665+
// Thanks to IR invariants, the above can only occur with address-exposed locals. However,
666+
// even a 'normal' untracked local may be redefined via a local store; we handle this via
667+
// the check on 'defStatus'.
668+
//
669+
if ((defStatus == DefStatus::Unknown) ||
670+
m_compiler->lvaGetDesc(node->AsLclVarCommon())->IsAddressExposed())
671+
{
672+
returntrue;
673+
}
674+
675+
returnfalse;
653676
}
654677

655678
// Local address nodes always point to the stack (native or shadow). Constant handles will
656-
// only point to immortal and immovable (frozen) objects.
657-
return !node->OperIs(GT_LCL_ADDR) && !node->IsIconHandle() && !node->IsIntegralConst(0);
679+
// only point to immortal and immovable (frozen) objects. TODO-LLVM-LSSA-CQ: add LCLHEAP here.
680+
if (node->OperIs(GT_LCL_ADDR) || node->IsIconHandle() || node->IsIntegralConst(0))
681+
{
682+
returnfalse;
683+
}
684+
685+
returntrue;
658686
}
659687

660-
boolIsGcExposedLocalValue(LclSsaVarDsc* ssaDscDEBUGARG(constchar** pReason =nullptr))
688+
boolIsGcExposedLocalValue(
689+
LclVarDsc* varDsc,unsigned ssaNum, DefStatus defStatusDEBUGARG(constchar** pReason =nullptr))
661690
{
691+
LclSsaVarDsc* ssaDsc = varDsc->GetPerSsaData(ssaNum);
662692
unsigned status =GetGcExposedStatus(ssaDscDEBUGARG(pReason));
663693
if (status == GC_EXPOSED_UNKNOWN)
664694
{
665695
INDEBUG(constchar* reason =nullptr);
666696
GenTreeLclVarCommon* defNode = ssaDsc->GetDefNode();
667-
if ((defNode !=nullptr) && defNode->OperIs(GT_STORE_LCL_VAR) && !IsGcExposedValue(defNode->Data()))
697+
if ((defNode !=nullptr) && defNode->OperIs(GT_STORE_LCL_VAR) &&
698+
!IsGcExposedValue(defNode->Data(), DefStatus::Unknown))
668699
{
669700
INDEBUG(reason ="value not exposed");
670701
status = GC_EXPOSED_NO;
@@ -678,6 +709,26 @@ class ShadowStackAllocator
678709
SetGcExposedStatus(ssaDsc, statusDEBUGARG(reason));
679710
DBEXEC(pReason !=nullptr, *pReason = reason);
680711
}
712+
// Take care not to depend on potentially stale information. Consider:
713+
//
714+
// V00/1 = X
715+
// SAFE_POINT(...) ; V00/1 is spilled
716+
// V01/1 = V00/1
717+
//
718+
// V00/2 = Y
719+
// SAFE_POINT(...) ; V00/2 is also spilled, to the same stack slot.
720+
//
721+
// USE(V01/1) ; We have to spill V01/1, since its equal to 'X', not 'Y'
722+
// that is the active value of the spilled V00.
723+
//
724+
// TODO-LLVM-LSSA-CQ: we can be more intelligent here be allocating separate
725+
// shadow stack slots to distinct SSA definitions of the same local.
726+
//
727+
elseif ((status == GC_EXPOSED_SPILL) && (defStatus == DefStatus::Unknown) &&
728+
(m_activeDefs.Top(varDsc->lvVarIndex) != ssaNum))
729+
{
730+
status = GC_EXPOSED_YES;
731+
}
681732

682733
assert(status != GC_EXPOSED_UNKNOWN);
683734
return status == GC_EXPOSED_YES;
@@ -686,7 +737,7 @@ class ShadowStackAllocator
686737
voidSetGcExposedStatus(LclSsaVarDsc* ssaDsc,unsigned statusDEBUGARG(constchar* reason))
687738
{
688739
unsigned* pStatus =GetRawGcExposedStatusRef(ssaDsc);
689-
assert((*pStatus == GC_EXPOSED_UNKNOWN) || ((*pStatus == GC_EXPOSED_YES) && (status ==GC_EXPOSED_NO)));
740+
assert((*pStatus == GC_EXPOSED_UNKNOWN) || ((*pStatus == GC_EXPOSED_YES) && (status ==GC_EXPOSED_SPILL)));
690741
assert(status != GC_EXPOSED_UNKNOWN);
691742

692743
DBEXEC(reason !=nullptr, m_gcExposedStatusReasonMap.Set(ssaDsc, reason));
@@ -708,38 +759,42 @@ class ShadowStackAllocator
708759
voidMarkLocalSpilled(LclVarDsc* varDsc, LclSsaVarDsc* ssaDsc)
709760
{
710761
varDsc->SetRegNum(REG_STK_CANDIDATE_COMMITED);
711-
SetGcExposedStatus(ssaDsc,GC_EXPOSED_NODEBUGARG("already spilled"));
762+
SetGcExposedStatus(ssaDsc,GC_EXPOSED_SPILLDEBUGARG("already spilled"));
712763
}
713764

714765
voidProcessDef(BasicBlock* block, GenTree* node)
715766
{
716-
if (node->OperIsLocal())
767+
if (node->OperIs(GT_PHI, GT_PHI_ARG))
717768
{
718-
LclVarDsc* varDsc = m_compiler->lvaGetDesc(node->AsLclVarCommon());
719-
if (IsCandidateLocal(varDsc))
720-
{
721-
// We depend here on the conservativeness of GC in not reloading after a safepoint.
722-
assert(m_lssa->IsGcConservative());
723-
node->SetRegNum(REG_LLVM);
769+
// These are never exposed and do not participate in our use counting.
770+
return;
771+
}
724772

725-
// If this is a definition, add it to the stack of currently active ones and update liveness.
726-
unsigned ssaNum = node->AsLclVarCommon()->GetSsaNum();
727-
if (node->OperIsLocalStore())
728-
{
729-
PushActiveLocalDef(block, varDsc, ssaNum);
730-
UpdateLiveLocalDefs(varDsc, node->AsLclVarCommon()->HasLastUse(),/* isBorn*/true);
731-
}
732-
// Increment the "active" use count used for accurate last use detection. Note that skipping
733-
// unused locals here means that we could technically extend their live ranges unnecessarily
734-
// (if this was the last 'use'), but all such cases should have been DCEd by this point, and
735-
// it's not a correctness problem to skip them.
736-
elseif (!node->OperIs(GT_PHI_ARG) && !node->IsUnusedValue())
737-
{
738-
IncrementActiveUseCount(varDsc->GetPerSsaData(ssaNum));
739-
}
773+
LclVarDsc* varDsc;
774+
if (IsCandidateLocalNode(node, &varDsc))
775+
{
776+
// We depend here on the conservativeness of GC in not reloading after a safepoint.
777+
assert(m_lssa->IsGcConservative());
778+
node->SetRegNum(REG_LLVM);
779+
780+
// If this is a definition, add it to the stack of currently active ones and update liveness.
781+
unsigned ssaNum = node->AsLclVarCommon()->GetSsaNum();
782+
if (node->OperIsLocalStore())
783+
{
784+
PushActiveLocalDef(block, varDsc, ssaNum);
785+
UpdateLiveLocalDefs(varDsc, node->AsLclVarCommon()->HasLastUse(),/* isBorn*/true);
786+
}
787+
// Increment the "active" use count used for accurate last use detection. Note that skipping
788+
// unused locals here means that we could technically extend their live ranges unnecessarily
789+
// (if this was the last 'use'), but all such cases should have been DCEd by this point, and
790+
// it's not a correctness problem to skip them.
791+
elseif (!node->IsUnusedValue())
792+
{
793+
IncrementActiveUseCount(varDsc->GetPerSsaData(ssaNum));
740794
}
741795
}
742-
elseif (node->IsValue() && !node->IsUnusedValue() &&IsGcExposedType(node) &&IsGcExposedSdsuValue(node))
796+
elseif (node->IsValue() && !node->IsUnusedValue() &&IsGcExposedType(node) &&
797+
IsGcExposedSdsuValue(node, DefStatus::Active))
743798
{
744799
node->gtLIRFlags |= LIR::Flags::Mark;
745800
m_liveSdsuGcDefs.AddOrUpdate(node, BAD_VAR_NUM);
@@ -754,7 +809,8 @@ class ShadowStackAllocator
754809
}
755810
if (node->TypeIs(TYP_STRUCT))
756811
{
757-
if (node->OperIs(GT_IND, GT_PHI))
812+
// TODO-LLVM: delete this once we're up to date with upstream deleting "STORE_DYN_BLK".
813+
if (node->OperIs(GT_IND))
758814
{
759815
returnfalse;
760816
}
@@ -837,6 +893,24 @@ class ShadowStackAllocator
837893
(varDsc->GetRegNum() == REG_STK_CANDIDATE_COMMITED);
838894
}
839895

896+
boolIsCandidateLocalNode(GenTree* node, LclVarDsc** pVarDsc =nullptr)
897+
{
898+
if (node->OperIsLocal())
899+
{
900+
LclVarDsc* varDsc = m_compiler->lvaGetDesc(node->AsLclVarCommon());
901+
if (IsCandidateLocal(varDsc))
902+
{
903+
if (pVarDsc !=nullptr)
904+
{
905+
*pVarDsc = varDsc;
906+
}
907+
returntrue;
908+
}
909+
}
910+
911+
returnfalse;
912+
}
913+
840914
#ifdef DEBUG
841915
voidVerifyActiveUseCounts()
842916
{

‎src/tests/nativeaot/SmokeTests/DynamicGenerics/DynamicGenerics.csproj‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@
1616
<RdXmlFileInclude="rd.xml" />
1717
</ItemGroup>
1818

19+
<ItemGroupCondition="'$(TargetsWasm)' == 'true'">
20+
<!-- NativeAOT-LLVM: make this relatively involved test our GC stress victim.-->
21+
<IlcArgInclude="--codegenopt:JitGcStress=1" />
22+
</ItemGroup>
23+
1924
<ItemGroup>
2025
<CompileInclude="*.cs" />
2126
<CompileInclude="Internal\*.cs" />

‎src/tests/nativeaot/SmokeTests/HelloWasm/HelloWasm.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
<ProjectReferenceInclude="CpObj.ilproj" />
2929
<ProjectReferenceInclude="CkFinite.ilproj" />
3030
<ProjectReferenceInclude="ILHelpers.ilproj" />
31+
<ProjectReferenceInclude="LSSATests.IL.ilproj" />
3132
</ItemGroup>
3233

3334
<PropertyGroupCondition="$(TargetOS) != 'wasi'">

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp