Fix fgNewBBinRegion when inserting into filters. #11134

pgavlin · 2017-04-21T17:32:44Z

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:

The filter region consists of multiple blocks
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.

CarolEidt

I don't understand the extra refcounts, which may be just a commenting issue, but otherwise seems OK.
Would really like to see @BruceForstall review this.

CarolEidt · 2017-04-21T21:56:05Z

src/jit/flowgraph.cpp

+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 that the terminal block of a filter region is


Should be "the CLR ABI states that"?

Yes, good catch.

CarolEidt · 2017-04-21T21:58:22Z

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


Why are these extra refcounts needed?

They are expected by fgExtendEHRegionBefore.

To elaborate a bit, these blocks start out with an extra ref count. fgComputePreds only increments the ref count of a BB for formal incoming edges, which these blocks do not have. The relevant code in fgExtendEHRegionBefore is here and here.

Thanks - so I gather that this is not something that is new, but was previously omitted. I was trying to figure out how that was a new requirement.

I gather that this is not something that is new, but was previously omitted

Yes, that's right.

BruceForstall · 2017-04-24T18:13:01Z

src/jit/flowgraph.cpp

+#if FEATURE_EH_FUNCLETS
+            bPrev->bbFlags |= BBF_FUNCLET_BEG;
+#endif // FEATURE_EH_FUNCLETS
+


This doesn't look right. 'try' is not necessarily the beginning of a funclet

Yeah, I have a fix for this locally.

BruceForstall · 2017-04-24T18:15:21Z

Why are you addressing the case "2. The filter region consists of a single block"? Does anyone use that? I thought we discussed that wasn't necessary. In particular, if there is a single BBJ_EHFILTERRET and a new block is being
created, then the caller must have split the block already, since they need to generate a BBJ_COND or BBJ_ALWAYS to branch to the new block.

pgavlin · 2017-04-24T21:06:36Z

Why are you addressing the case "2. The filter region consists of a single block"? Does anyone use that?

Yes: fgAddCodeRef is called during fgSimpleLowering and may need to insert throw blocks into single-block filter regions.

BruceForstall · 2017-04-24T21:42:30Z

Please run desktop SPMI diffs

BruceForstall · 2017-04-24T21:54:20Z

src/jit/flowgraph.cpp

+            }
+#endif // FEATURE_EH_FUNCLETS
+
+


Seems like this isn't necessary, because if block is the beginning of a funclet, then it will also be the first block of a handler, and thus the handler check below will do the right thing. Plus, if you do this, I would think the handler check assert below would fail because you would have already cleared the bit.

BruceForstall · 2017-04-24T21:57:44Z

src/jit/flowgraph.cpp

+    // insert after its predecessor instead: the CLR ABI 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 s.t. the inserted block is marked as


s.t. [](start = 67, length = 4)

Can you please spell out "s.t."?

BruceForstall · 2017-04-24T22:33:27Z

src/jit/flowgraph.cpp

@@ -17069,6 +17098,9 @@ 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));

+    const bool insertingIntoFilter = !putInTryRegion && (startBlk == ehGetDsc(regionIndex - 1)->ebdFilter) &&
+                                     (endBlk == ehGetDsc(regionIndex - 1)->ebdHndBeg);
+


I assume you hoisted this because it looked loop-insensitive. However, you've introduced a bug where we're accessing ebdFilter even if the EH region does not represent a filter, which is illegal (since it is a union; we really should assert if we could). Below, we can do this because we know at that point blk == startBlk and blk is the first block of a filter, and the function requires the caller to pass exactly a filter region as startBlk/endBlk in this case. You could also use !putInTryRegion && ehGetDsc(regionIndex - 1)->HasFilter() && ... (but now the number of duplicated ehGetDsc() is getting ridiculous)

Could this be rewritten as const bool insertingIntoFilter = !putInTryRegion && (startBlk->bbCatchType == BBCT_FILTER) && (endBlk == ehGetDsc(regionIndex - 1)->ebdHndBeg);?

