Fix fgNewBBinRegion when inserting into filters.

In order to ensure that the GC operates properly when handling
exceptions, the CLR ABI requires that control exits a filter region
throguh its terminal instruction. `fgNewBBinRegion` was violating this
invariant, however, which lead to GC stress failures when the GC was
invoked immediately after a filter finished executing but before control
entered any handlers. This change fixes the behavior of
`fgNewBBinRegion` when inserting new blocks into filter regions.

There are two cases to consider when inserting a new BB into a filter
region:
1. The filter region consists of multiple blocks
2. The filter region consists of a single block

The first case is straightforward: instead of inserting a new BB after
the last block of the filter, insert it before the last block of the
filter. Because the filter contains multiple blocks, the predecessor of
the filter must also be in the filter region.

The latter case requires splitting the single block, as inserting a new
block before the single block would otherwise change control flow (the
filter would begin with the new block rather than the single block). In
order to acheive this with minimal complexity, this change first inserts
a `BBJ_ALWAYS` block that transfers control to the existing single
block, then inserts a second new block immediately before the existing
single block. The second new block is then returned. To put it
visually, the transformation is from this:

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

to this:

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

and the function returns `BBN + 2`.

Fixes VSO 406163.

diff --git a/Documentation/botr/clr-abi.md b/Documentation/botr/clr-abi.md
index 8a226ed..a85bfa4 100644 (file)
--- a/Documentation/botr/clr-abi.md
+++ b/Documentation/botr/clr-abi.md
@@ -470,7 +470,7 @@ When the inner "throw new UserException4" is executed, the exception handling fi
 
 ## Filter GC semantics
 
-Filters are invoked in the 1st pass of EH processing and as such execution might resume back at the faulting address, or in the filter-handler, or someplace else. Because the VM must allow GC's to occur during and after a filter invocation, but before the EH subsystem knows where it will resume, we need to keep everything alive at both the faulting address **and** within the filter. This is accomplished by 3 means: (1) the VM's stackwalker and GCInfoDecoder report as live both the filter frame and its corresponding parent frame, (2) the JIT encodes all stack slots that are live within the filter as being pinned, and (3) the JIT reports as live (and possible zero-initializes) anything live-out of the filter. Because of (1) it is likely that a stack variable that is live within the filter and the try body will be double reported. During the mark phase of the GC double reporting is not a problem. The problem only arises if the object is relocated: if the same location is reported twice, the GC will try to relocate the address stored at that location twice. Thus we prevent the object from being relocated by pinning it, which leads us to why we must do (2). (3) is done so that after the filter returns, we can still safely incur a GC before executing the filter-handler or any outer handler within the same frame.
+Filters are invoked in the 1st pass of EH processing and as such execution might resume back at the faulting address, or in the filter-handler, or someplace else. Because the VM must allow GC's to occur during and after a filter invocation, but before the EH subsystem knows where it will resume, we need to keep everything alive at both the faulting address **and** within the filter. This is accomplished by 3 means: (1) the VM's stackwalker and GCInfoDecoder report as live both the filter frame and its corresponding parent frame, (2) the JIT encodes all stack slots that are live within the filter as being pinned, and (3) the JIT reports as live (and possible zero-initializes) anything live-out of the filter. Because of (1) it is likely that a stack variable that is live within the filter and the try body will be double reported. During the mark phase of the GC double reporting is not a problem. The problem only arises if the object is relocated: if the same location is reported twice, the GC will try to relocate the address stored at that location twice. Thus we prevent the object from being relocated by pinning it, which leads us to why we must do (2). (3) is done so that after the filter returns, we can still safely incur a GC before executing the filter-handler or any outer handler within the same frame. For the same reason, control must exit a filter region via its final block (in other words, a filter region must terminate with the instruction that leaves the filter region, and the program may not exit the filter region via other paths).
 
 ## Duplicated Clauses
 

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index c385155..21132f3 100644 (file)
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -3189,9 +3189,15 @@ void Compiler::fgComputePreds()
         if (ehDsc->HasFilter())
         {
             ehDsc->ebdFilter->bbFlags |= BBF_JMP_TARGET | BBF_HAS_LABEL;
+
+            // The first block of a filter has an artifical extra refcount.
+            ehDsc->ebdFilter->bbRefs++;
         }
 
         ehDsc->ebdHndBeg->bbFlags |= BBF_JMP_TARGET | BBF_HAS_LABEL;
+
+        // The first block of a handler has an artificial extra refcount.
+        ehDsc->ebdHndBeg->bbRefs++;
     }
 
     fgModified         = false;
@@ -12081,12 +12087,6 @@ void Compiler::fgInsertFuncletPrologBlock(BasicBlock* block)
     fgExtendEHRegionBefore(block);    // Update the EH table to make the prolog block the first block in the block's EH
                                       // block.
 
-    // fgExtendEHRegionBefore mucks with the bbRefs without updating the pred list, which we will
-    // do below for this block. So, undo that change.
-    assert(newHead->bbRefs > 0);
-    newHead->bbRefs--;
-    block->bbRefs++;
-
     // Distribute the pred list between newHead and block. Incoming edges coming from outside
     // the handler go to the prolog. Edges coming from with the handler are back-edges, and
     // go to the existing 'block'.
@@ -16569,6 +16569,7 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block)
 #endif // DEBUG
             HBtab->ebdTryBeg = bPrev;
             bPrev->bbFlags |= BBF_TRY_BEG | BBF_DONT_REMOVE | BBF_HAS_LABEL;
