Fix dotnet/coreclr#11273 by adjusting the fix for VSO 406163.

Issue dotnet/coreclr#11273 revealed an issue with the fix for VSO 406163: as of today,
the compiler models the incoming exception object in a handler by
looking for `GT_CATCH_ARG` in the first block of a handler and if found
marking the register that contains the exception object as holding a GC
pointer. Unfortunately, this breaks down under the transform that the
original fix (dotnet/coreclr#11134) was performing for single-block filters. That
transformation was from this:

```
    filter start: BBN (BBJ_EHFILTERRET)
```

to this:

```
    filter start: BBN + 1 (BBJ_ALWAYS -> BBN)
                  BBN + 2 (jumpKind)
                  BBN     (BBJ_EHFILTERRET)
```

After this transform, `BBN` still contains the `GT_CATCH_ARG`, but the
register containing the exception object must be live through `BBN + 1`
as well as live in to `BBN`.

There are a number of possible solutions to this problem (e.g. modeling
the liveness of the exception object in some fashion or splitting the
single-block filter either after the catch arg or before the filter
ret), but many of them are complicated by the fact that this
transformation needs to operate on both HIR and LIR and must be be able
to run in a variety of locations, including after registers have been
allocated. Instead, this change takes the approach of ensuring that no
single-block filter regions exist on platforms that target the JIT32 GC
encoder by inserting a spill of the exception object to a local var in
its own block at the start of a filter.

Commit migrated from https://github.com/dotnet/coreclr/commit/cf0e382bd7b235d6cae0204d3a11850d4d95df44

diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h
index 998b647..75cb4dc 100644 (file)
--- a/src/coreclr/src/jit/compiler.h
+++ b/src/coreclr/src/jit/compiler.h
@@ -3187,7 +3187,7 @@ private:
     static fgWalkPreFn impFindValueClasses;
     void impSpillLclRefs(ssize_t lclNum);
 
-    BasicBlock* impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_HANDLE clsHnd);
+    BasicBlock* impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_HANDLE clsHnd, bool isSingleBlockFilter);
 
     void impImportBlockCode(BasicBlock* block);
 

diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp
index 1582179..6b35cd0 100644 (file)
--- a/src/coreclr/src/jit/flowgraph.cpp
+++ b/src/coreclr/src/jit/flowgraph.cpp
@@ -17284,19 +17284,21 @@ BasicBlock* Compiler::fgFindInsertPoint(unsigned    regionIndex,
 
 DONE:
 
+#if defined(JIT32_GCENCODER)
     // If we are inserting into a filter and the best block is the end of the filter region, we need to
-    // insert after its predecessor instead: the CLR ABI states that the terminal block of a filter region
-    // is its exit block. If the filter region consists of a single block, a new block cannot be inserted
-    // without either splitting the single block before inserting a new block or inserting the new block
-    // before the single block and updating the filter description such that the inserted block is marked
-    // as the entry block for the filter. This work must be done by the caller; this function returns
-    // `nullptr` to indicate this case.
-    if (insertingIntoFilter && (bestBlk == endBlk->bbPrev) && (bestBlk == startBlk))
+    // insert after its predecessor instead: the JIT32 GC encoding used by the x86 CLR ABI  states that the
+    // terminal block of a filter region is its exit block. If the filter region consists of a single block,
+    // a new block cannot be inserted without either splitting the single block before inserting a new block
+    // or inserting the new block before the single block and updating the filter description such that the
+    // inserted block is marked as the entry block for the filter. Becuase this sort of split can be complex
+    // (especially given that it must ensure that the liveness of the exception object is properly tracked),
+    // we avoid this situation by never generating single-block filters on x86 (see impPushCatchArgOnStack).
+    if (insertingIntoFilter && (bestBlk == endBlk->bbPrev))
     {
-        assert(bestBlk != nullptr);
-        assert(bestBlk->bbJumpKind == BBJ_EHFILTERRET);
-        bestBlk = nullptr;
+        assert(bestBlk != startBlk);
+        bestBlk = bestBlk->bbPrev;
     }
+#endif // defined(JIT32_GCENCODER)
 
     return bestBlk;
 }