It seems like we should be checking to ensure the startBlk and endBlk are in the same EH table entry (even though the caller should presumably guarantee it)

In reply to: 113073328 [](ancestors = 113073328)

Okay. How about

bool insertingIntoFilter = false; if (!putInTryRegion) { EHblkDsc* const dsc = ehGetDsc(regionIndex - 1); insertingIntoFilter = dsc->HasFilter() && (startBlk == dsc->ebdFilter) && (endBlk == dsc->ebdHndBeg); }

That works.

BruceForstall · 2017-04-24T22:35:59Z

src/jit/flowgraph.cpp

@@ -17437,6 +17481,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 exists a filter via the last instruction in the filter range, this situation requires logically


exists [](start = 20, length = 6)

exists => exits

BruceForstall · 2017-04-24T22:43:12Z

src/jit/flowgraph.cpp

+                blockRefs++;
+            }
+        }
+
        if (checkBBNum)


This looks expensive. Can you instead change the assert below from:

/* Check the bbRefs */ noway_assert(!checkBBRefs || block->bbRefs == blockRefs);

to:

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

Thus, it is only computed if required, on demand.

BruceForstall · 2017-04-24T23:01:16Z

Thanks for cleaning up the bbRefs inconsistencies!

pgavlin · 2017-04-25T17:06:41Z

@BruceForstall desktop SPMI reported no asm diffs. I have verified that there are diffs on x86 in the expected cases, however.

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.

Issue #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#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.

pgavlin added the area-CodeGen label Apr 21, 2017

pgavlin requested review from CarolEidt and BruceForstall April 21, 2017 17:32

dnfclas added the cla-already-signed label Apr 21, 2017

pgavlin force-pushed the VSO406163 branch from 052bdd1 to 6b5d898 Compare April 21, 2017 20:44

CarolEidt suggested changes Apr 21, 2017

View reviewed changes

CarolEidt approved these changes Apr 21, 2017

View reviewed changes

BruceForstall reviewed Apr 24, 2017

View reviewed changes

BruceForstall approved these changes Apr 24, 2017

View reviewed changes

pgavlin force-pushed the VSO406163 branch from 3e1b63c to c098785 Compare April 25, 2017 17:10

pgavlin merged commit 4a3a184 into dotnet:master Apr 25, 2017

pgavlin deleted the VSO406163 branch April 25, 2017 22:36

pgavlin mentioned this pull request Apr 29, 2017

Fix #11273 by adjusting the fix for VSO 406163. #11304

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fgNewBBinRegion when inserting into filters. #11134

Fix fgNewBBinRegion when inserting into filters. #11134

pgavlin commented Apr 21, 2017 •

edited

Loading

CarolEidt left a comment

CarolEidt Apr 21, 2017

pgavlin Apr 21, 2017

pgavlin Apr 24, 2017

CarolEidt Apr 21, 2017

pgavlin Apr 21, 2017

pgavlin Apr 21, 2017

CarolEidt Apr 21, 2017

pgavlin Apr 21, 2017 •

edited

Loading

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall commented Apr 24, 2017

pgavlin commented Apr 24, 2017

BruceForstall commented Apr 24, 2017

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall Apr 24, 2017

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall Apr 24, 2017

pgavlin Apr 24, 2017

BruceForstall commented Apr 24, 2017

pgavlin commented Apr 25, 2017

		}
		#endif // FEATURE_EH_FUNCLETS

Fix fgNewBBinRegion when inserting into filters. #11134

Fix fgNewBBinRegion when inserting into filters. #11134

Conversation

pgavlin commented Apr 21, 2017 • edited Loading

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgavlin Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall commented Apr 24, 2017

pgavlin commented Apr 24, 2017

BruceForstall commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall commented Apr 24, 2017

pgavlin commented Apr 25, 2017

pgavlin commented Apr 21, 2017 •

edited

Loading

pgavlin Apr 21, 2017 •

edited

Loading