+
             // clear the TryBeg flag unless it begins another try region
             if (!bbIsTryBeg(block))
             {
@@ -16591,6 +16592,16 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block)
 
             HBtab->ebdHndBeg = bPrev;
             bPrev->bbFlags |= BBF_DONT_REMOVE | BBF_HAS_LABEL;
+
+#if FEATURE_EH_FUNCLETS
+            if (fgFuncletsCreated)
+            {
+                assert((block->bbFlags & BBF_FUNCLET_BEG) != 0);
+                bPrev->bbFlags |= BBF_FUNCLET_BEG;
+                block->bbFlags &= ~BBF_FUNCLET_BEG;
+            }
+#endif // FEATURE_EH_FUNCLETS
+
             bPrev->bbRefs++;
 
             // If this is a handler for a filter, the last block of the filter will end with
@@ -16630,6 +16641,16 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block)
 
             HBtab->ebdFilter = bPrev;
             bPrev->bbFlags |= BBF_DONT_REMOVE | BBF_HAS_LABEL;
+
+#if FEATURE_EH_FUNCLETS
+            if (fgFuncletsCreated)
+            {
+                assert((block->bbFlags & BBF_FUNCLET_BEG) != 0);
+                bPrev->bbFlags |= BBF_FUNCLET_BEG;
+                block->bbFlags &= ~BBF_FUNCLET_BEG;
+            }
+#endif // FEATURE_EH_FUNCLETS
+
             bPrev->bbRefs++;
         }
     }
@@ -17036,8 +17057,8 @@ bool Compiler::fgCheckEHCanInsertAfterBlock(BasicBlock* blk, unsigned regionInde
 //
 // Return Value:
 //    A block with the desired characteristics, so the new block will be inserted after this one.
-//    If there is no suitable location, return nullptr. This should basically never happen.
-
+//    If there is no suitable location, return nullptr. This should basically never happen except in the case of
+//    single-block filters.
 BasicBlock* Compiler::fgFindInsertPoint(unsigned    regionIndex,
                                         bool        putInTryRegion,
                                         BasicBlock* startBlk,
@@ -17069,6 +17090,13 @@ BasicBlock* Compiler::fgFindInsertPoint(unsigned    regionIndex,
             regionIndex, dspBool(putInTryRegion), startBlk->bbNum, (endBlk == nullptr) ? 0 : endBlk->bbNum,
             (nearBlk == nullptr) ? 0 : nearBlk->bbNum, (jumpBlk == nullptr) ? 0 : jumpBlk->bbNum, dspBool(runRarely));
 
+    bool insertingIntoFilter = false;
+    if (!putInTryRegion)
+    {
+        EHblkDsc* const dsc = ehGetDsc(regionIndex - 1);
+        insertingIntoFilter = dsc->HasFilter() && (startBlk == dsc->ebdFilter) && (endBlk == dsc->ebdHndBeg);
+    }
+
     bool        reachedNear = false; // Have we reached 'nearBlk' in our search? If not, we'll keep searching.
     bool        inFilter    = false; // Are we in a filter region that we need to skip?
     BasicBlock* bestBlk =
@@ -17110,9 +17138,7 @@ BasicBlock* Compiler::fgFindInsertPoint(unsigned    regionIndex,
         {
             // Record the fact that we entered a filter region, so we don't insert into filters...
             // Unless the caller actually wanted the block inserted in this exact filter region.
-            // Detect this by the fact that startBlk and endBlk point to the filter begin and end.
-            if (putInTryRegion || (blk != startBlk) || (startBlk != ehGetDsc(regionIndex - 1)->ebdFilter) ||
-                (endBlk != ehGetDsc(regionIndex - 1)->ebdHndBeg))
+            if (!insertingIntoFilter || (blk != startBlk))
             {
                 inFilter = true;
             }
@@ -17258,7 +17284,21 @@ BasicBlock* Compiler::fgFindInsertPoint(unsigned    regionIndex,
         bestBlk = goodBlk;
     }
 
-DONE:;
+DONE:
+
+    // 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))
+    {
+        assert(bestBlk != nullptr);
+        assert(bestBlk->bbJumpKind == BBJ_EHFILTERRET);
+        bestBlk = nullptr;
+    }
 
     return bestBlk;
 }
@@ -17437,6 +17477,21 @@ 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'. */
@@ -20508,7 +20563,28 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef
         }
 
         /* Check the bbRefs */
-        noway_assert(!checkBBRefs || block->bbRefs == blockRefs);
+        if (checkBBRefs)
+        {
+            if (block->bbRefs != blockRefs)
+            {
+                // Check to see if this block is the beginning of a filter or a handler and adjust the ref count
+                // appropriately.
+                for (EHblkDsc *HBtab = compHndBBtab, *HBtabEnd = &compHndBBtab[compHndBBtabCount]; HBtab != HBtabEnd;
+                     HBtab++)
+                {
+                    if (HBtab->ebdHndBeg == block)
+                    {
+                        blockRefs++;
+                    }
+                    if (HBtab->HasFilter() && (HBtab->ebdFilter == block))
+                    {
+                        blockRefs++;
+                    }
+                }
+            }
+
+            assert(block->bbRefs == blockRefs);
+        }
 
         /* Check that BBF_HAS_HANDLER is valid bbTryIndex */
         if (block->hasTryIndex())