JIT: make sure the block a catch returns to generates an instruction

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

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.

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

@@ -0,0 +1,268 @@
+// 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. 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;
+    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;
+    }
+
+    // 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)
+        {
+            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);
+    }
+
+    [ActiveIssue("https://github.com/dotnet/runtime/issues/135871", typeof(TestLibrary.Utilities), nameof(TestLibrary.Utilities.IsCoreClrInterpreter))]
+    [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsCoreCLR), nameof(PlatformDetection.IsMultithreadingSupported))]
+    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);
+    }
+}

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

@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="test135637.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <ProjectReference Include="$(TestLibraryProjectPath)" />
+  </ItemGroup>
+</Project>