Add try finally test cases and fix asserts

lib/Backend/BailOut.cpp

@@ -1095,6 +1095,9 @@ Js::Var BailOutRecord::BailOut(BailOutRecord const * bailOutRecord)
     Js::JavascriptCallStackLayout *const layout = bailOutRecord->GetStackLayout();
     Js::ScriptFunction * function = (Js::ScriptFunction *)layout->functionObject;
 
+    Assert(function->GetScriptContext()->GetThreadContext()->GetPendingFinallyException() == nullptr ||
+        bailOutRecord->bailOutKind == IR::BailOutOnException);
+
     if (bailOutRecord->bailOutKind == IR::BailOutOnImplicitCalls)
     {
         function->GetScriptContext()->GetThreadContext()->CheckAndResetImplicitCallAccessorFlag();

lib/Backend/FlowGraph.cpp

@@ -933,6 +933,7 @@ Loop::InsertLandingPad(FlowGraph *fg)
 
     landingPadLabel->SetBasicBlock(landingPad);
     landingPadLabel->SetRegion(headBlock->GetFirstInstr()->AsLabelInstr()->GetRegion());
+    landingPadLabel->m_hasNonBranchRef = headBlock->GetFirstInstr()->AsLabelInstr()->m_hasNonBranchRef;
     landingPad->SetBlockNum(fg->blockCount++);
     landingPad->SetFirstInstr(landingPadLabel);
     landingPad->SetLastInstr(landingPadLabel);
@@ -1768,6 +1769,10 @@ FlowGraph::Destroy(void)
                         break;
                     case Js::OpCode::BrOnNoException:
                         Assert(region->GetType() == RegionTypeTry || region->GetType() == RegionTypeCatch || region->GetType() == RegionTypeFinally ||
+                            // A BrOnException from finally to early exit can be converted to BrOnNoException and Br
+                            // The Br block maybe a common successor block for early exit along with the BrOnNoException block
+                            // Region from Br block will be picked up from a predecessor which is not BrOnNoException due to early exit
+                            // See test0() in test/EH/tryfinallytests.js
                             (predRegion->GetType() == RegionTypeFinally && predBlock->GetLastInstr()->AsBranchInstr()->m_brFinallyToEarlyExit));
                         break;
                     case Js::OpCode::Br:
@@ -1782,11 +1787,13 @@ FlowGraph::Destroy(void)
                         }
                         else if (region->GetType() == RegionTypeFinally && region != predRegion)
                         {
-                            AssertMsg(predRegion->GetType() == RegionTypeTry, "Bad region type for the try");
+                            // We may be left with edges from finally region to early exit
+                            AssertMsg(predRegion->IsNonExceptingFinally() || predRegion->GetType() == RegionTypeTry, "Bad region type for the try");
                         }
                         else
                         {
-                            AssertMsg(region == predRegion, "Bad region propagation through interior block");
+                            // We may be left with edges from finally region to early exit
+                            AssertMsg(predRegion->IsNonExceptingFinally() || region == predRegion, "Bad region propagation through interior block");
                         }
                         break;
                     default:
@@ -1895,7 +1902,6 @@ FlowGraph::UpdateRegionForBlock(BasicBlock * block)
         // If a LeaveNull block had only BrOnException edges from predecessor
         // We will end up in inaccurate region propagation if we propagate from the predecessor edges
         // So pick up the finally region from the map
-        Assert(this->leaveNullLabelToFinallyLabelMap->ContainsKey(block->GetFirstInstr()->AsLabelInstr()));
         IR::LabelInstr * finallyLabel = this->leaveNullLabelToFinallyLabelMap->Item(block->GetFirstInstr()->AsLabelInstr());
         Assert(finallyLabel);
         region = finallyLabel->GetRegion();
@@ -1939,7 +1945,11 @@ FlowGraph::UpdateRegionForBlock(BasicBlock * block)
             labelInstr->m_hasNonBranchRef = true;
         }
 
