From 76b5e50ad9c17d38440eddd0451371b3d41dc530 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 9 Jun 2023 14:25:32 +0200 Subject: [PATCH 1/4] JIT: Fix handling of filter successors Filters are run during the first pass of EH which makes determining their successors complicated, and the JIT did not get this right. In particular, after running filters as part of first-pass EH, control may flow to any enclosed finally or fault handler as part of second-pass EH. Thus, these should be considered as EH successors. This adds a BasicBlock::VisitEHSecondPassSuccs to visit these successors. The logic was mostly extracted from liveness that (almost) got it right: there was a condition that meant the logic did not run for top-level try-filter clauses. This change folds in Bruce's change to fix this scenario as well. There was one more bug in the logic in liveness: to determine the enclosed fault/finally blocks, the logic iterates backwards in the EH table while looking for enclosed clauses. However, this was only checking for clauses enclosed in the try region when it should also check for clauses enclosed in the handler region. Fix #86538 Fix #88168 Co-authored-by: Bruce Forstall --- src/coreclr/jit/block.cpp | 24 +++ src/coreclr/jit/block.h | 5 + src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/compiler.hpp | 156 ++++++++++++++--- src/coreclr/jit/gentree.cpp | 8 +- src/coreclr/jit/jiteh.cpp | 12 ++ src/coreclr/jit/liveness.cpp | 164 ++++++------------ src/coreclr/jit/lsra.cpp | 6 +- src/coreclr/jit/promotionliveness.cpp | 136 +++++---------- .../JitBlue/Runtime_86538/Runtime_86538.cs | 36 ++++ .../Runtime_86538/Runtime_86538.csproj | 8 + .../JitBlue/Runtime_88168/Runtime_88168.cs | 67 +++++++ .../Runtime_88168/Runtime_88168.csproj | 11 ++ 13 files changed, 402 insertions(+), 234 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.csproj create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 33dfd017e8505e..d7eabe832ac98a 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -150,6 +150,30 @@ FlowEdge* Compiler::BlockPredsWithEH(BasicBlock* blk) } } + if (ehblk->HasFinallyOrFaultHandler() && (ehblk->ebdHndBeg == blk)) + { + // block is a finally or fault handler; all enclosing filters are predecessors + unsigned enclosing = ehblk->ebdEnclosingTryIndex; + while (enclosing != EHblkDsc::NO_ENCLOSING_INDEX) + { + EHblkDsc* enclosingDsc = ehGetDsc(enclosing); + if (enclosingDsc->HasFilter()) + { + for (BasicBlock* filterBlk = enclosingDsc->ebdFilter; filterBlk != enclosingDsc->ebdHndBeg; + filterBlk = filterBlk->bbNext) + { + res = new (this, CMK_FlowEdge) FlowEdge(filterBlk, res); + + assert(filterBlk->VisitEHSecondPassSuccs(this, [blk](BasicBlock* succ) { + return succ == blk ? BasicBlockVisit::Abort : BasicBlockVisit::Continue; + }) == BasicBlockVisit::Abort); + } + } + + enclosing = enclosingDsc->ebdEnclosingTryIndex; + } + } + #ifdef DEBUG unsigned hash = SsaStressHashHelper(); if (hash != 0) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index b3f006de66ac43..cbdcd66af8590f 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1221,9 +1221,14 @@ struct BasicBlock : private LIR::Range } }; + template + BasicBlockVisit VisitEHSecondPassSuccs(Compiler* comp, TFunc func); + template BasicBlockVisit VisitAllSuccs(Compiler* comp, TFunc func); + bool HasPotentialEHSuccs(Compiler* comp); + // BBSuccList: adapter class for forward iteration of block successors, using range-based `for`, // normally used via BasicBlock::Succs(), e.g.: // for (BasicBlock* const target : block->Succs()) ... diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 01e5446bd73777..c228810d4ee18d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2224,6 +2224,7 @@ class Compiler bool bbInCatchHandlerILRange(BasicBlock* blk); bool bbInFilterILRange(BasicBlock* blk); + bool bbInFilterBBRange(BasicBlock* blk); bool bbInTryRegions(unsigned regionIndex, BasicBlock* blk); bool bbInExnFlowRegions(unsigned regionIndex, BasicBlock* blk); bool bbInHandlerRegions(unsigned regionIndex, BasicBlock* blk); @@ -4907,7 +4908,7 @@ class Compiler void fgPerNodeLocalVarLiveness(GenTreeHWIntrinsic* hwintrinsic); #endif // FEATURE_HW_INTRINSICS - VARSET_VALRET_TP fgGetHandlerLiveVars(BasicBlock* block); + void fgAddHandlerLiveVars(BasicBlock* block, VARSET_TP& ehHandlerLiveVars); void fgLiveVarAnalysis(bool updateInternalOnly = false); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index fd7809cdf848bc..ae9e180dbe83df 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -312,6 +312,94 @@ inline EHblkDsc* Compiler::ehGetBlockHndDsc(BasicBlock* block) return BasicBlockVisit::Abort; \ } +//------------------------------------------------------------------------------ +// VisitEHSecondPassSuccs: Given a block, if it is a filter (i.e. invoked in +// first-pass EH), then visit all successors that control may flow to as part +// of second-pass EH. +// +// Arguments: +// comp - Compiler instance +// func - Callback +// +// Returns: +// Whether or not the visiting should proceed. +// +// Remarks: +// This function handles the semantics of first and second pass EH where the +// EH subsystem may invoke any enclosed finally/fault right after invoking a +// filter. +// +template +BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs(Compiler* comp, TFunc func) +{ + if (!hasHndIndex()) + { + return BasicBlockVisit::Continue; + } + + const unsigned thisHndIndex = getHndIndex(); + EHblkDsc* enclosingHBtab = comp->ehGetDsc(thisHndIndex); + + if (!enclosingHBtab->InFilterRegionBBRange(this)) + { + return BasicBlockVisit::Continue; + } + + assert(enclosingHBtab->HasFilter()); + + // Search the EH table for enclosed regions. + // + // All the enclosed regions will be lower numbered and + // immediately prior to and contiguous with the enclosing + // region in the EH tab. + unsigned index = thisHndIndex; + + while (index > 0) + { + index--; + bool inTry; + unsigned enclosingIndex = comp->ehGetEnclosingRegionIndex(index, &inTry); + bool isEnclosed = false; + + // To verify this is an enclosed region, search up + // through the enclosing regions until we find the + // region associated with the filter. + while (enclosingIndex != EHblkDsc::NO_ENCLOSING_INDEX) + { + if (enclosingIndex == thisHndIndex) + { + isEnclosed = true; + break; + } + + enclosingIndex = comp->ehGetEnclosingRegionIndex(enclosingIndex, &inTry); + } + + // If we found an enclosed region, check if the region + // is a try fault or try finally, and if so, invoke the callback + // for the enclosed region's handler. + if (isEnclosed) + { + if (inTry) + { + EHblkDsc* enclosedHBtab = comp->ehGetDsc(index); + + if (enclosedHBtab->HasFinallyOrFaultHandler()) + { + RETURN_ON_ABORT(func(enclosedHBtab->ebdHndBeg)); + } + } + } + // Once we run across a non-enclosed region, we can stop searching. + else + { + break; + } + } + + return BasicBlockVisit::Continue; +} + //------------------------------------------------------------------------------ // VisitEHSuccessors: Given a block inside a handler region, visit all handlers // that control may flow to as part of EH. @@ -329,41 +417,39 @@ inline EHblkDsc* Compiler::ehGetBlockHndDsc(BasicBlock* block) // if a basic block BB1 occurs in a try block, we consider the first basic // block BB2 of the corresponding handler to be an "EH successor" of BB1. // -// TODO-BUG: This function currently does not take into account that filters -// are invoked in the first pass of EH, which means that enclosed finally -// blocks may be successors of filter blocks (as part of the second pass of -// EH). See fgGetHandlerLiveVars for code that does take this into account. -// template static BasicBlockVisit VisitEHSuccessors(Compiler* comp, BasicBlock* block, TFunc func) { - EHblkDsc* eh = comp->ehGetBlockExnFlowDsc(block); - if (eh == nullptr) + if (!block->HasPotentialEHSuccs(comp)) { return BasicBlockVisit::Continue; } - while (true) + EHblkDsc* eh = comp->ehGetBlockExnFlowDsc(block); + if (eh != nullptr) { - // If the original block whose EH successors we're iterating over - // is a BBJ_CALLFINALLY, that finally clause's first block - // will be yielded as a normal successor. Don't also yield as - // an exceptional successor. - BasicBlock* flowBlock = eh->ExFlowBlock(); - if (!block->KindIs(BBJ_CALLFINALLY) || (block->bbJumpDest != flowBlock)) + while (true) { - RETURN_ON_ABORT(func(flowBlock)); - } + // If the original block whose EH successors we're iterating over + // is a BBJ_CALLFINALLY, that finally clause's first block + // will be yielded as a normal successor. Don't also yield as + // an exceptional successor. + BasicBlock* flowBlock = eh->ExFlowBlock(); + if (!block->KindIs(BBJ_CALLFINALLY) || (block->bbJumpDest != flowBlock)) + { + RETURN_ON_ABORT(func(flowBlock)); + } - if (eh->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) - { - break; - } + if (eh->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) + { + break; + } - eh = comp->ehGetDsc(eh->ebdEnclosingTryIndex); + eh = comp->ehGetDsc(eh->ebdEnclosingTryIndex); + } } - return BasicBlockVisit::Continue; + return block->VisitEHSecondPassSuccs(comp, func); } //------------------------------------------------------------------------------ @@ -567,6 +653,32 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) #undef RETURN_ON_ABORT +//------------------------------------------------------------------------------ +// HasPotentialEHSuccs: Fast check to see if this block could have successors +// that control may flow to as part of EH. +// +// Arguments: +// comp - Compiler instance +// +// Returns: +// True if so. +// +inline bool BasicBlock::HasPotentialEHSuccs(Compiler* comp) +{ + if (hasTryIndex()) + { + return true; + } + + EHblkDsc* hndDesc = comp->ehGetBlockHndDsc(this); + if (hndDesc == nullptr) + { + return false; + } + + return hndDesc->InFilterRegionBBRange(this); +} + #if defined(FEATURE_EH_FUNCLETS) /***************************************************************************** diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 455e30df7198d5..e5a8571175e77d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18012,9 +18012,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b case GT_CNS_INT: { - if (obj->IsIconHandle(GTF_ICON_OBJ_HDL)) + if (tree->IsIconHandle(GTF_ICON_OBJ_HDL)) { - objClass = info.compCompHnd->getObjectType((CORINFO_OBJECT_HANDLE)obj->AsIntCon()->IconValue()); + objClass = info.compCompHnd->getObjectType((CORINFO_OBJECT_HANDLE)tree->AsIntCon()->IconValue()); if (objClass != NO_CLASS_HANDLE) { // if we managed to get a class handle it's definitely not null @@ -18029,14 +18029,14 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b { // If we see a RET_EXPR, recurse through to examine the // return value expression. - GenTree* retExpr = obj->AsRetExpr()->gtInlineCandidate; + GenTree* retExpr = tree->AsRetExpr()->gtInlineCandidate; objClass = gtGetClassHandle(retExpr, pIsExact, pIsNonNull); break; } case GT_CALL: { - GenTreeCall* call = obj->AsCall(); + GenTreeCall* call = tree->AsCall(); if (call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) { NamedIntrinsic ni = lookupNamedIntrinsic(call->gtCallMethHnd); diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index f59d83e6fe26fb..9fc864b26fffcd 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -347,6 +347,18 @@ bool Compiler::bbInFilterILRange(BasicBlock* blk) return HBtab->InFilterRegionILRange(blk); } +bool Compiler::bbInFilterBBRange(BasicBlock* blk) +{ + EHblkDsc* HBtab = ehGetBlockHndDsc(blk); + + if (HBtab == nullptr) + { + return false; + } + + return HBtab->InFilterRegionBBRange(blk); +} + // Given a handler region, find the innermost try region that contains it. // NOTE: handlerIndex is 1-based (0 means no handler). unsigned short Compiler::bbFindInnermostTryRegionContainingHandlerRegion(unsigned handlerIndex) diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index d45d90b90df2b3..517dc848dc6b35 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -1039,14 +1039,13 @@ void Compiler::fgExtendDbgLifetimes() } //------------------------------------------------------------------------ -// fgGetHandlerLiveVars: determine set of locals live because of implicit +// fgAddHandlerLiveVars: determine set of locals live because of implicit // exception flow from a block. // // Arguments: // block - the block in question -// -// Returns: -// Additional set of locals to be considered live throughout the block. +// ehHandlerLiveVars - On entry, contains an allocated VARSET_TP that the +// function will add handler live vars into. // // Notes: // Assumes caller has screened candidate blocks to only those with @@ -1073,116 +1072,57 @@ void Compiler::fgExtendDbgLifetimes() // { // Console.WriteLine("In catch 1"); // } - -VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block) +// +void Compiler::fgAddHandlerLiveVars(BasicBlock* block, VARSET_TP& ehHandlerLiveVars) { - noway_assert(block); - noway_assert(ehBlockHasExnFlowDsc(block)); + assert(block->HasPotentialEHSuccs(this)); - VARSET_TP liveVars(VarSetOps::MakeEmpty(this)); - EHblkDsc* HBtab = ehGetBlockExnFlowDsc(block); - - do + if (ehBlockHasExnFlowDsc(block)) { - /* Either we enter the filter first or the catch/finally */ - if (HBtab->HasFilter()) + 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); - } - - /* 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); - - } 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(); - EHblkDsc* enclosingHBtab = ehGetDsc(thisHndIndex); - - if (enclosingHBtab->InFilterRegionBBRange(block)) - { - assert(enclosingHBtab->HasFilter()); - - // Search the EH table for enclosed regions. - // - // All the enclosed regions will be lower numbered and - // immediately prior to and contiguous with the enclosing - // region in the EH tab. - unsigned index = thisHndIndex; - - while (index > 0) + } + else { - index--; - unsigned enclosingIndex = ehGetEnclosingTryIndex(index); - bool isEnclosed = false; - - // To verify this is an enclosed region, search up - // through the enclosing regions until we find the - // region associated with the filter. - while (enclosingIndex != EHblkDsc::NO_ENCLOSING_INDEX) - { - if (enclosingIndex == thisHndIndex) - { - isEnclosed = true; - break; - } - - enclosingIndex = ehGetEnclosingTryIndex(enclosingIndex); - } + VarSetOps::UnionD(this, ehHandlerLiveVars, HBtab->ebdHndBeg->bbLiveIn); + } - // If we found an enclosed region, check if the region - // is a try fault or try finally, and if so, add any - // locals live into the enclosed region's handler into this - // block's live-in set. - if (isEnclosed) - { - EHblkDsc* enclosedHBtab = ehGetDsc(index); + /* If we have nested try's edbEnclosing will provide them */ + noway_assert((HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) || + (HBtab->ebdEnclosingTryIndex > ehGetIndex(HBtab))); - if (enclosedHBtab->HasFinallyOrFaultHandler()) - { - VarSetOps::UnionD(this, liveVars, enclosedHBtab->ebdHndBeg->bbLiveIn); - } - } - // Once we run across a non-enclosed region, we can stop searching. - else - { - break; - } + unsigned outerIndex = HBtab->ebdEnclosingTryIndex; + if (outerIndex == EHblkDsc::NO_ENCLOSING_INDEX) + { + break; } - } + HBtab = ehGetDsc(outerIndex); + + } while (true); } - return liveVars; + if (bbInFilterBBRange(block)) + { + block->VisitEHSecondPassSuccs(this, [this, &ehHandlerLiveVars](BasicBlock* succ) { + VarSetOps::UnionD(this, ehHandlerLiveVars, succ->bbLiveIn); + return BasicBlockVisit::Continue; + }); + } } class LiveVarAnalysis @@ -1195,6 +1135,7 @@ class LiveVarAnalysis unsigned m_memoryLiveOut; VARSET_TP m_liveIn; VARSET_TP m_liveOut; + VARSET_TP m_ehHandlerLiveVars; LiveVarAnalysis(Compiler* compiler) : m_compiler(compiler) @@ -1203,6 +1144,7 @@ class LiveVarAnalysis , m_memoryLiveOut(emptyMemoryKindSet) , m_liveIn(VarSetOps::MakeEmpty(compiler)) , m_liveOut(VarSetOps::MakeEmpty(compiler)) + , m_ehHandlerLiveVars(VarSetOps::MakeEmpty(compiler)) { } @@ -1278,11 +1220,12 @@ 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 (block->HasPotentialEHSuccs(m_compiler)) { - const VARSET_TP& liveVars(m_compiler->fgGetHandlerLiveVars(block)); - VarSetOps::UnionD(m_compiler, m_liveIn, liveVars); - VarSetOps::UnionD(m_compiler, m_liveOut, liveVars); + VarSetOps::ClearD(m_compiler, m_ehHandlerLiveVars); + m_compiler->fgAddHandlerLiveVars(block, m_ehHandlerLiveVars); + 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. @@ -2568,6 +2511,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 */ @@ -2576,12 +2521,11 @@ void Compiler::fgInterBlockLocalVarLiveness() /* Remember those vars live on entry to exception handlers */ /* if we are part of a try block */ + VarSetOps::ClearD(this, volatileVars); - VARSET_TP volatileVars(VarSetOps::MakeEmpty(this)); - - if (ehBlockHasExnFlowDsc(block)) + if (block->HasPotentialEHSuccs(this)) { - VarSetOps::Assign(this, volatileVars, fgGetHandlerLiveVars(block)); + fgAddHandlerLiveVars(block, volatileVars); // volatileVars is a subset of exceptVars noway_assert(VarSetOps::IsSubset(this, volatileVars, exceptVars)); diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index b33bd2ab325237..182279e96f7bfe 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -2396,9 +2396,11 @@ 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)) + if (block->HasPotentialEHSuccs(compiler)) { - VarSetOps::DiffD(compiler, liveInNotComputedLive, compiler->fgGetHandlerLiveVars(block)); + VARSET_TP ehHandlerLiveVars(VarSetOps::MakeEmpty(compiler)); + compiler->fgAddHandlerLiveVars(block, ehHandlerLiveVars); + VarSetOps::DiffD(compiler, liveInNotComputedLive, ehHandlerLiveVars); } VarSetOps::Iter liveInNotComputedLiveIter(compiler, liveInNotComputedLive); unsigned liveInNotComputedLiveIndex = 0; diff --git a/src/coreclr/jit/promotionliveness.cpp b/src/coreclr/jit/promotionliveness.cpp index 9791bd08a0d90b..88678ab07c73d0 100644 --- a/src/coreclr/jit/promotionliveness.cpp +++ b/src/coreclr/jit/promotionliveness.cpp @@ -352,7 +352,7 @@ bool PromotionLiveness::PerBlockLiveness(BasicBlock* block) BitVecOps::LivenessD(m_bvTraits, m_liveIn, bbInfo.VarDef, bbInfo.VarUse, bbInfo.LiveOut); - if (m_compiler->ehBlockHasExnFlowDsc(block)) + if (block->HasPotentialEHSuccs(m_compiler)) { BitVecOps::ClearD(m_bvTraits, m_ehLiveVars); AddHandlerLiveVars(block, m_ehLiveVars); @@ -374,118 +374,64 @@ bool PromotionLiveness::PerBlockLiveness(BasicBlock* block) //------------------------------------------------------------------------ // AddHandlerLiveVars: // Find variables that are live-in to handlers reachable by implicit control -// flow and add them to a specified bit vector. +// flow and return them in the specified bit vector. // // Parameters: // block - The block -// ehLiveVars - The bit vector to mark in +// ehLiveVars - The bit vector to mark in. // // Remarks: // Similar to Compiler::fgGetHandlerLiveVars used by regular liveness. // void PromotionLiveness::AddHandlerLiveVars(BasicBlock* block, BitVec& ehLiveVars) { - assert(m_compiler->ehBlockHasExnFlowDsc(block)); - EHblkDsc* HBtab = m_compiler->ehGetBlockExnFlowDsc(block); + assert(block->HasPotentialEHSuccs(m_compiler)); - 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); - } - - // 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); - - } 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(); - EHblkDsc* enclosingHBtab = m_compiler->ehGetDsc(thisHndIndex); - - if (enclosingHBtab->InFilterRegionBBRange(block)) - { - assert(enclosingHBtab->HasFilter()); + } + else + { + BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[HBtab->ebdHndBeg->bbNum].LiveIn); + } - // Search the EH table for enclosed regions. - // - // All the enclosed regions will be lower numbered and - // immediately prior to and contiguous with the enclosing - // region in the EH tab. - unsigned index = thisHndIndex; + // If we have nested try's edbEnclosing will provide them + assert((HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) || + (HBtab->ebdEnclosingTryIndex > m_compiler->ehGetIndex(HBtab))); - while (index > 0) + unsigned outerIndex = HBtab->ebdEnclosingTryIndex; + if (outerIndex == EHblkDsc::NO_ENCLOSING_INDEX) { - index--; - unsigned enclosingIndex = m_compiler->ehGetEnclosingTryIndex(index); - bool isEnclosed = false; - - // To verify this is an enclosed region, search up - // through the enclosing regions until we find the - // region associated with the filter. - while (enclosingIndex != EHblkDsc::NO_ENCLOSING_INDEX) - { - if (enclosingIndex == thisHndIndex) - { - isEnclosed = true; - break; - } - - enclosingIndex = m_compiler->ehGetEnclosingTryIndex(enclosingIndex); - } + break; + } + HBtab = m_compiler->ehGetDsc(outerIndex); - // If we found an enclosed region, check if the region - // is a try fault or try finally, and if so, add any - // locals live into the enclosed region's handler into this - // block's live-in set. - if (isEnclosed) - { - EHblkDsc* enclosedHBtab = m_compiler->ehGetDsc(index); + } while (true); + } - if (enclosedHBtab->HasFinallyOrFaultHandler()) - { - BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[enclosedHBtab->ebdHndBeg->bbNum].LiveIn); - } - } - // Once we run across a non-enclosed region, we can stop searching. - else - { - break; - } - } - } + if (m_compiler->bbInFilterBBRange(block)) + { + block->VisitEHSecondPassSuccs(m_compiler, [this, &ehLiveVars](BasicBlock* succ) { + BitVecOps::UnionD(m_bvTraits, ehLiveVars, m_bbInfo[succ->bbNum].LiveIn); + return BasicBlockVisit::Continue; + }); } } @@ -510,7 +456,7 @@ void PromotionLiveness::FillInLiveness() BitVecOps::ClearD(m_bvTraits, volatileVars); - if (m_compiler->ehBlockHasExnFlowDsc(block)) + if (block->HasPotentialEHSuccs(m_compiler)) { AddHandlerLiveVars(block, volatileVars); } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.cs b/src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.cs new file mode 100644 index 00000000000000..e53db71ebab6b0 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.cs @@ -0,0 +1,36 @@ +// 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 Xunit; + +public class Runtime_86538 +{ + [Fact] + public static int TestEntryPoint() + { + int result = 99; + int finallyResult = -1; + try + { + try + { + throw new Exception(); + } + finally + { + finallyResult = result; + } + } + catch when (result++ == 99) + { + } + + if (finallyResult != 100) + { + Console.WriteLine("FAIL: finallyResult == {0}", finallyResult); + } + + return finallyResult; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.csproj new file mode 100644 index 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_86538/Runtime_86538.csproj @@ -0,0 +1,8 @@ + + + True + + + + + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.cs b/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.cs new file mode 100644 index 00000000000000..a5178613d5a62e --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.cs @@ -0,0 +1,67 @@ +// 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 Xunit; + +// Repro for https://github.com/dotnet/runtime/issues/88168. +// Derived from (and similar to) test GitHub_22820 for https://github.com/dotnet/coreclr/issues/22820. +// +// Run with optimized codegen and DOTNET_GCStress=0x4 + +class DisposableObject : IDisposable +{ + public void Dispose() + { + Console.WriteLine("In dispose"); + } +} + +public class Program +{ + public static bool IsExpectedException(Exception e) + { + Console.WriteLine("In filter"); + GC.Collect(); + return e is OperationCanceledException; + } + + public static IDisposable AllocateObject() + { + return new DisposableObject(); + } + + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)] + private static void top_level_filter_test() + { + try + { + using (AllocateObject()) + { + throw new Exception(); + } + } + catch (Exception e1) when (IsExpectedException(e1)) + { + Console.WriteLine("In catch 1"); + } + } + + [Fact] + public static int TestEntryPoint() + { + int result = 0; + + try + { + top_level_filter_test(); + } + catch (Exception e2) + { + Console.WriteLine("In catch 2"); + result = 100; + } + + return result; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj new file mode 100644 index 00000000000000..33612751ddc799 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj @@ -0,0 +1,11 @@ + + + + true + True + + + + + + \ No newline at end of file From 31e74a6877bd641fa3fb98952bf3b013ec7172e8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jul 2023 00:59:49 +0200 Subject: [PATCH 2/4] Revert unrelated change --- src/coreclr/jit/gentree.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index e5a8571175e77d..455e30df7198d5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18012,9 +18012,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b case GT_CNS_INT: { - if (tree->IsIconHandle(GTF_ICON_OBJ_HDL)) + if (obj->IsIconHandle(GTF_ICON_OBJ_HDL)) { - objClass = info.compCompHnd->getObjectType((CORINFO_OBJECT_HANDLE)tree->AsIntCon()->IconValue()); + objClass = info.compCompHnd->getObjectType((CORINFO_OBJECT_HANDLE)obj->AsIntCon()->IconValue()); if (objClass != NO_CLASS_HANDLE) { // if we managed to get a class handle it's definitely not null @@ -18029,14 +18029,14 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b { // If we see a RET_EXPR, recurse through to examine the // return value expression. - GenTree* retExpr = tree->AsRetExpr()->gtInlineCandidate; + GenTree* retExpr = obj->AsRetExpr()->gtInlineCandidate; objClass = gtGetClassHandle(retExpr, pIsExact, pIsNonNull); break; } case GT_CALL: { - GenTreeCall* call = tree->AsCall(); + GenTreeCall* call = obj->AsCall(); if (call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) { NamedIntrinsic ni = lookupNamedIntrinsic(call->gtCallMethHnd); From b9b29589b7b378a1d4a0e236e65d5ef9c8ab177c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jul 2023 01:03:06 +0200 Subject: [PATCH 3/4] Add function header --- src/coreclr/jit/jiteh.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 9fc864b26fffcd..a257ebc173502e 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -347,6 +347,16 @@ bool Compiler::bbInFilterILRange(BasicBlock* blk) return HBtab->InFilterRegionILRange(blk); } +//------------------------------------------------------------------------ +// bbInFilterBBRange: +// Check if this block is part of a filter. +// +// Arguments: +// blk - The block +// +// Return Value: +// True if the block is part of a filter clause. Otherwise false. +// bool Compiler::bbInFilterBBRange(BasicBlock* blk) { EHblkDsc* HBtab = ehGetBlockHndDsc(blk); @@ -446,7 +456,7 @@ bool Compiler::bbInTryRegions(unsigned regionIndex, BasicBlock* blk) // Notes: // For this check, a funclet is considered to be in the region it was // extracted from. - +// bool Compiler::bbInExnFlowRegions(unsigned regionIndex, BasicBlock* blk) { assert(regionIndex < EHblkDsc::NO_ENCLOSING_INDEX); From 75b67e81939d9e89d5dfae01533d13fb3300e835 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jul 2023 12:08:41 +0200 Subject: [PATCH 4/4] Remove forced GCStress in test Leave it up to the GCStress jobs instead. Some platforms do not support GCStress. --- .../JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj index 33612751ddc799..15edd99711a1a4 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_88168/Runtime_88168.csproj @@ -1,11 +1,8 @@ - - true True - \ No newline at end of file