[Merge chakra-core/ChakraCore@1e7793cfdf] [1.6>1.7] [MERGE #3378 @meg…

…-gupta] TryFinally Fixes

Merge pull request #3378 from meg-gupta:tryfinallyminorfixes

-  Dont run region propagation code on deleted/dead blocks
- Set m_hasNonBranchRef on a label in eh region which can become unreferenced after globopt removes BrOnException edges
-  Add ByteCodeOffset to BrOnException added from finally to early exit

deps/chakrashim/core/lib/Backend/FlowGraph.cpp

@@ -693,6 +693,7 @@ void FlowGraph::InsertEdgeFromFinallyToEarlyExit(BasicBlock *finallyEndBlock, IR
 
         IR::BranchInstr *brToExit = IR::BranchInstr::New(Js::OpCode::BrOnException, exitLabel, this->func);
         brToExit->m_brFinallyToEarlyExit = true;
+        brToExit->SetByteCodeOffset(lastInstr);
         leaveLabel->InsertBefore(brToExit);
 
         this->AddBlock(brLabel, brToExit, finallyEndBlock->GetNext(), finallyEndBlock /*prevBlock*/);
@@ -704,6 +705,7 @@ void FlowGraph::InsertEdgeFromFinallyToEarlyExit(BasicBlock *finallyEndBlock, IR
     {
         // If the Leave/LeaveNull at the end of finally was preceeded by a Label, we reuse the block inserting BrOnException to early exit in it
         IR::BranchInstr *brToExit = IR::BranchInstr::New(Js::OpCode::BrOnException, exitLabel, this->func);
+        brToExit->SetByteCodeOffset(lastInstr);
         brToExit->m_brFinallyToEarlyExit = true;
         leaveLabel->InsertBefore(brToExit);
         this->AddEdge(finallyEndBlock, exitLabel->GetBasicBlock());
@@ -1978,6 +1980,12 @@ FlowGraph::UpdateRegionForBlockFromEHPred(BasicBlock * block, bool reassign)
         Assert(this->func->HasTry() && (this->func->DoOptimizeTry() || (this->func->IsSimpleJit() && this->func->hasBailout)));
         return;
     }
+    if (block->isDead || block->isDeleted)
+    {
+        // We can end up calling this function with such blocks, return doing nothing
+        // See test5() in tryfinallytests.js
+        return;
+    }
 
     if (block == this->blockList)
     {

deps/chakrashim/core/lib/Backend/GlobOpt.cpp

@@ -2550,44 +2550,14 @@ GlobOpt::OptInstr(IR::Instr *&instr, bool* isInstrRemoved)
     {
         ProcessTryHandler(instr);
     }
-    else if (instr->m_opcode == Js::OpCode::BrOnException)
+    else if (instr->m_opcode == Js::OpCode::BrOnException || instr->m_opcode == Js::OpCode::BrOnNoException)
     {
-        if (instr->AsBranchInstr()->GetTarget()->GetRegion()->GetType() != RegionType::RegionTypeFinally)
-        {
-            // BrOnException was added to model flow from try region to the catch region to assist
-            // the backward pass in propagating bytecode upward exposed info from the catch block
-            // to the try, and to handle break blocks. Removing it here as it has served its purpose
-            // and keeping it around might also have unintended effects while merging block data for
-            // the catch block's predecessors.
-            // Note that the Deadstore pass will still be able to propagate bytecode upward exposed info
-            // because it doesn't skip dead blocks for that.
-            this->RemoveFlowEdgeToCatchBlock(instr);
-            *isInstrRemoved = true;
-            this->currentBlock->RemoveInstr(instr);
-            return instrNext;
-        }
-        else
-        {
-            // We add BrOnException from a finally region to early exit
-            if (this->RemoveFlowEdgeToFinallyOnExceptionBlock(instr))
+        if (this->ProcessExceptionHandlingEdges(instr))
             {
             *isInstrRemoved = true;
-            this->currentBlock->RemoveInstr(instr);
             return instrNext;
         }
     }
-    }
-    else if (instr->m_opcode == Js::OpCode::BrOnNoException)
-    {
-        if (instr->AsBranchInstr()->GetTarget()->GetRegion()->GetType() == RegionType::RegionTypeCatch)
-        {
-            this->RemoveFlowEdgeToCatchBlock(instr);
-        }
-        else
-        {
-            this->RemoveFlowEdgeToFinallyOnExceptionBlock(instr);
-        }
-    }
 
     bool isAlreadyTypeSpecialized = false;
     if (!IsLoopPrePass() && instr->HasBailOutInfo())
@@ -17544,6 +17514,46 @@ GlobOpt::ProcessTryHandler(IR::Instr* instr)
     ToVar(writeThroughSymbolsSet, this->currentBlock);
 }
 
+bool
+GlobOpt::ProcessExceptionHandlingEdges(IR::Instr* instr)
+{
+    Assert(instr->m_opcode == Js::OpCode::BrOnException || instr->m_opcode == Js::OpCode::BrOnNoException);
+
+    if (instr->m_opcode == Js::OpCode::BrOnException)
+    {
+        if (instr->AsBranchInstr()->GetTarget()->GetRegion()->GetType() == RegionType::RegionTypeCatch)
+        {
+            // BrOnException was added to model flow from try region to the catch region to assist
+            // the backward pass in propagating bytecode upward exposed info from the catch block
+            // to the try, and to handle break blocks. Removing it here as it has served its purpose
+            // and keeping it around might also have unintended effects while merging block data for
+            // the catch block's predecessors.
+            // Note that the Deadstore pass will still be able to propagate bytecode upward exposed info
+            // because it doesn't skip dead blocks for that.
+            this->RemoveFlowEdgeToCatchBlock(instr);
+            this->currentBlock->RemoveInstr(instr);
+            return true;
+        }
+        else
+        {
+            // We add BrOnException from a finally region to early exit, remove that since it has served its purpose
+            return this->RemoveFlowEdgeToFinallyOnExceptionBlock(instr);
+        }
+    }
+    else if (instr->m_opcode == Js::OpCode::BrOnNoException)
+    {
+        if (instr->AsBranchInstr()->GetTarget()->GetRegion()->GetType() == RegionType::RegionTypeCatch)
+        {
+            this->RemoveFlowEdgeToCatchBlock(instr);
+        }
+        else
+        {
+            this->RemoveFlowEdgeToFinallyOnExceptionBlock(instr);
+        }
+    }
+    return false;
+}
+
 void
 GlobOpt::InsertToVarAtDefInTryRegion(IR::Instr * instr, IR::Opnd * dstOpnd)
 {
@@ -17664,14 +17674,39 @@ GlobOpt::RemoveFlowEdgeToFinallyOnExceptionBlock(IR::Instr * instr)
     }
 
     Assert(finallyBlock && predBlock);
+
         if (this->func->m_fg->FindEdge(predBlock, finallyBlock))
         {
             predBlock->RemoveDeadSucc(finallyBlock, this->func->m_fg);
+
+        if (instr->m_opcode == Js::OpCode::BrOnException)
+        {
+            this->currentBlock->RemoveInstr(instr);
+        }
+
+        if (finallyBlock->GetFirstInstr()->AsLabelInstr()->IsUnreferenced())
+        {
+            // Traverse predBlocks of finallyBlock, if any of the preds have a different region, set m_hasNonBranchRef to true
+            // If not, this label can get eliminated and an incorrect region from the predecessor can get propagated in lowered code
+            // See test3() in tryfinallytests.js
+
+            Region * finallyRegion = finallyBlock->GetFirstInstr()->AsLabelInstr()->GetRegion();
+            FOREACH_PREDECESSOR_BLOCK(pred, finallyBlock)
+            {
+                Region * predRegion = pred->GetFirstInstr()->AsLabelInstr()->GetRegion();
+                if (predRegion != finallyRegion)
+                {
+                    finallyBlock->GetFirstInstr()->AsLabelInstr()->m_hasNonBranchRef = true;
+                }
+            } NEXT_PREDECESSOR_BLOCK;
+        }
+
             if (predBlock == this->currentBlock)
             {
                 predBlock->DecrementDataUseCount();
             }
         }
+
     return true;
 }
 

deps/chakrashim/core/lib/Backend/GlobOpt.h

@@ -921,6 +921,7 @@ class GlobOpt
     void                    OptimizeIndirUses(IR::IndirOpnd *indir, IR::Instr * *pInstr, Value **indirIndexValRef);
     void                    RemoveCodeAfterNoFallthroughInstr(IR::Instr *instr);
     void                    ProcessTryHandler(IR::Instr* instr);
+    bool                    ProcessExceptionHandlingEdges(IR::Instr* instr);
     void                    InsertToVarAtDefInTryRegion(IR::Instr * instr, IR::Opnd * dstOpnd);
     void                    RemoveFlowEdgeToCatchBlock(IR::Instr * instr);
     bool                    RemoveFlowEdgeToFinallyOnExceptionBlock(IR::Instr * instr);

deps/chakrashim/core/test/EH/tryfinallytests.js

@@ -46,6 +46,63 @@ function test2() {
   ary | 0;
 }
 
+function test3() {
+  var IntArr0 = new Array();
+  var VarArr0 = [];
+  try {
+  } catch (ex) {
+  } finally {
+    do {
+      try {
+        VarArr0.reverse();
+      } catch (ex) {
+        continue;
+      } finally {
+      }
+    } while (IntArr0[5]);
+  }
+}
+
+function test5() {
+  for (let rwcjvd = 0, ljcyer = /x/g; rwcjvd; rwcjvd) {
+    {
+      if (/x/) {
+        try {
+        } catch (e) {
+        }
+      } else {
+        throw "err" ;
+        while (new Array.isArray(/x/g, 'u56DC') && 0) {
+        }
+      }
+    }
+  }
+}
+
+function test4() {
+  var obj0 = {};
+  var obj1 = {};
+  var arrObj0 = {};
+  var func0 = function () {
+  };
+  var func2 = function () {
+  };
+  obj0.method1 = func0;
+  obj1.method1 = func2;
+  var ary = Array();
+  var protoObj1 = Object(obj1);
+  while (typeof 11) {
+    protoObj1.method1(obj0.method1(ary.unshift((Object.defineProperty(obj1, 'prop1', {})))));
+    try {
+    } catch (ex) {
+      continue;
+    } finally {
+      obj1.prop1 = typeof arrObj0.length;
+      break;
+    }
+  }
+}
+
 test0();
 test0();
 test0();
@@ -58,4 +115,17 @@ test2();
 test2();
 test2();
 
+test3();
+test3();
+test3();
+
+test4();
+test4();
+test4();
+
+test5();
+test5();
+test5();
+
+
 WScript.Echo("passed");