-        if (region && this->func->HasFinally() && region->GetType() == RegionTypeRoot && !labelInstr->m_hasNonBranchRef)
+        // One of the pred blocks maybe an eh region, in that case it is important to mark this label's m_hasNonBranchRef
+        // If not later in codegen, this label can get deleted. And during SccLiveness, region is propagated to newly created labels in lowerer from the previous label's region
+        // We can end up assigning an eh region to a label in a non eh region. And if there is a bailout in such a region, bad things will happen in the interpreter :)
+        // See test2() in tryfinallytests.js
+        if (region && region->GetType() == RegionTypeRoot && !labelInstr->m_hasNonBranchRef)
         {
             FOREACH_PREDECESSOR_BLOCK(predBlock, block)
             {
@@ -2440,7 +2450,7 @@ FlowGraph::InsertCompensationCodeForBlockMove(FlowEdge * edge,  bool insertToLoo
 
     bool assignRegionsBeforeGlobopt = this->func->HasTry() && (this->func->DoOptimizeTry() ||
         (this->func->IsSimpleJit() && this->func->hasBailout));
-    // MGTODO : maybe we can just set the pred region instead of calling Propagate function ?
+
     if (assignRegionsBeforeGlobopt)
     {
         UpdateRegionForBlockFromEHPred(compBlock);

lib/Backend/GlobOpt.cpp

@@ -17617,6 +17617,11 @@ GlobOpt::RemoveFlowEdgeToFinallyOnExceptionBlock(IR::Instr * instr)
     {
         // We add edge from finally to early exit block
         // We should not remove this edge
+        // If a loop has continue, and we add edge in finally to continue
+        // Break block removal can move all continues inside the loop to branch to the continue added within finally
+        // If we get rid of this edge, then loop may loose all backedges
+        // Ideally, doing tail duplication before globopt would enable us to remove these edges, but since we do it after globopt, keep it this way for now
+        // See test1() in core/test/tryfinallytests.js
         return false;
     }
 
@@ -17643,9 +17648,9 @@ GlobOpt::RemoveFlowEdgeToFinallyOnExceptionBlock(IR::Instr * instr)
         {
             if (!(nextLabel->m_next->IsBranchInstr() && nextLabel->m_next->AsBranchInstr()->IsUnconditional()))
             {
-                // Already processed in loop prepass
                 return false;
             }
+
             BasicBlock * nextBlock = nextLabel->GetBasicBlock();
             IR::BranchInstr * branchTofinallyBlockOrEarlyExit = nextLabel->m_next->AsBranchInstr();
             IR::LabelInstr * finallyBlockLabelOrEarlyExitLabel = branchTofinallyBlockOrEarlyExit->GetTarget();

lib/Backend/IR.h

@@ -750,7 +750,7 @@ public:
     static BranchInstr * New(Js::OpCode opcode, Opnd* destOpnd, LabelInstr * branchTarget, Opnd *srcOpnd, Func *func);
     static BranchInstr * New(Js::OpCode opcode, LabelInstr * branchTarget, Opnd *src1Opnd, Opnd *src2Opnd, Func *func);
 
-    BranchInstr(bool hasBailOutInfo = false) : Instr(hasBailOutInfo), m_branchTarget(nullptr), m_isAirlock(false), m_isSwitchBr(false), m_isOrphanedLeave(false), m_areCmpRegisterFlagsUsedLater(false), m_brFinallyToEarlyExit(nullptr)
+    BranchInstr(bool hasBailOutInfo = false) : Instr(hasBailOutInfo), m_branchTarget(nullptr), m_isAirlock(false), m_isSwitchBr(false), m_isOrphanedLeave(false), m_areCmpRegisterFlagsUsedLater(false), m_brFinallyToEarlyExit(false)
     {
 #if DBG
         m_isMultiBranch = false;

test/EH/rlexe.xml

@@ -113,4 +113,9 @@
       <baseline>tryfinallybug0.baseline</baseline>
     </default>
   </test>
+  <test>
+    <default>
+      <files>tryfinallytests.js</files>
+    </default>
+  </test>
 </regress-exe>

test/EH/tryfinallytests.js

@@ -0,0 +1,61 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+function test0() {
+  var ui32 = new Uint32Array();
+  for (var _strvar23 in ui32) {
+    if (typeof _strvar23) {
+      continue;
+    }
+    try {
+    } catch (ex) {
+      continue;
+    } finally {
+    }
+    break;
+  }
+}
+
+function test1() {
+  var arrObj0 = {};
+  if (arrObj0.length) {
+    for (var _strvar0 of IntArr0) {
+      if (typeof _strvar0) {
+        continue;
+      }
+      try {
+      } catch (ex) {
+        continue;
+      } finally {
+      }
+      break;
+    }
+  }
+}
+
+function test2() {
+  var obj1 = {};
+  var ary = Array();
+  var i8 = new Int8Array();
+  if (!(-530320868 >= ((obj1.method1) & 255))) {
+    for (var _strvar1 of i8) {
+    }
+  }
+  ary | 0;
+}
+
+test0();
+test0();
+test0();
+
+test1();
+test1();
+test1();
+
+test2();
+test2();
+test2();
+
+WScript.Echo("passed");