Fix bug converting BBJ_CALLFINALLY block to BBJ_THROW on ARM32 (#13094)

* Fix bug converting BBJ_CALLFINALLY block to BBJ_THROW on ARM32

On ARM32, the target block of a return from a finally (aka, the
"continuation") is marked by a bit, `BBF_FINALLY_TARGET`, that is
used to generate proper code to ensure correct unwinding behavior.
This bit is set in the importer, and maintained through to codegen.
We assert if we attempt to delete a block with this bit. Thus, various
code that deletes dead code needs to call fgClearFinallyTargetBit()
to keep the bit updated.

This bug is a case where very early in morph, a BBJ_CALLFINALLY
block is converted to a BBJ_THROW. We should be able to just call
fgClearFinallyTargetBit(), but that function depends on the predecessor
lists, which in this case, haven't yet been built. So, instead, clear
the bits everywhere, and then recompute them at the end of morph.
This is similar to what is done with the various try/finally
optimizations done at the beginning of morph.

(It's an open question about whether we could avoid setting the bits
at all until just before codegen.)

Fixes VSO468731

No diffs in desktop altjit asm diffs.

* Formatting

diff --git a/src/jit/compiler.h b/src/jit/compiler.h
index 90c4502..8ea07fd 100644 (file)
--- a/src/jit/compiler.h
+++ b/src/jit/compiler.h
@@ -3652,6 +3652,16 @@ public:
 
     void fgUpdateFinallyTargetFlags();
 
+    void fgClearAllFinallyTargetBits();
+
+    void fgAddFinallyTargetFlags();
+
+#if FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+    // Sometimes we need to defer updating the BBF_FINALLY_TARGET bit. fgNeedToAddFinallyTargetBits signals
+    // when this is necessary.
+    bool fgNeedToAddFinallyTargetBits;
+#endif // FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+
     bool fgRetargetBranchesToCanonicalCallFinally(BasicBlock*      block,
                                                   BasicBlock*      handler,
                                                   BlockToBlockMap& continuationMap);

diff --git a/src/jit/compiler.hpp b/src/jit/compiler.hpp
index 127c56e..33fae0f 100644 (file)
--- a/src/jit/compiler.hpp
+++ b/src/jit/compiler.hpp
@@ -3020,6 +3020,35 @@ inline unsigned Compiler::fgThrowHlpBlkStkLevel(BasicBlock* block)
 */
 inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block)
 {
+    // If we're converting a BBJ_CALLFINALLY block to a BBJ_THROW block,
+    // then mark the subsequent BBJ_ALWAYS block as unreferenced.
+    if (block->isBBCallAlwaysPair())
+    {
+        BasicBlock* leaveBlk = block->bbNext;
+        noway_assert(leaveBlk->bbJumpKind == BBJ_ALWAYS);
+
+        leaveBlk->bbFlags &= ~BBF_DONT_REMOVE;
+        leaveBlk->bbRefs  = 0;
+        leaveBlk->bbPreds = nullptr;
+
+#if FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+        // This function (fgConvertBBToThrowBB) can be called before the predecessor lists are created (e.g., in
+        // fgMorph). The fgClearFinallyTargetBit() function to update the BBF_FINALLY_TARGET bit depends on these
+        // predecessor lists. If there are no predecessor lists, we immediately clear all BBF_FINALLY_TARGET bits
+        // (to allow subsequent dead code elimination to delete such blocks without asserts), and set a flag to
+        // recompute them later, before they are required.
+        if (fgComputePredsDone)
+        {
+            fgClearFinallyTargetBit(leaveBlk->bbJumpDest);
+        }
+        else
+        {
+            fgClearAllFinallyTargetBits();
+            fgNeedToAddFinallyTargetBits = true;
+        }
+#endif // FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+    }
+
     block->bbJumpKind = BBJ_THROW;
     block->bbSetRunRarely(); // any block with a throw is rare
 }

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 0c8a28e..0d8a1e0 100644 (file)
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -24405,13 +24405,49 @@ void Compiler::fgUpdateFinallyTargetFlags()
 
     JITDUMP("In fgUpdateFinallyTargetFlags, updating finally target flag bits\n");
 
+    fgClearAllFinallyTargetBits();
+    fgAddFinallyTargetFlags();
+
+#endif // FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+}
+
+//------------------------------------------------------------------------
+// fgClearAllFinallyTargetBits: Clear all BBF_FINALLY_TARGET bits; these will need to be
+// recomputed later.
+//
+void Compiler::fgClearAllFinallyTargetBits()
+{
+#if FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+
+    JITDUMP("*************** In fgClearAllFinallyTargetBits()\n");
+
+    // Note that we clear the flags even if there are no EH clauses (compHndBBtabCount == 0)
+    // in case bits are left over from EH clauses being deleted.
+
     // Walk all blocks, and reset the target bits.
     for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
     {
         block->bbFlags &= ~BBF_FINALLY_TARGET;
     }
 
-    // Walk all blocks again, and set the target bits.
+#endif // FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+}
+
+//------------------------------------------------------------------------
+// fgAddFinallyTargetFlags: Add BBF_FINALLY_TARGET bits to all finally targets.
+//
+void Compiler::fgAddFinallyTargetFlags()
+{
+#if FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+
+    JITDUMP("*************** In fgAddFinallyTargetFlags()\n");
+
+    if (compHndBBtabCount == 0)
+    {
+        JITDUMP("No EH in this method, no flags to set.\n");
+        return;
+    }
+
     for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
     {
         if (block->isBBCallAlwaysPair())

diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index 8cead02..ce1adc7 100644 (file)
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -17427,6 +17427,15 @@ void Compiler::fgMorph()
     DBEXEC(VERBOSE, fgDispBasicBlocks(true));
 #endif
 
+#if FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+    if (fgNeedToAddFinallyTargetBits)
+    {
+        // We previously wiped out the BBF_FINALLY_TARGET bits due to some morphing; add them back.
+        fgAddFinallyTargetFlags();
+        fgNeedToAddFinallyTargetBits = false;
+    }
+#endif // FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
+
     /* Decide the kind of code we want to generate */
 
     fgSetOptions();