From 94849dcfc8c08f08ec74b61d860826c582442469 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Wed, 11 Oct 2023 23:55:49 +0300 Subject: [PATCH 1/2] Remove BBF_TRY_BEG The flag duplicates information already present in the EH table. Also delete some remnants of verification from "impVerifyEHBlock". --- src/coreclr/jit/block.cpp | 4 -- src/coreclr/jit/block.h | 2 +- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgbasic.cpp | 9 +--- src/coreclr/jit/fgdiagnostic.cpp | 12 +++--- src/coreclr/jit/fgehopt.cpp | 14 +------ src/coreclr/jit/fgopt.cpp | 15 ++++--- src/coreclr/jit/flowgraph.cpp | 2 +- src/coreclr/jit/importer.cpp | 72 ++++---------------------------- src/coreclr/jit/jiteh.cpp | 44 ++++++++----------- src/coreclr/jit/optimizer.cpp | 6 +-- 11 files changed, 49 insertions(+), 133 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index d24e360eb82c47..dcf6cb843e42cd 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -399,10 +399,6 @@ void BasicBlock::dspFlags() { printf("failV "); } - if (bbFlags & BBF_TRY_BEG) - { - printf("try "); - } if (bbFlags & BBF_RUN_RARELY) { printf("rare "); diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index fb48792ca856c6..177175f92f46f4 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -371,7 +371,7 @@ enum BasicBlockFlags : unsigned __int64 BBF_IMPORTED = MAKE_BBFLAG( 4), // BB byte-code has been imported BBF_INTERNAL = MAKE_BBFLAG( 5), // BB has been added by the compiler BBF_FAILED_VERIFICATION = MAKE_BBFLAG( 6), // BB has verification exception - BBF_TRY_BEG = MAKE_BBFLAG( 7), // BB starts a 'try' block +// BBF_UNUSED = MAKE_BBFLAG( 7), BBF_FUNCLET_BEG = MAKE_BBFLAG( 8), // BB is the beginning of a funclet BBF_CLONED_FINALLY_BEGIN = MAKE_BBFLAG( 9), // First block of a cloned finally region BBF_CLONED_FINALLY_END = MAKE_BBFLAG(10), // Last block of a cloned finally region diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 3dd93a0451f455..14e416caa221f6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4259,7 +4259,7 @@ class Compiler void impReimportMarkBlock(BasicBlock* block); - void impVerifyEHBlock(BasicBlock* block, bool isTryStart); + void impVerifyEHBlock(BasicBlock* block); void impImportBlockPending(BasicBlock* block); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 75078fc57f8d6f..e266b99890c7ba 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -3726,10 +3726,6 @@ void Compiler::fgFindBasicBlocks() } } - /* Mark the initial block and last blocks in the 'try' region */ - - tryBegBB->bbFlags |= BBF_TRY_BEG; - /* Prevent future optimizations of removing the first block */ /* of a TRY block and the first block of an exception handler */ @@ -4599,9 +4595,8 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr) newBlock->bbFlags = curr->bbFlags; // Remove flags that the new block can't have. - newBlock->bbFlags &= - ~(BBF_TRY_BEG | BBF_LOOP_HEAD | BBF_LOOP_CALL0 | BBF_LOOP_CALL1 | BBF_FUNCLET_BEG | BBF_LOOP_PREHEADER | - BBF_KEEP_BBJ_ALWAYS | BBF_PATCHPOINT | BBF_BACKWARD_JUMP_TARGET | BBF_LOOP_ALIGN); + newBlock->bbFlags &= ~(BBF_LOOP_HEAD | BBF_LOOP_CALL0 | BBF_LOOP_CALL1 | BBF_FUNCLET_BEG | BBF_LOOP_PREHEADER | + BBF_KEEP_BBJ_ALWAYS | BBF_PATCHPOINT | BBF_BACKWARD_JUMP_TARGET | BBF_LOOP_ALIGN); // Remove the GC safe bit on the new block. It seems clear that if we split 'curr' at the end, // such that all the code is left in 'curr', and 'newBlock' just gets the control flow, then diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 60aad57559d3e6..b8d12b3e648ad5 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -907,13 +907,15 @@ bool Compiler::fgDumpFlowGraph(Phases phase, PhasePosition pos) if (displayBlockFlags) { // Don't display the `[` `]` unless we're going to display something. - const BasicBlockFlags allDisplayedBlockFlags = BBF_TRY_BEG | BBF_FUNCLET_BEG | BBF_RUN_RARELY | - BBF_LOOP_HEAD | BBF_LOOP_PREHEADER | BBF_LOOP_ALIGN; - if (block->bbFlags & allDisplayedBlockFlags) + const bool isTryEntryBlock = bbIsTryBeg(block); + const BasicBlockFlags allDisplayedBlockFlags = + BBF_FUNCLET_BEG | BBF_RUN_RARELY | BBF_LOOP_HEAD | BBF_LOOP_PREHEADER | BBF_LOOP_ALIGN; + + if (isTryEntryBlock || ((block->bbFlags & allDisplayedBlockFlags) != 0)) { // Display a very few, useful, block flags fprintf(fgxFile, " ["); - if (block->bbFlags & BBF_TRY_BEG) + if (isTryEntryBlock) { fprintf(fgxFile, "T"); } @@ -2171,7 +2173,7 @@ void Compiler::fgTableDispBasicBlock(BasicBlock* block, int ibcColWidth /* = 0 * /* brace matching editor workaround to compensate for the preceding line: } */ } - if (flags & BBF_TRY_BEG) + if (bbIsTryBeg(block)) { // Output a brace for every try region that this block opens diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 8fd44c5e6f89c0..40fe7a69871593 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -224,12 +224,6 @@ PhaseStatus Compiler::fgRemoveEmptyFinally() } } - if (block == firstTryBlock) - { - assert((block->bbFlags & BBF_TRY_BEG) != 0); - block->bbFlags &= ~BBF_TRY_BEG; - } - if (block == lastTryBlock) { break; @@ -500,12 +494,6 @@ PhaseStatus Compiler::fgRemoveEmptyTry() } } - if (block == firstTryBlock) - { - assert((block->bbFlags & BBF_TRY_BEG) != 0); - block->bbFlags &= ~BBF_TRY_BEG; - } - if (block == lastTryBlock) { break; @@ -2101,7 +2089,7 @@ PhaseStatus Compiler::fgTailMergeThrows() // Workaround: don't consider try entry blocks as candidates // for merging; if the canonical throw is later in the same try, // we'll create invalid flow. - if ((block->bbFlags & BBF_TRY_BEG) != 0) + if (bbIsTryBeg(block)) { continue; } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index bb9cb076a801ba..5d72ba736d0072 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1671,7 +1671,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() fgSetTryBeg(HBtab, newTryEntry); // Try entry blocks get specially marked and have special protection. - HBtab->ebdTryBeg->bbFlags |= BBF_DONT_REMOVE | BBF_TRY_BEG; + HBtab->ebdTryBeg->bbFlags |= BBF_DONT_REMOVE; // We are keeping this try region removeTryRegion = false; @@ -2040,8 +2040,8 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext) // Make sure the second block is not the start of a TRY block or an exception handler + noway_assert(!bbIsTryBeg(bNext)); noway_assert(bNext->bbCatchTyp == BBCT_NONE); - noway_assert((bNext->bbFlags & BBF_TRY_BEG) == 0); noway_assert((bNext->bbFlags & BBF_DONT_REMOVE) == 0); /* both or none must have an exception handler */ @@ -6400,10 +6400,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) goto REPEAT; } - /* Remove unreachable or empty blocks - do not consider blocks marked BBF_DONT_REMOVE or genReturnBB block - * These include first and last block of a TRY, exception handlers and RANGE_CHECK_FAIL THROW blocks */ - - if ((block->bbFlags & BBF_DONT_REMOVE) == BBF_DONT_REMOVE || block == genReturnBB) + // Remove unreachable or empty blocks - do not consider blocks marked BBF_DONT_REMOVE + // These include first and last block of a TRY, exception handlers and THROW blocks. + if ((block->bbFlags & BBF_DONT_REMOVE) != 0) { bPrev = block; continue; @@ -6421,8 +6420,8 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) } #endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - noway_assert(!block->bbCatchTyp); - noway_assert(!(block->bbFlags & BBF_TRY_BEG)); + assert(!bbIsTryBeg(block)); + noway_assert(block->bbCatchTyp == BBCT_NONE); /* Remove unreachable blocks * diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 3f7a622716024a..9ebe73546fdd54 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -1617,7 +1617,7 @@ void Compiler::fgAddSyncMethodEnterExit() // EH regions in fgFindBasicBlocks(). Note that the try has no enclosing // handler, and the fault has no enclosing try. - tryBegBB->bbFlags |= BBF_DONT_REMOVE | BBF_TRY_BEG | BBF_IMPORTED; + tryBegBB->bbFlags |= BBF_DONT_REMOVE | BBF_IMPORTED; faultBB->bbFlags |= BBF_DONT_REMOVE | BBF_IMPORTED; faultBB->bbCatchTyp = BBCT_FAULT; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index d2ac329403a884..a34c7f828438d2 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11043,13 +11043,7 @@ inline void Compiler::impReimportMarkBlock(BasicBlock* block) block->bbFlags &= ~BBF_IMPORTED; } -/***************************************************************************** - * - * Filter wrapper to handle only passed in exception code - * from it). - */ - -void Compiler::impVerifyEHBlock(BasicBlock* block, bool isTryStart) +void Compiler::impVerifyEHBlock(BasicBlock* block) { assert(block->hasTryIndex()); assert(!compIsForInlining()); @@ -11057,16 +11051,9 @@ void Compiler::impVerifyEHBlock(BasicBlock* block, bool isTryStart) unsigned tryIndex = block->getTryIndex(); EHblkDsc* HBtab = ehGetDsc(tryIndex); - if (isTryStart) + if (bbIsTryBeg(block) && (block->bbStkDepth != 0)) { - assert(block->bbFlags & BBF_TRY_BEG); - - // The Stack must be empty - // - if (block->bbStkDepth != 0) - { - BADCODE("Evaluation stack must be empty on entry into a try block"); - } + BADCODE("Evaluation stack must be empty on entry into a try block"); } // Save the stack contents, we'll need to restore it later @@ -11121,12 +11108,6 @@ void Compiler::impVerifyEHBlock(BasicBlock* block, bool isTryStart) // Process the filter block, if we haven't already done so. if (HBtab->HasFilter()) { - /* @VERIFICATION : Ideally the end of filter state should get - propagated to the catch handler, this is an incompleteness, - but is not a security/compliance issue, since the only - interesting state is the 'thisInit' state. - */ - BasicBlock* filterBB = HBtab->ebdFilter; if (((filterBB->bbFlags & BBF_IMPORTED) == 0) && (impGetPendingBlockMember(filterBB) == 0)) @@ -11203,51 +11184,14 @@ void Compiler::impImportBlock(BasicBlock* block) /* Set the current stack state to the merged result */ verResetCurrentState(block, &verCurrentState); - /* Now walk the code and import the IL into GenTrees */ - + if (block->hasTryIndex()) { - /* @VERIFICATION : For now, the only state propagation from try - to it's handler is "thisInit" state (stack is empty at start of try). - In general, for state that we track in verification, we need to - model the possibility that an exception might happen at any IL - instruction, so we really need to merge all states that obtain - between IL instructions in a try block into the start states of - all handlers. - - However we do not allow the 'this' pointer to be uninitialized when - entering most kinds try regions (only try/fault are allowed to have - an uninitialized this pointer on entry to the try) - - Fortunately, the stack is thrown away when an exception - leads to a handler, so we don't have to worry about that. - We DO, however, have to worry about the "thisInit" state. - But only for the try/fault case. - - The only allowed transition is from TIS_Uninit to TIS_Init. - - So for a try/fault region for the fault handler block - we will merge the start state of the try begin - and the post-state of each block that is part of this try region - */ - - // merge the start state of the try begin - // - if (block->bbFlags & BBF_TRY_BEG) - { - impVerifyEHBlock(block, true); - } - - impImportBlockCode(block); - - // As discussed above: - // merge the post-state of each block that is part of this try region - // - if (block->hasTryIndex()) - { - impVerifyEHBlock(block, false); - } + impVerifyEHBlock(block); } + // Now walk the code and import the IL into GenTrees. + impImportBlockCode(block); + if (compDonotInline()) { return; diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 66169d8eb380b1..6cb2b8b6232979 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -2214,7 +2214,7 @@ bool Compiler::fgNormalizeEHCase2() // Note that we don't need to clear any flags on the old try start, since it is still a 'try' // start. - newTryStart->bbFlags |= (BBF_TRY_BEG | BBF_DONT_REMOVE | BBF_INTERNAL); + newTryStart->bbFlags |= (BBF_DONT_REMOVE | BBF_INTERNAL); if (insertBeforeBlk->bbFlags & BBF_BACKWARD_JUMP_TARGET) { @@ -3007,7 +3007,6 @@ void Compiler::fgVerifyHandlerTab() assert(HBtab->ebdHndBeg != nullptr); assert(HBtab->ebdHndLast != nullptr); - assert(HBtab->ebdTryBeg->bbFlags & BBF_TRY_BEG); assert(HBtab->ebdTryBeg->bbFlags & BBF_DONT_REMOVE); assert(HBtab->ebdHndBeg->bbFlags & BBF_DONT_REMOVE); @@ -3499,12 +3498,6 @@ void Compiler::fgVerifyHandlerTab() #endif // FEATURE_EH_FUNCLETS } - // Only the first block of 'try' regions should have BBF_TRY_BEG set. - if (!blockTryBegSet[block->bbNum]) - { - assert((block->bbFlags & BBF_TRY_BEG) == 0); - } - // Check for legal block types switch (block->GetJumpKind()) { @@ -4339,12 +4332,18 @@ bool Compiler::fgRelocateEHRegions() #endif // !FEATURE_EH_FUNCLETS -/***************************************************************************** - * We've inserted a new block before 'block' that should be part of the same EH region as 'block'. - * Update the EH table to make this so. Also, set the new block to have the right EH region data - * (copy the bbTryIndex, bbHndIndex, and bbCatchTyp from 'block' to the new predecessor, and clear - * 'bbCatchTyp' from 'block'). - */ +//------------------------------------------------------------------------ +// fgExtendEHRegionBefore: Modify the EH table to account for a new block. +// +// We've inserted a new block before 'block' that should be part of the same +// EH region as 'block'. Update the EH table to make this so. Also, set the +// new block to have the right EH region data (copy the bbTryIndex, bbHndIndex, +// and bbCatchTyp from 'block' to the new predecessor, and clear 'bbCatchTyp' +// from 'block'). +// +// Arguments: +// block - The block before which a new block has been inserted +// void Compiler::fgExtendEHRegionBefore(BasicBlock* block) { assert(!block->IsFirst()); @@ -4369,13 +4368,7 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block) } #endif // DEBUG HBtab->ebdTryBeg = bPrev; - bPrev->bbFlags |= BBF_TRY_BEG | BBF_DONT_REMOVE; - - // clear the TryBeg flag unless it begins another try region - if (!bbIsTryBeg(block)) - { - block->bbFlags &= ~BBF_TRY_BEG; - } + bPrev->bbFlags |= BBF_DONT_REMOVE; } if (HBtab->ebdHndBeg == block) @@ -4387,12 +4380,13 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block) } #endif // DEBUG + HBtab->ebdHndBeg = bPrev; + bPrev->bbFlags |= BBF_DONT_REMOVE; + // The first block of a handler has an artificial extra refcount. Transfer that to the new block. noway_assert(block->countOfInEdges() > 0); block->bbRefs--; - - HBtab->ebdHndBeg = bPrev; - bPrev->bbFlags |= BBF_DONT_REMOVE; + bPrev->bbRefs++; #if defined(FEATURE_EH_FUNCLETS) if (fgFuncletsCreated) @@ -4403,8 +4397,6 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block) } #endif // FEATURE_EH_FUNCLETS - bPrev->bbRefs++; - // If this is a handler for a filter, the last block of the filter will end with // a BBJ_EHFILTERRET block that has a bbJumpDest that jumps to the first block of // its handler. So we need to update it to keep things in sync. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index a7125734a718d8..1d4c4609c9dfcf 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -7989,11 +7989,11 @@ void Compiler::fgSetEHRegionForNewLoopHead(BasicBlock* newHead, BasicBlock* top) assert(newHead->NextIs(top)); assert(!fgIsFirstBlockOfFilterOrHandler(top)); - if ((top->bbFlags & BBF_TRY_BEG) != 0) + if (bbIsTryBeg(top)) { // `top` is the beginning of a try block. Figure out the EH region to use. assert(top->hasTryIndex()); - unsigned short newTryIndex = (unsigned short)ehTrueEnclosingTryIndexIL(top->getTryIndex()); + unsigned newTryIndex = ehTrueEnclosingTryIndexIL(top->getTryIndex()); if (newTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) { // No EH try index. @@ -8107,7 +8107,7 @@ bool Compiler::fgCreateLoopPreHeader(unsigned lnum) // Does this existing region have the same EH region index that we will use when we create the pre-header? // If not, we want to create a new pre-header with the expected region. bool headHasCorrectEHRegion = false; - if ((top->bbFlags & BBF_TRY_BEG) != 0) + if (bbIsTryBeg(top)) { assert(top->hasTryIndex()); unsigned newTryIndex = ehTrueEnclosingTryIndexIL(top->getTryIndex()); From afef031c2a82b6648eac3ffc06f6270673e34467 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 13 Oct 2023 00:59:26 +0300 Subject: [PATCH 2/2] Delete blockTryBegSet --- src/coreclr/jit/jiteh.cpp | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 6cb2b8b6232979..a94e4585c511eb 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -3074,19 +3074,16 @@ void Compiler::fgVerifyHandlerTab() #endif // To verify that bbCatchTyp is set properly on all blocks, and that some BBF_* flags are only set on the first - // block of 'try' or handlers, create two bool arrays indexed by block number: one for the set of blocks that - // are the beginning blocks of 'try' regions, and one for blocks that are the beginning of handlers (including - // filters). Note that since this checking function runs before EH normalization, we have to handle the case - // where blocks can be both the beginning of a 'try' as well as the beginning of a handler. After we've iterated - // over the EH table, loop over all blocks and verify that only handler begin blocks have bbCatchTyp == BBCT_NONE, - // and some other things. + // block of handlers, create a bool arrays indexed by block number for blocks that are the beginning of handlers + // (including filters). Note that since this checking function runs before EH normalization, we have to handle + // the case where blocks can be both the beginning of a 'try' as well as the beginning of a handler. After we've + // iterated over the EH table, loop over all blocks and verify that only handler begin blocks have + // bbCatchTyp != BBCT_NONE, and some other things. size_t blockBoolSetBytes = (bbNumMax + 1) * sizeof(bool); - bool* blockTryBegSet = (bool*)_alloca(blockBoolSetBytes); bool* blockHndBegSet = (bool*)_alloca(blockBoolSetBytes); for (unsigned i = 0; i <= bbNumMax; i++) { - blockTryBegSet[i] = false; blockHndBegSet[i] = false; } @@ -3365,12 +3362,7 @@ void Compiler::fgVerifyHandlerTab() } } - // Set up blockTryBegSet and blockHndBegSet. - // We might want to have this assert: - // if (fgNormalizeEHDone) assert(!blockTryBegSet[HBtab->ebdTryBeg->bbNum]); - // But we can't, because if we have mutually-protect 'try' regions, we'll see exactly the same tryBeg twice - // (or more). - blockTryBegSet[HBtab->ebdTryBeg->bbNum] = true; + // Set up blockHndBegSet. assert(!blockHndBegSet[HBtab->ebdHndBeg->bbNum]); blockHndBegSet[HBtab->ebdHndBeg->bbNum] = true;