JIT: keep the block a catch returns to non-empty when optimizations are disabled

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);

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))

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<OperationCanceledException>(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);
+    }
+}

src/tests/Regressions/coreclr/GitHub_135637/test135637.csproj

@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <CLRTestPriority>1</CLRTestPriority>
+    <!-- Needed for CLRTestTargetUnsupported -->
+    <RequiresProcessIsolation>true</RequiresProcessIsolation>
+    <!-- ControlledExecution is supported on CoreCLR only -->
+    <CLRTestTargetUnsupported Condition="'$(RuntimeFlavor)' != 'coreclr'">true</CLRTestTargetUnsupported>
+    <NativeAotIncompatible>true</NativeAotIncompatible>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="test135637.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <ProjectReference Include="$(TestLibraryProjectPath)" />
+  </ItemGroup>
+</Project>