From 60fab43c7368f291bd0ea23f67ca4ea5017b1ba3 Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Mon, 12 Oct 2026 12:17:55 -0700 Subject: [PATCH 1/6] JIT: keep the block a catch returns to non-empty when optimizations are disabled When a catch ends with a thread abort pending, the runtime raises the abort again from the address the catch returns to and looks the handlers up for that address. fgOptimizeEmptyBlock keeps the block a catch returns to from being empty (it adds a GT_NO_OP) when the block is in a different EH region than its successor, so that the address stays inside the block's regions. That only runs when optimizing. In MinOpts and debuggable code the block falls into its successor without generating code, the address is the end of its regions, and the abort is dispatched as if it had been raised in the code that follows: a finally the catch's leave passes through does not run, and the handlers that see the abort differ from optimized code. Move the check into a helper and run it from fgPostImportationCleanup when optimizations are disabled. Optimized code is unchanged. Fixes #135637 --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/fgopt.cpp | 154 ++++++++++------ .../coreclr/GitHub_135637/test135637.cs | 172 ++++++++++++++++++ .../coreclr/GitHub_135637/test135637.csproj | 16 ++ 4 files changed, 289 insertions(+), 55 deletions(-) create mode 100644 src/tests/Regressions/coreclr/GitHub_135637/test135637.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 95d461d4a44536..a3c268f4966e72 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6950,6 +6950,8 @@ class Compiler bool fgBlockIsGoodTailDuplicationCandidate(BasicBlock* block, unsigned* lclNum); + bool fgKeepCatchRetTargetNonEmpty(BasicBlock* block); + bool fgOptimizeEmptyBlock(BasicBlock* block); bool fgLeadsToEmptyBlockCycle(BasicBlock* block); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index dc5ca4132df32c..e0d943bf43c6d8 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -363,12 +363,30 @@ PhaseStatus Compiler::fgPostImportationCleanup() } } + // The flow graph optimizations keep the block a catch returns to from + // being empty when it is in a different EH region than its successor + // (see fgOptimizeEmptyBlock). They do not run when optimizations are + // disabled, so do that part here. + // + bool keptCatchRetTargets = false; + + if (opts.OptimizationDisabled() && (compHndBBtabCount > 0)) + { + for (BasicBlock* const block : Blocks()) + { + if (block->KindIs(BBJ_ALWAYS) && block->isEmpty() && fgKeepCatchRetTargetNonEmpty(block)) + { + keptCatchRetTargets = true; + } + } + } + // If no blocks were removed, we're done. // Unless we are an OSR method with a try entry. // if ((removedBlks == 0) && !(opts.IsOSR() && fgOSREntryBB->hasTryIndex())) { - return PhaseStatus::MODIFIED_NOTHING; + return keptCatchRetTargets ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } // Update all references in the exception handler table. @@ -747,7 +765,8 @@ PhaseStatus Compiler::fgPostImportationCleanup() // Did we make any changes? // - const bool madeChanges = (addedBlocks > 0) || (delCnt > 0) || (removedBlks > 0) || addedTemps; + const bool madeChanges = + (addedBlocks > 0) || (delCnt > 0) || (removedBlks > 0) || addedTemps || keptCatchRetTargets; // Note that we have now run post importation cleanup, // so we can enable more stringent checking. @@ -1425,6 +1444,81 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc return false; } +//------------------------------------------------------------- +// fgKeepCatchRetTargetNonEmpty: +// Add a GT_NO_OP to an empty block that a catch returns to, if the block +// is in a different EH region than its successor. +// +// Arguments: +// block - an empty BBJ_ALWAYS block +// +// Returns: true if a GT_NO_OP was added to the block +// +// Notes: +// When a catch ends with a thread abort pending, the runtime raises the +// abort again from the address the catch returns to, and looks the handlers +// up for that address. So that address must be inside the EH regions of +// the block the catch returns to (see also impImportLeave). An empty block +// that falls into its successor generates no code, and the address would +// then belong to the successor. The GT_NO_OP makes codegen emit an +// instruction for the block. +// +bool Compiler::fgKeepCatchRetTargetNonEmpty(BasicBlock* block) +{ + assert(block->KindIs(BBJ_ALWAYS)); + assert(block->isEmpty()); + + BasicBlock* const succBlock = block->GetTarget(); + + if ((succBlock == nullptr) || BasicBlock::sameEHRegion(block, succBlock)) + { + return false; + } + + // The empty block and the block that follows it are in different + // EH regions. Does a catch return to the empty block? + + bool isCatchRetTarget = false; + for (BasicBlock* const predBlock : block->PredBlocks()) + { + if (predBlock->KindIs(BBJ_EHCATCHRET)) + { + assert(predBlock->TargetIs(block)); + isCatchRetTarget = true; + break; + } + } + + if (!isCatchRetTarget) + { + return false; + } + + // Insert a NOP in the empty block to ensure we generate code + // for the catchret target in the right EH region. + GenTree* nop = new (this, GT_NO_OP) GenTree(GT_NO_OP, TYP_VOID); + + if (block->IsLIR()) + { + LIR::AsRange(block).InsertAtEnd(nop); + LIR::ReadOnlyRange range(nop, nop); + m_pLowering->LowerRange(block, range); + } + else + { + Statement* nopStmt = fgNewStmtAtEnd(block, nop); + if (fgNodeThreading == NodeThreading::AllTrees) + { + fgSetStmtSeq(nopStmt); + } + gtSetStmtInfo(nopStmt); + } + + JITDUMP("\nKeeping empty block " FMT_BB " - it is the target of a catch return\n", block->bbNum); + + return true; +} + //------------------------------------------------------------- // fgOptimizeEmptyBlock: // Does flow optimization of an empty block (can remove it in some cases) @@ -1506,60 +1600,10 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block) // catch be in the correct EH region, for re-raise of thread // abort exceptions to work. Insert a NOP in the empty block // to ensure we generate code for the block, if we keep it. + if (fgKeepCatchRetTargetNonEmpty(block)) { - BasicBlock* succBlock = block->GetTarget(); - - if ((succBlock != nullptr) && !BasicBlock::sameEHRegion(block, succBlock)) - { - // The empty block and the block that follows it are in different - // EH regions. Is this a case where they can't be merged? - - bool okToMerge = true; // assume it's ok - for (BasicBlock* const predBlock : block->PredBlocks()) - { - if (predBlock->KindIs(BBJ_EHCATCHRET)) - { - assert(predBlock->TargetIs(block)); - okToMerge = false; // we can't get rid of the empty block - break; - } - } - - if (!okToMerge) - { - // Insert a NOP in the empty block to ensure we generate code - // for the catchret target in the right EH region. - GenTree* nop = new (this, GT_NO_OP) GenTree(GT_NO_OP, TYP_VOID); - - if (block->IsLIR()) - { - LIR::AsRange(block).InsertAtEnd(nop); - LIR::ReadOnlyRange range(nop, nop); - m_pLowering->LowerRange(block, range); - } - else - { - Statement* nopStmt = fgNewStmtAtEnd(block, nop); - if (fgNodeThreading == NodeThreading::AllTrees) - { - fgSetStmtSeq(nopStmt); - } - gtSetStmtInfo(nopStmt); - } - - madeChanges = true; - -#ifdef DEBUG - if (verbose) - { - printf("\nKeeping empty block " FMT_BB " - it is the target of a catch return\n", - block->bbNum); - } -#endif // DEBUG - - break; // go to the next block - } - } + madeChanges = true; + break; // go to the next block } if (!ehCanDeleteEmptyBlock(block)) diff --git a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs new file mode 100644 index 00000000000000..1f41bd0ac6d6ec --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs @@ -0,0 +1,172 @@ +// 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 System.Runtime; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using TestLibrary; +using Xunit; + +// ControlledExecution.Run is the API that aborts a thread +#pragma warning disable SYSLIB0046 + +// A thread abort that is raised again after a catch block has swallowed it is dispatched from the address the catch +// returns to. The block the catch returns to has to generate code of its own when it ends an enclosing EH region, +// otherwise that address belongs to the code that follows it and the abort reaches different handlers. +public class Runtime_135637 +{ + private static volatile bool s_readyForCancellation; + private static volatile int s_counter; + private static string s_trace; + // Set by call-free stores, so that what is observed does not depend on a call after a catch getting to run + // while the abort is pending. + private static volatile bool s_finallyRan; + private static volatile bool s_newTryRan; + private static volatile bool s_reachedAfterCatch; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void RunInfiniteLoop() + { + s_readyForCancellation = true; + while (true) + { + if ((++s_counter & 0xfffff) == 0) + { + Thread.Sleep(0); + } + } + } + + // The catch's leave passes through a finally before it reaches the code after the try/finally + [MethodImpl(MethodImplOptions.NoInlining)] + private static void CatchThenFinally() + { + try + { + try + { + RunInfiniteLoop(); + } + catch (ThreadAbortException) + { + s_trace += "catch "; + } + finally + { + s_finallyRan = true; + s_trace += "finally "; + } + } + catch (ThreadAbortException) + { + s_trace += "outer "; + } + + s_reachedAfterCatch = true; + } + + // The catch's leave target is outside the enclosing try + [MethodImpl(MethodImplOptions.NoInlining)] + private static void LeaveOutOfOuterTry() + { + try + { + try + { + RunInfiniteLoop(); + } + catch (ThreadAbortException) + { + s_trace += "inner "; + goto Done; + } + } + catch (ThreadAbortException) + { + s_trace += "outer "; + } + + Done: + s_reachedAfterCatch = true; + } + + // The code after the catch is the start of another try + [MethodImpl(MethodImplOptions.NoInlining)] + private static void ConsecutiveTry() + { + try + { + RunInfiniteLoop(); + } + catch (ThreadAbortException) + { + s_trace += "inner "; + } + + try + { + s_newTryRan = true; + s_trace += "new try "; + } + catch (ThreadAbortException) + { + s_trace += "new catch "; + } + + s_reachedAfterCatch = true; + } + + private static void CancelWhenReady(CancellationTokenSource cts) + { + while (!s_readyForCancellation) + { + Thread.Sleep(10); + } + + cts.Cancel(); + } + + // Runs the action, aborts it once it is ready and checks which of its blocks ran + private static void RunAndAbort(string name, Action action, string expectedTrace, bool expectedFinallyRan) + { + Console.WriteLine(name); + s_trace = ""; + s_readyForCancellation = false; + s_finallyRan = false; + s_newTryRan = false; + s_reachedAfterCatch = false; + + var cts = new CancellationTokenSource(); + Task.Run(() => CancelWhenReady(cts)); + + Exception exception = null; + try + { + ControlledExecution.Run(action, cts.Token); + } + catch (Exception e) + { + exception = e; + } + + Assert.IsType(exception); + Assert.Equal(expectedTrace, s_trace); + Assert.Equal(expectedFinallyRan, s_finallyRan); + Assert.False(s_newTryRan); + Assert.False(s_reachedAfterCatch); + } + + // The handlers that see the abort raised again after a catch follow from the code the JIT lays out for the + // catch's return. Under the interpreter the outer catch of LeaveOutOfOuterTry does not run. + public static bool IsTestCodeCompiled => PlatformDetection.IsMultithreadingSupported && !Utilities.IsCoreClrInterpreter; + + [ConditionalFact(typeof(Runtime_135637), nameof(IsTestCodeCompiled))] + public static void TestEntryPoint() + { + RunAndAbort(nameof(CatchThenFinally), CatchThenFinally, "catch finally outer ", expectedFinallyRan: true); + RunAndAbort(nameof(LeaveOutOfOuterTry), LeaveOutOfOuterTry, "inner outer ", expectedFinallyRan: false); + RunAndAbort(nameof(ConsecutiveTry), ConsecutiveTry, "inner ", expectedFinallyRan: false); + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj b/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj new file mode 100644 index 00000000000000..ebde94d263e518 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj @@ -0,0 +1,16 @@ + + + 1 + + true + + true + true + + + + + + + + From 43f529dc9aca768cf00275acdf6ff5e04c16b9ee Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Thu, 15 Oct 2026 09:02:43 -0700 Subject: [PATCH 2/6] JIT: check in codegen that the block a catch returns to emits code Whether the block a catch returns to generates an instruction is only known in codegen. The block can be empty, as in unoptimized code, or in optimized code when it is the first block of a try region (fgUpdateFlowGraph skips those), or it can contain only nodes that generate no code, such as GC.KeepAlive of an argument, which fgOptimizeEmptyBlock does not see as an empty block. Either way the address the catch returns to then belongs to the code that follows, and a thread abort raised again at that address reaches the wrong handlers. Do the check in genEmitEndBlock: if a BBJ_ALWAYS block that a catch returns to has emitted nothing, emit a nop when its jump to the next block is omitted, and otherwise do not mark the jump as a candidate for emitRemoveJumpToNextInst, which on xarch could remove it and leave the block empty again. Do not depend on the EH region of the next block: when that block emits nothing either, the address belongs to a later one. Remove the GT_NO_OP that fgPostImportationCleanup added when optimizations are disabled, and the one fgOptimizeEmptyBlock adds: it still keeps such a block, and codegen now emits the instruction, without a nop in front of a jump that the block keeps. --- src/coreclr/jit/codegen.h | 1 + src/coreclr/jit/codegenlinear.cpp | 55 ++++++++++++- src/coreclr/jit/compiler.h | 2 - src/coreclr/jit/fgopt.cpp | 130 +++++++----------------------- 4 files changed, 83 insertions(+), 105 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 41e96fd2e808a7..fdf2ef58420054 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -237,6 +237,7 @@ class CodeGen final : public CodeGenInterface void genEmitStartBlock(BasicBlock* block); void genEmitEndBlock(BasicBlock* block); + bool genIsCatchRetTargetWithoutCode(BasicBlock* block); public: #if HAS_FIXED_REGISTER_SET diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 8688cf3209314c..c96c8ddee908e5 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -893,12 +893,20 @@ void CodeGen::genEmitEndBlock(BasicBlock* block) } #endif // TARGET_AMD64 + // A block a catch returns to must emit at least one instruction. + if (genIsCatchRetTargetWithoutCode(block)) + { + instGen(INS_nop); + } + removedJmp = true; break; } #ifdef TARGET_XARCH - // Do not remove a jump between hot and cold regions. - bool isRemovableJmpCandidate = !m_compiler->fgInDifferentRegions(block, block->GetTarget()); + // Do not remove a jump between hot and cold regions, or a jump that is the only + // instruction of a block a catch returns to. + bool isRemovableJmpCandidate = + !m_compiler->fgInDifferentRegions(block, block->GetTarget()) && !genIsCatchRetTargetWithoutCode(block); inst_JMP(EJ_jmp, block->GetTarget(), isRemovableJmpCandidate); #else // !TARGET_XARCH @@ -969,6 +977,49 @@ void CodeGen::genEmitEndBlock(BasicBlock* block) #endif // FEATURE_LOOP_ALIGN } +//------------------------------------------------------------------------ +// genIsCatchRetTargetWithoutCode: check whether a catch returns to a block +// that has not emitted any code so far +// +// Arguments: +// block - the block whose code is being generated +// +// Return Value: +// true if 'block' is the target of a catch return and nothing has been +// emitted since its label +// +// Notes: +// When a catch returns with a thread abort pending, the runtime raises the +// abort again at the address the catch returns to and finds the handlers +// for it by that address. That address must be inside the EH regions of +// the block the catch returns to (see impImportLeave), so the block must +// emit at least one instruction of its own: if it emits none, the address +// belongs to the code that follows it. Whether the block emits anything is +// only known here: it can be empty in the IR, or have nodes that generate +// no code. +// +bool CodeGen::genIsCatchRetTargetWithoutCode(BasicBlock* block) +{ + // The target of a catch return always has a label, so nothing has been + // emitted for the block if the emitter is still at the start of the + // label's instruction group. + if ((block->bbEmitCookie != GetEmitter()->emitCurIG) || GetEmitter()->emitCurIGnonEmpty()) + { + return false; + } + + for (BasicBlock* const predBlock : block->PredBlocks()) + { + if (predBlock->KindIs(BBJ_EHCATCHRET)) + { + assert(block->HasFlag(BBF_HAS_LABEL)); + return true; + } + } + + return false; +} + // TODO-WASM-Factoring: this ifdef factoring is temporary. The end factoring should look like this: // 1. Everything shared by all codegen backends (incl. WASM) is moved to codegencommon.cpp. // 2. Everything else stays here. diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a3c268f4966e72..95d461d4a44536 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6950,8 +6950,6 @@ class Compiler bool fgBlockIsGoodTailDuplicationCandidate(BasicBlock* block, unsigned* lclNum); - bool fgKeepCatchRetTargetNonEmpty(BasicBlock* block); - bool fgOptimizeEmptyBlock(BasicBlock* block); bool fgLeadsToEmptyBlockCycle(BasicBlock* block); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e0d943bf43c6d8..32bca83af9eb48 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -363,30 +363,12 @@ PhaseStatus Compiler::fgPostImportationCleanup() } } - // The flow graph optimizations keep the block a catch returns to from - // being empty when it is in a different EH region than its successor - // (see fgOptimizeEmptyBlock). They do not run when optimizations are - // disabled, so do that part here. - // - bool keptCatchRetTargets = false; - - if (opts.OptimizationDisabled() && (compHndBBtabCount > 0)) - { - for (BasicBlock* const block : Blocks()) - { - if (block->KindIs(BBJ_ALWAYS) && block->isEmpty() && fgKeepCatchRetTargetNonEmpty(block)) - { - keptCatchRetTargets = true; - } - } - } - // If no blocks were removed, we're done. // Unless we are an OSR method with a try entry. // if ((removedBlks == 0) && !(opts.IsOSR() && fgOSREntryBB->hasTryIndex())) { - return keptCatchRetTargets ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; + return PhaseStatus::MODIFIED_NOTHING; } // Update all references in the exception handler table. @@ -765,8 +747,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() // Did we make any changes? // - const bool madeChanges = - (addedBlocks > 0) || (delCnt > 0) || (removedBlks > 0) || addedTemps || keptCatchRetTargets; + const bool madeChanges = (addedBlocks > 0) || (delCnt > 0) || (removedBlks > 0) || addedTemps; // Note that we have now run post importation cleanup, // so we can enable more stringent checking. @@ -1444,81 +1425,6 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc return false; } -//------------------------------------------------------------- -// fgKeepCatchRetTargetNonEmpty: -// Add a GT_NO_OP to an empty block that a catch returns to, if the block -// is in a different EH region than its successor. -// -// Arguments: -// block - an empty BBJ_ALWAYS block -// -// Returns: true if a GT_NO_OP was added to the block -// -// Notes: -// When a catch ends with a thread abort pending, the runtime raises the -// abort again from the address the catch returns to, and looks the handlers -// up for that address. So that address must be inside the EH regions of -// the block the catch returns to (see also impImportLeave). An empty block -// that falls into its successor generates no code, and the address would -// then belong to the successor. The GT_NO_OP makes codegen emit an -// instruction for the block. -// -bool Compiler::fgKeepCatchRetTargetNonEmpty(BasicBlock* block) -{ - assert(block->KindIs(BBJ_ALWAYS)); - assert(block->isEmpty()); - - BasicBlock* const succBlock = block->GetTarget(); - - if ((succBlock == nullptr) || BasicBlock::sameEHRegion(block, succBlock)) - { - return false; - } - - // The empty block and the block that follows it are in different - // EH regions. Does a catch return to the empty block? - - bool isCatchRetTarget = false; - for (BasicBlock* const predBlock : block->PredBlocks()) - { - if (predBlock->KindIs(BBJ_EHCATCHRET)) - { - assert(predBlock->TargetIs(block)); - isCatchRetTarget = true; - break; - } - } - - if (!isCatchRetTarget) - { - return false; - } - - // Insert a NOP in the empty block to ensure we generate code - // for the catchret target in the right EH region. - GenTree* nop = new (this, GT_NO_OP) GenTree(GT_NO_OP, TYP_VOID); - - if (block->IsLIR()) - { - LIR::AsRange(block).InsertAtEnd(nop); - LIR::ReadOnlyRange range(nop, nop); - m_pLowering->LowerRange(block, range); - } - else - { - Statement* nopStmt = fgNewStmtAtEnd(block, nop); - if (fgNodeThreading == NodeThreading::AllTrees) - { - fgSetStmtSeq(nopStmt); - } - gtSetStmtInfo(nopStmt); - } - - JITDUMP("\nKeeping empty block " FMT_BB " - it is the target of a catch return\n", block->bbNum); - - return true; -} - //------------------------------------------------------------- // fgOptimizeEmptyBlock: // Does flow optimization of an empty block (can remove it in some cases) @@ -1598,12 +1504,34 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block) // from its successor block, if the block is the target of a // catch return. It is required that the return address of a // catch be in the correct EH region, for re-raise of thread - // abort exceptions to work. Insert a NOP in the empty block - // to ensure we generate code for the block, if we keep it. - if (fgKeepCatchRetTargetNonEmpty(block)) + // abort exceptions to work. Codegen makes sure that the block + // generates an instruction (see genIsCatchRetTargetWithoutCode). { - madeChanges = true; - break; // go to the next block + BasicBlock* succBlock = block->GetTarget(); + + if ((succBlock != nullptr) && !BasicBlock::sameEHRegion(block, succBlock)) + { + // The empty block and the block that follows it are in different + // EH regions. Is this a case where they can't be merged? + + bool okToMerge = true; // assume it's ok + for (BasicBlock* const predBlock : block->PredBlocks()) + { + if (predBlock->KindIs(BBJ_EHCATCHRET)) + { + assert(predBlock->TargetIs(block)); + okToMerge = false; // we can't get rid of the empty block + break; + } + } + + if (!okToMerge) + { + JITDUMP("\nKeeping empty block " FMT_BB " - it is the target of a catch return\n", + block->bbNum); + break; // go to the next block + } + } } if (!ehCanDeleteEmptyBlock(block)) From 56c457157eb90777afa68de199408f5d1456277e Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Thu, 15 Oct 2026 09:02:43 -0700 Subject: [PATCH 3/6] Add optimized-code shapes to the test for #135637 In optimized code the block a catch returns to can still generate no code: - KeepAliveAfterCatch and KeepAliveThenFinally: the block only calls GC.KeepAlive on an argument. The outer catch did not see the abort raised again after the inner catch, and the finally did not run. The inner try ends with a store rather than with the call, because a nop can follow a call at the end of a try (on x64 always, on arm64 for GC info) and then hides the problem. - CatchInLoop: the catch's try is the body of a loop. The catch ends up returning to the loop's head, an empty block followed by the try, so the abort was raised again inside the try and the same catch ran again. The methods are marked AggressiveOptimization so the default test run covers optimized code. --- .../coreclr/GitHub_135637/test135637.cs | 101 +++++++++++++++++- 1 file changed, 100 insertions(+), 1 deletion(-) diff --git a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs index 1f41bd0ac6d6ec..5ebc8a4e2b26c3 100644 --- a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs +++ b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs @@ -14,7 +14,9 @@ // A thread abort that is raised again after a catch block has swallowed it is dispatched from the address the catch // returns to. The block the catch returns to has to generate code of its own when it ends an enclosing EH region, -// otherwise that address belongs to the code that follows it and the abort reaches different handlers. +// otherwise that address belongs to the code that follows it and the abort reaches different handlers. That block can +// be empty, as in unoptimized code or at the head of a loop, or contain only nodes that generate no code, as +// GC.KeepAlive of an argument does in optimized code. public class Runtime_135637 { private static volatile bool s_readyForCancellation; @@ -118,6 +120,100 @@ private static void ConsecutiveTry() s_reachedAfterCatch = true; } + // The catch returns to a block in the enclosing try that only keeps an argument alive. The store after the call + // keeps the try from ending with a call, after which the JIT may emit a nop that would hide the problem. + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static void KeepAliveAfterCatch(object o) + { + try + { + try + { + RunInfiniteLoop(); + s_counter = 0; + } + catch (ThreadAbortException) + { + s_trace += "inner "; + } + + GC.KeepAlive(o); + } + catch (ThreadAbortException) + { + s_trace += "outer "; + } + + s_reachedAfterCatch = true; + } + + // Same, with a finally that the code after the catch leaves + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static void KeepAliveThenFinally(object o) + { + try + { + try + { + try + { + RunInfiniteLoop(); + s_counter = 0; + } + catch (ThreadAbortException) + { + s_trace += "catch "; + } + + GC.KeepAlive(o); + } + finally + { + s_finallyRan = true; + s_trace += "finally "; + } + } + catch (ThreadAbortException) + { + s_trace += "outer "; + } + + s_reachedAfterCatch = true; + } + + // The catch returns to the head of a loop whose body is the catch's try. The abort raised again at the end of the + // catch must reach the outer catch; if it is raised inside the try, the same catch runs again, so stop after a few + // times. + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static void CatchInLoop() + { + int count = 0; + try + { + while (true) + { + try + { + RunInfiniteLoop(); + } + catch (ThreadAbortException) + { + s_trace += "catch "; + if (++count == 3) + { + break; + } + } + } + } + catch (ThreadAbortException) + { + s_trace += "outer "; + } + + s_reachedAfterCatch = true; + } + private static void CancelWhenReady(CancellationTokenSource cts) { while (!s_readyForCancellation) @@ -168,5 +264,8 @@ public static void TestEntryPoint() RunAndAbort(nameof(CatchThenFinally), CatchThenFinally, "catch finally outer ", expectedFinallyRan: true); RunAndAbort(nameof(LeaveOutOfOuterTry), LeaveOutOfOuterTry, "inner outer ", expectedFinallyRan: false); RunAndAbort(nameof(ConsecutiveTry), ConsecutiveTry, "inner ", expectedFinallyRan: false); + RunAndAbort(nameof(KeepAliveAfterCatch), () => KeepAliveAfterCatch(new object()), "inner outer ", expectedFinallyRan: false); + RunAndAbort(nameof(KeepAliveThenFinally), () => KeepAliveThenFinally(new object()), "catch finally outer ", expectedFinallyRan: true); + RunAndAbort(nameof(CatchInLoop), CatchInLoop, "catch outer ", expectedFinallyRan: false); } } From 9b93eabda701d14f9b71e48b964f24504b7d3f99 Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Thu, 15 Oct 2026 10:24:34 -0700 Subject: [PATCH 4/6] Track the interpreter exclusion in the test for #135637 with an issue Under the interpreter the outer catch does not see the abort raised again after the inner catch in LeaveOutOfOuterTry and CatchInLoop. That is #135871, so skip the test there with an ActiveIssue instead of folding the interpreter into the test's own condition. --- src/tests/Regressions/coreclr/GitHub_135637/test135637.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs index 5ebc8a4e2b26c3..be914beb571801 100644 --- a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs +++ b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs @@ -254,11 +254,8 @@ private static void RunAndAbort(string name, Action action, string expectedTrace Assert.False(s_reachedAfterCatch); } - // The handlers that see the abort raised again after a catch follow from the code the JIT lays out for the - // catch's return. Under the interpreter the outer catch of LeaveOutOfOuterTry does not run. - public static bool IsTestCodeCompiled => PlatformDetection.IsMultithreadingSupported && !Utilities.IsCoreClrInterpreter; - - [ConditionalFact(typeof(Runtime_135637), nameof(IsTestCodeCompiled))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/135871", typeof(TestLibrary.Utilities), nameof(TestLibrary.Utilities.IsCoreClrInterpreter))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] public static void TestEntryPoint() { RunAndAbort(nameof(CatchThenFinally), CatchThenFinally, "catch finally outer ", expectedFinallyRan: true); From 6dc56e4ad3ca428e87fa864429fa59c0e8feea52 Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Thu, 15 Oct 2026 11:21:55 -0700 Subject: [PATCH 5/6] Run the test for #135637 in the merged test runner Skip it on runtimes other than CoreCLR with ConditionalFact instead of CLRTestTargetUnsupported, which needed RequiresProcessIsolation. --- src/tests/Regressions/coreclr/GitHub_135637/test135637.cs | 2 +- src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs index be914beb571801..196575e9a6316c 100644 --- a/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs +++ b/src/tests/Regressions/coreclr/GitHub_135637/test135637.cs @@ -255,7 +255,7 @@ private static void RunAndAbort(string name, Action action, string expectedTrace } [ActiveIssue("https://github.com/dotnet/runtime/issues/135871", typeof(TestLibrary.Utilities), nameof(TestLibrary.Utilities.IsCoreClrInterpreter))] - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsCoreCLR), nameof(PlatformDetection.IsMultithreadingSupported))] public static void TestEntryPoint() { RunAndAbort(nameof(CatchThenFinally), CatchThenFinally, "catch finally outer ", expectedFinallyRan: true); diff --git a/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj b/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj index ebde94d263e518..6e9bed1068cbc2 100644 --- a/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj +++ b/src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj @@ -1,10 +1,6 @@ 1 - - true - - true true From 98d74aed439df5cf5c1c152c74b135fde6bbe7e1 Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Thu, 15 Oct 2026 16:09:03 -0700 Subject: [PATCH 6/6] JIT: keep the GT_NO_OP in an empty block a catch returns to fgOptimizeEmptyBlock keeps an empty block that a catch returns to when the block is in a different EH region than its successor, and adds a GT_NO_OP to it. An earlier commit here removed that node, because codegen now makes such a block emit an instruction. But the node also keeps the block non-empty in the IR, and later flow optimizations depend on that: fgOptimizeBranchToEmptyUnconditional redirects the branches to an empty BBJ_ALWAYS block to its target. On Wasm, fgWasmEhFlow adds, for each catch continuation, a resume pad that branches to it. With the continuation empty, the fgUpdateFlowGraph call in fgLateLiveness, after lowering, redirected the pad past it, so the continuation was reachable only from the catch return, which fgWasmControlFlow does not support ("NYI_WASM: Method has blocks only reachable via EH"). Crossgen of System.Private.CoreLib for browser-wasm failed that way, in IndentedTextWriter.WriteAsync and OutputTabsAsync. Keep the GT_NO_OP, as before. Codegen still covers the catch-return targets that fgOptimizeEmptyBlock does not handle. --- src/coreclr/jit/fgopt.cpp | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 32bca83af9eb48..dc5ca4132df32c 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1504,8 +1504,8 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block) // from its successor block, if the block is the target of a // catch return. It is required that the return address of a // catch be in the correct EH region, for re-raise of thread - // abort exceptions to work. Codegen makes sure that the block - // generates an instruction (see genIsCatchRetTargetWithoutCode). + // abort exceptions to work. Insert a NOP in the empty block + // to ensure we generate code for the block, if we keep it. { BasicBlock* succBlock = block->GetTarget(); @@ -1527,8 +1527,36 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block) if (!okToMerge) { - JITDUMP("\nKeeping empty block " FMT_BB " - it is the target of a catch return\n", - block->bbNum); + // Insert a NOP in the empty block to ensure we generate code + // for the catchret target in the right EH region. + GenTree* nop = new (this, GT_NO_OP) GenTree(GT_NO_OP, TYP_VOID); + + if (block->IsLIR()) + { + LIR::AsRange(block).InsertAtEnd(nop); + LIR::ReadOnlyRange range(nop, nop); + m_pLowering->LowerRange(block, range); + } + else + { + Statement* nopStmt = fgNewStmtAtEnd(block, nop); + if (fgNodeThreading == NodeThreading::AllTrees) + { + fgSetStmtSeq(nopStmt); + } + gtSetStmtInfo(nopStmt); + } + + madeChanges = true; + +#ifdef DEBUG + if (verbose) + { + printf("\nKeeping empty block " FMT_BB " - it is the target of a catch return\n", + block->bbNum); + } +#endif // DEBUG + break; // go to the next block } }