Skip to content

Fix GC hole with filter liveness #89228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BruceForstall wants to merge 1 commit into from
Closed

Fix GC hole with filter liveness #89228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4899,7 +4899,7 @@ class Compiler
void fgPerNodeLocalVarLiveness(GenTreeHWIntrinsic* hwintrinsic);
#endif // FEATURE_HW_INTRINSICS

VARSET_VALRET_TP fgGetHandlerLiveVars(BasicBlock* block);
bool fgGetHandlerLiveVars(BasicBlock* block, VARSET_TP& ehHandlerLiveVars);

void fgLiveVarAnalysis(bool updateInternalOnly = false);

Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/jit/jiteh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ bool Compiler::ehIsBlockEHLast(BasicBlock* block)
//------------------------------------------------------------------------
// ehGetBlockExnFlowDsc:
// Get the EH descriptor for the most nested region (if any) that may
// handle exceptions raised in the given block
// handle exceptions raised in the given block.
//
// Arguments:
// block - Consider exceptions raised from this block
Expand All @@ -658,7 +658,7 @@ bool Compiler::ehIsBlockEHLast(BasicBlock* block)
// nullptr - The given block's exceptions propagate to caller
// non-null - This region is the innermost handler for exceptions raised in
// the given block

//
EHblkDsc* Compiler::ehGetBlockExnFlowDsc(BasicBlock* block)
{
EHblkDsc* hndDesc = ehGetBlockHndDsc(block);
Expand All @@ -668,9 +668,10 @@ EHblkDsc* Compiler::ehGetBlockExnFlowDsc(BasicBlock* block)
// If an exception is thrown in a filter (or escapes a callee in a filter),
// or if exception_continue_search (0/false) is returned at
// the end of a filter, the (original) exception is propagated to
// the next outer handler. The "next outer handler" is the handler
// of the try region enclosing the try that the filter protects.
// This may not be the same as the try region enclosing the filter,
// the next outer handler. This is because a new exception thrown in
// a filter is discarded and the filter is treated as returning 0.
// The "next outer handler" is the handler of the try region enclosing the try
// that the filter protects. This may not be the same as the try region enclosing the filter,
// e.g. in cases like this:
// try {
// ...
Expand Down
98 changes: 51 additions & 47 deletions src/coreclr/jit/liveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,9 +1044,11 @@ void Compiler::fgExtendDbgLifetimes()
//
// Arguments:
// block - the block in question
// ehHandlerLiveVars - On entry, contains an allocated VARSET_TP. On exit, contains the set of handler live vars.
//
// Returns:
// Additional set of locals to be considered live throughout the block.
// `true` if there are any EH live vars (`ehHandlerLiveVars` is updated in this case).
// `false` if there are no EH live vars.
//
// Notes:
// Assumes caller has screened candidate blocks to only those with
Expand All @@ -1073,57 +1075,59 @@ void Compiler::fgExtendDbgLifetimes()
// {
// Console.WriteLine("In catch 1");
// }

VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block)
//
bool Compiler::fgGetHandlerLiveVars(BasicBlock* block, VARSET_TP& ehHandlerLiveVars)
{
noway_assert(block);
noway_assert(ehBlockHasExnFlowDsc(block));
bool anyEhVars = false;

VARSET_TP liveVars(VarSetOps::MakeEmpty(this));
EHblkDsc* HBtab = ehGetBlockExnFlowDsc(block);
VarSetOps::ClearD(this, ehHandlerLiveVars);

do
if (ehBlockHasExnFlowDsc(block))
{
/* Either we enter the filter first or the catch/finally */
if (HBtab->HasFilter())
anyEhVars = true;

EHblkDsc* HBtab = ehGetBlockExnFlowDsc(block);

do
{
VarSetOps::UnionD(this, liveVars, HBtab->ebdFilter->bbLiveIn);
/* Either we enter the filter first or the catch/finally */
if (HBtab->HasFilter())
{
VarSetOps::UnionD(this, ehHandlerLiveVars, HBtab->ebdFilter->bbLiveIn);
#if defined(FEATURE_EH_FUNCLETS)
// The EH subsystem can trigger a stack walk after the filter
// has returned, but before invoking the handler, and the only
// IP address reported from this method will be the original
// faulting instruction, thus everything in the try body
// must report as live any variables live-out of the filter
// (which is the same as those live-in to the handler)
VarSetOps::UnionD(this, liveVars, HBtab->ebdHndBeg->bbLiveIn);
// The EH subsystem can trigger a stack walk after the filter
// has returned, but before invoking the handler, and the only
// IP address reported from this method will be the original
// faulting instruction, thus everything in the try body
// must report as live any variables live-out of the filter
// (which is the same as those live-in to the handler)
VarSetOps::UnionD(this, ehHandlerLiveVars, HBtab->ebdHndBeg->bbLiveIn);
#endif // FEATURE_EH_FUNCLETS
}
else
{
VarSetOps::UnionD(this, liveVars, HBtab->ebdHndBeg->bbLiveIn);
}
}
else
{
VarSetOps::UnionD(this, ehHandlerLiveVars, HBtab->ebdHndBeg->bbLiveIn);
}

/* If we have nested try's edbEnclosing will provide them */
noway_assert((HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) ||
(HBtab->ebdEnclosingTryIndex > ehGetIndex(HBtab)));
/* If we have nested try's edbEnclosing will provide them */
noway_assert((HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) ||
(HBtab->ebdEnclosingTryIndex > ehGetIndex(HBtab)));

unsigned outerIndex = HBtab->ebdEnclosingTryIndex;
if (outerIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
break;
}
HBtab = ehGetDsc(outerIndex);
unsigned outerIndex = HBtab->ebdEnclosingTryIndex;
if (outerIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
break;
}
HBtab = ehGetDsc(outerIndex);

} while (true);
} while (true);
}