@@ -17475,21 +17477,6 @@ BasicBlock* Compiler::fgNewBBinRegion(BBjumpKinds jumpKind,
     // Now find the insertion point.
     afterBlk = fgFindInsertPoint(regionIndex, putInTryRegion, startBlk, endBlk, nearBlk, nullptr, runRarely);
 
-    // If afterBlk is nullptr, we must be inserting into a single-block filter region. Because the CLR ABI requires
-    // that control exits a filter via the last instruction in the filter range, this situation requires logically
-    // splitting the single block. In practice, we simply insert a new block at the beginning of the filter region
-    // that transfers control flow to the existing single block.
-    if (afterBlk == nullptr)
-    {
-        assert(putInFilter);
-
-        BasicBlock* newFilterEntryBlock = fgNewBBbefore(BBJ_ALWAYS, startBlk, true);
-        newFilterEntryBlock->bbJumpDest = startBlk;
-        fgAddRefPred(startBlk, newFilterEntryBlock);
-
-        afterBlk = newFilterEntryBlock;
-    }
-
 _FoundAfterBlk:;
 
     /* We have decided to insert the block after 'afterBlk'. */

diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp
index 0da280e..8be54e9 100644 (file)
--- a/src/coreclr/src/jit/importer.cpp
+++ b/src/coreclr/src/jit/importer.cpp
@@ -2402,7 +2402,7 @@ void Compiler::impSpillLclRefs(ssize_t lclNum)
  *  Returns the basic block of the actual handler.
  */
 
-BasicBlock* Compiler::impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_HANDLE clsHnd)
+BasicBlock* Compiler::impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_HANDLE clsHnd, bool isSingleBlockFilter)
 {
     // Do not inject the basic block twice on reimport. This should be
     // hit only under JIT stress. See if the block is the one we injected.
@@ -2440,8 +2440,14 @@ BasicBlock* Compiler::impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_H
      * moved around since it is tied to a fixed location (EAX) */
     arg->gtFlags |= GTF_ORDER_SIDEEFF;
 
+#if defined(JIT32_GCENCODER)
+    const bool forceInsertNewBlock = isSingleBlockFilter || compStressCompile(STRESS_CATCH_ARG, 5);
+#else
+    const bool forceInsertNewBlock = compStressCompile(STRESS_CATCH_ARG, 5);
+#endif // defined(JIT32_GCENCODER)
+
     /* Spill GT_CATCH_ARG to a temp if there are jumps to the beginning of the handler */
-    if (hndBlk->bbRefs > 1 || compStressCompile(STRESS_CATCH_ARG, 5))
+    if (hndBlk->bbRefs > 1 || forceInsertNewBlock)
     {
         if (hndBlk->bbRefs == 1)
         {
@@ -15620,7 +15626,7 @@ void Compiler::impVerifyEHBlock(BasicBlock* block, bool isTryStart)
 
                 // push catch arg the stack, spill to a temp if necessary
                 // Note: can update HBtab->ebdHndBeg!
-                hndBegBB = impPushCatchArgOnStack(hndBegBB, clsHnd);
+                hndBegBB = impPushCatchArgOnStack(hndBegBB, clsHnd, false);
             }
 
             // Queue up the handler for importing
@@ -15641,7 +15647,8 @@ void Compiler::impVerifyEHBlock(BasicBlock* block, bool isTryStart)
 
                 // push catch arg the stack, spill to a temp if necessary
                 // Note: can update HBtab->ebdFilter!
-                filterBB = impPushCatchArgOnStack(filterBB, impGetObjectClass());
+                const bool isSingleBlockFilter = (filterBB->bbNext == hndBegBB);
+                filterBB = impPushCatchArgOnStack(filterBB, impGetObjectClass(), isSingleBlockFilter);
 
                 impImportBlockPending(filterBB);
             }