JIT: fix filter liveness computation (#23044)
authorAndy Ayers <andya@microsoft.com>
Thu, 7 Mar 2019 18:52:19 +0000 (10:52 -0800)
committerGitHub <noreply@github.com>
Thu, 7 Mar 2019 18:52:19 +0000 (10:52 -0800)
When a filter is finished executing, control can logically pass to the
associated handler, any enclosing handler or filter, or any finally or fault
handler nested within the associated try. This is a consequence of two-pass EH.

The jit was not propagating liveness from the nested handlers, which lead to a
live object being collected inadvertently.

This change updates `fgGetHandlerLiveVars` to find the nested handlers and
merge their live-in into the filter block live sets.

Because these implicit EH flow edges can create cycles in the liveness dataflow
equations, the jit will also now always iterate liveness when it sees there is
exception flow, to ensure livness reaches the appropriate fixed point.

Added test case.

Closes #22820.

src/jit/liveness.cpp
tests/src/JIT/Regression/JitBlue/GitHub_22330/GitHub_22330.cs
tests/src/JIT/Regression/JitBlue/GitHub_22820/GitHub_22820.cs [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_22820/GitHub_22820.csproj [new file with mode: 0644]

index ded6c05..c9d5452 100644 (file)
@@ -1056,6 +1056,42 @@ void Compiler::fgExtendDbgLifetimes()
 #endif // DEBUG
 }
 
+//------------------------------------------------------------------------
+// fgGetHandlerLiveVars: 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.
+//
+// Notes:
+//    Assumes caller has screened candidate blocks to only those with
+//    exception flow, via `ehBlockHasExnFlowDsc`.
+//
+//    Exception flow can arise because of a newly raised exception (for
+//    blocks within try regions) or because of an actively propagating exception
+//    (for filter blocks). This flow effectively creates additional successor
+//    edges in the flow graph that the jit does not model. This method computes
+//    the net contribution from all the missing successor edges.
+//
+//    For example, with the following C# source, during EH processing of the throw,
+//    the outer filter will execute in pass1, before the inner handler executes
+//    in pass2, and so the filter blocks should show the inner handler's local is live.
+//
+//    try
+//    {
+//        using (AllocateObject())   // ==> try-finally; handler calls Dispose
+//        {
+//            throw new Exception();
+//        }
+//    }
+//    catch (Exception e1) when (IsExpectedException(e1))
+//    {
+//        Console.WriteLine("In catch 1");
+//    }
+
 VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block)
 {
     noway_assert(block);
@@ -1067,7 +1103,6 @@ VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block)
     do
     {
         /* Either we enter the filter first or the catch/finally */
-
         if (HBtab->HasFilter())
         {
             VarSetOps::UnionD(this, liveVars, HBtab->ebdFilter->bbLiveIn);
@@ -1099,6 +1134,72 @@ VARSET_VALRET_TP Compiler::fgGetHandlerLiveVars(BasicBlock* block)
 
     } 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)
+            {
+                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);
+                }
+
+                // 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 (enclosedHBtab->HasFinallyOrFaultHandler())
+                    {
+                        VarSetOps::UnionD(this, liveVars, enclosedHBtab->ebdHndBeg->bbLiveIn);
+                    }
+                }
+                // Once we run across a non-enclosed region, we can stop searching.
+                else
+                {
+                    break;
+                }
+            }
+        }
+    }
+
     return liveVars;
 }
 
@@ -1171,14 +1272,17 @@ class LiveVarAnalysis
         // since (without proof otherwise) the use and def may touch different memory at run-time.
         m_memoryLiveIn = m_memoryLiveOut | block->bbMemoryUse;
 
-        /* Can exceptions from this block be handled (in this function)? */
-
+        // 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))
         {
             const VARSET_TP& liveVars(m_compiler->fgGetHandlerLiveVars(block));
-
             VarSetOps::UnionD(m_compiler, m_liveIn, liveVars);
             VarSetOps::UnionD(m_compiler, m_liveOut, liveVars);
+
+            // Implicit eh edges can induce loop-like behavior,
+            // so make sure we iterate to closure.
+            m_hasPossibleBackEdge = true;
         }
 
         /* Has there been any change in either live set? */
@@ -2399,7 +2503,6 @@ void Compiler::fgInterBlockLocalVarLiveness()
         if (block->bbCatchTyp != BBCT_NONE)
         {
             /* Note the set of variables live on entry to exception handler */
-
             VarSetOps::UnionD(this, exceptVars, block->bbLiveIn);
         }
 
index 59adfba..b0ed8e2 100644 (file)
@@ -1,3 +1,7 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
 using System;
 using System.Runtime.CompilerServices;
 
@@ -46,4 +50,4 @@ class X
         Console.WriteLine($"Result={result}");
         return result;
     }
-}
\ No newline at end of file
+}
diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_22820/GitHub_22820.cs b/tests/src/JIT/Regression/JitBlue/GitHub_22820/GitHub_22820.cs
new file mode 100644 (file)
index 0000000..95121ab
--- /dev/null
@@ -0,0 +1,60 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+
+// Repro for issue 22820. On x86 we need to report enclosed handler
+// live-in locals as live into any enclosing filter.
+//
+// Run with optimized codegen and COMPlus_GCStress=0x4
+
+class DisposableObject : IDisposable
+{
+    public void Dispose()
+    {
+        Console.WriteLine("In dispose");
+    }
+}
+
+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();
+    }
+    
+    static int Main(string[] args)
+    {
+        int result = 0;
+
+        try
+        {
+            try
+            {
+                using (AllocateObject())
+                {
+                    throw new Exception();
+                }
+            }
+            catch (Exception e1) when (IsExpectedException(e1))
+            {
+                Console.WriteLine("In catch 1");
+            }
+        }
+        catch (Exception e2)
+        {
+            Console.WriteLine("In catch 2");
+            result = 100;
+        }
+
+        return result;
+    }
+}
diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_22820/GitHub_22820.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_22820/GitHub_22820.csproj
new file mode 100644 (file)
index 0000000..42f8a01
--- /dev/null
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{2649FAFE-07BF-4F93-8120-BA9A69285ABB}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>
\ No newline at end of file