// If this block is within a filter, we also need to report as live
// any vars live into enclosed finally or fault handlers, since the
// filter will run during the first EH pass, and enclosed or enclosing
// handlers will run during the second EH pass. So all these handlers
// are "exception flow" successors of the filter.
//
// Note we are relying on ehBlockHasExnFlowDsc to return true
// for any filter block that we should examine here.
if (block->hasHndIndex())
{
const unsigned thisHndIndex = block->getHndIndex();
Expand Down Expand Up @@ -1170,7 +1174,8 @@ VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block)

if (enclosedHBtab->HasFinallyOrFaultHandler())
{
VarSetOps::UnionD(this, liveVars, enclosedHBtab->ebdHndBeg->bbLiveIn);
anyEhVars = true;
VarSetOps::UnionD(this, ehHandlerLiveVars, enclosedHBtab->ebdHndBeg->bbLiveIn);
}
}
// Once we run across a non-enclosed region, we can stop searching.
Expand All @@ -1182,7 +1187,7 @@ VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block)
}
}

return liveVars;
return anyEhVars;
}

class LiveVarAnalysis
Expand All @@ -1195,6 +1200,7 @@ class LiveVarAnalysis
unsigned m_memoryLiveOut;
VARSET_TP m_liveIn;
VARSET_TP m_liveOut;
VARSET_TP m_ehHandlerLiveVars;

LiveVarAnalysis(Compiler* compiler)
: m_compiler(compiler)
Expand All @@ -1203,6 +1209,7 @@ class LiveVarAnalysis
, m_memoryLiveOut(emptyMemoryKindSet)
, m_liveIn(VarSetOps::MakeEmpty(compiler))
, m_liveOut(VarSetOps::MakeEmpty(compiler))
, m_ehHandlerLiveVars(VarSetOps::MakeEmpty(compiler))
{
}

Expand Down Expand Up @@ -1278,11 +1285,10 @@ class LiveVarAnalysis

// Does this block have implicit exception flow to a filter or handler?
// If so, include the effects of that flow.
if (m_compiler->ehBlockHasExnFlowDsc(block))
if (m_compiler->fgGetHandlerLiveVars(block, m_ehHandlerLiveVars))
{
const VARSET_TP& liveVars(m_compiler->fgGetHandlerLiveVars(block));
VarSetOps::UnionD(m_compiler, m_liveIn, liveVars);
VarSetOps::UnionD(m_compiler, m_liveOut, liveVars);
VarSetOps::UnionD(m_compiler, m_liveIn, m_ehHandlerLiveVars);
VarSetOps::UnionD(m_compiler, m_liveOut, m_ehHandlerLiveVars);

// Implicit eh edges can induce loop-like behavior,
// so make sure we iterate to closure.
Expand Down Expand Up @@ -2568,6 +2574,8 @@ void Compiler::fgInterBlockLocalVarLiveness()
* Now fill in liveness info within each basic block - Backward DataFlow
*/

VARSET_TP volatileVars(VarSetOps::MakeEmpty(this));

for (BasicBlock* const block : Blocks())
{
/* Tell everyone what block we're working on */
Expand All @@ -2577,12 +2585,8 @@ void Compiler::fgInterBlockLocalVarLiveness()
/* Remember those vars live on entry to exception handlers */
/* if we are part of a try block */

VARSET_TP volatileVars(VarSetOps::MakeEmpty(this));

if (ehBlockHasExnFlowDsc(block))
if (fgGetHandlerLiveVars(block, volatileVars))
{
VarSetOps::Assign(this, volatileVars, fgGetHandlerLiveVars(block));

// volatileVars is a subset of exceptVars
noway_assert(VarSetOps::IsSubset(this, volatileVars, exceptVars));
}
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/lsra.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2396,9 +2396,10 @@ void LinearScan::checkLastUses(BasicBlock* block)
VARSET_TP liveInNotComputedLive(VarSetOps::Diff(compiler, block->bbLiveIn, computedLive));

// We may have exception vars in the liveIn set of exception blocks that are not computed live.
if (compiler->ehBlockHasExnFlowDsc(block))
VARSET_TP ehHandlerLiveVars(VarSetOps::MakeEmpty(compiler));
if (compiler->fgGetHandlerLiveVars(block, ehHandlerLiveVars))
{
VarSetOps::DiffD(compiler, liveInNotComputedLive, compiler->fgGetHandlerLiveVars(block));
VarSetOps::DiffD(compiler, liveInNotComputedLive, ehHandlerLiveVars);
}
VarSetOps::Iter liveInNotComputedLiveIter(compiler, liveInNotComputedLive);
unsigned liveInNotComputedLiveIndex = 0;
Expand Down
75 changes: 35 additions & 40 deletions src/coreclr/jit/promotionliveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,9 @@ bool PromotionLiveness::PerBlockLiveness(BasicBlock* block)

BitVecOps::LivenessD(m_bvTraits, m_liveIn, bbInfo.VarDef, bbInfo.VarUse, bbInfo.LiveOut);

if (m_compiler->ehBlockHasExnFlowDsc(block))
AddHandlerLiveVars(block, m_ehLiveVars);
if (!BitVecOps::IsEmpty(m_bvTraits, m_ehLiveVars))
{
BitVecOps::ClearD(m_bvTraits, m_ehLiveVars);
AddHandlerLiveVars(block, m_ehLiveVars);
BitVecOps::UnionD(m_bvTraits, m_liveIn, m_ehLiveVars);
BitVecOps::UnionD(m_bvTraits, bbInfo.LiveOut, m_ehLiveVars);
m_hasPossibleBackEdge = true;
Expand Down Expand Up @@ -385,51 +384,52 @@ bool PromotionLiveness::PerBlockLiveness(BasicBlock* block)
//
void PromotionLiveness::AddHandlerLiveVars(BasicBlock* block, BitVec& ehLiveVars)
{
assert(m_compiler->ehBlockHasExnFlowDsc(block));
EHblkDsc* HBtab = m_compiler->ehGetBlockExnFlowDsc(block);
BitVecOps::ClearD(m_bvTraits, ehLiveVars);

do
if (m_compiler->ehBlockHasExnFlowDsc(block))
{
// Either we enter the filter first or the catch/finally
if (HBtab->HasFilter())
EHblkDsc* HBtab = m_compiler->ehGetBlockExnFlowDsc(block);

do
{
BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdFilter->bbNum].LiveIn);
// Either we enter the filter first or the catch/finally
if (HBtab->HasFilter())
{
BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdFilter->bbNum].LiveIn);
#if defined(FEATURE_EH_FUNCLETS)
// The EH subsystem can trigger a stack walk after the filter
// has returned, but before invoking the handler, and the only
// IP address reported from this method will be the original
// faulting instruction, thus everything in the try body
// must report as live any variables live-out of the filter
// (which is the same as those live-in to the handler)
BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdHndBeg->bbNum].LiveIn);
// The EH subsystem can trigger a stack walk after the filter
// has returned, but before invoking the handler, and the only
// IP address reported from this method will be the original
// faulting instruction, thus everything in the try body
// must report as live any variables live-out of the filter
// (which is the same as those live-in to the handler)
BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdHndBeg->bbNum].LiveIn);
#endif // FEATURE_EH_FUNCLETS
}
else
{
BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdHndBeg->bbNum].LiveIn);
}
}
else
{
BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdHndBeg->bbNum].LiveIn);
}

// If we have nested try's edbEnclosing will provide them
assert((HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) ||
(HBtab->ebdEnclosingTryIndex > m_compiler->ehGetIndex(HBtab)));
// If we have nested try's edbEnclosing will provide them
assert((HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) ||
(HBtab->ebdEnclosingTryIndex > m_compiler->ehGetIndex(HBtab)));

unsigned outerIndex = HBtab->ebdEnclosingTryIndex;
if (outerIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
break;
}
HBtab = m_compiler->ehGetDsc(outerIndex);
unsigned outerIndex = HBtab->ebdEnclosingTryIndex;
if (outerIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
break;
}
HBtab = m_compiler->ehGetDsc(outerIndex);

} while (true);
} while (true);
}

// If this block is within a filter, we also need to report as live
// any vars live into enclosed finally or fault handlers, since the
// filter will run during the first EH pass, and enclosed or enclosing
// handlers will run during the second EH pass. So all these handlers
// are "exception flow" successors of the filter.
//
// Note we are relying on ehBlockHasExnFlowDsc to return true
// for any filter block that we should examine here.
if (block->hasHndIndex())
{
const unsigned thisHndIndex = block->getHndIndex();
Expand Down Expand Up @@ -508,12 +508,7 @@ void PromotionLiveness::FillInLiveness()

BasicBlockLiveness& bbInfo = m_bbInfo[block->bbNum];

BitVecOps::ClearD(m_bvTraits, volatileVars);

if (m_compiler->ehBlockHasExnFlowDsc(block))
{
AddHandlerLiveVars(block, volatileVars);
}
AddHandlerLiveVars(block, volatileVars);

BitVecOps::Assign(m_bvTraits, life, bbInfo.LiveOut);

Expand Down
Loading