Add SlotArrayCheck in Lowerer instead of IRBuilder

Fixes OS#19767482

We insert LdSlots at the top of the function in jitloopbody for all syms
that are coming in live to the loop. These LdSlots should be restored
correctly on BailOutFromSimpleJitToJitLoopBody.
However we do unreachable code elimination in flowgraph in simplejit,
which can dead code the uses of these syms, and so they will not be
restored on bailout.
This works if we run deadstore pass which will cleanup all the LdSlots
inserted at the top of the function, after we run unreachable block
elimination phase.
But since we turn off deadstore for functions with try/catch, we end up
having a nullptr AV when we do a SlotArrayCheck followed by LdSlot

lib/Backend/Func.cpp

@@ -143,7 +143,6 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
     , constantAddressRegOpnd(alloc)
     , lastConstantAddressRegLoadInstr(nullptr)
     , m_totalJumpTableSizeInBytesForSwitchStatements(0)
-    , slotArrayCheckTable(nullptr)
     , frameDisplayCheckTable(nullptr)
     , stackArgWithFormalsTracker(nullptr)
     , m_forInLoopBaseDepth(0)
@@ -1031,29 +1030,6 @@ Func::GetLocalsPointer() const
 
 #endif
 
-void Func::AddSlotArrayCheck(IR::SymOpnd *fieldOpnd)
-{
-    if (PHASE_OFF(Js::ClosureRangeCheckPhase, this))
-    {
-        return;
-    }
-
-    Assert(IsTopFunc());
-    if (this->slotArrayCheckTable == nullptr)
-    {
-        this->slotArrayCheckTable = SlotArrayCheckTable::New(m_alloc, 4);
-    }
-
-    PropertySym *propertySym = fieldOpnd->m_sym->AsPropertySym();
-    uint32 slot = propertySym->m_propertyId;
-    uint32 *pSlotId = this->slotArrayCheckTable->FindOrInsert(slot, propertySym->m_stackSym->m_id);
-
-    if (pSlotId && (*pSlotId == (uint32)-1 || *pSlotId < slot))
-    {
-        *pSlotId = propertySym->m_propertyId;
-    }
-}
-
 void Func::AddFrameDisplayCheck(IR::SymOpnd *fieldOpnd, uint32 slotId)
 {
     if (PHASE_OFF(Js::ClosureRangeCheckPhase, this))

lib/Backend/Func.h

@@ -665,7 +665,6 @@ public:
     PropertyIdSet lazyBailoutProperties;
     bool anyPropertyMayBeWrittenTo;
 
-    SlotArrayCheckTable *slotArrayCheckTable;
     FrameDisplayCheckTable *frameDisplayCheckTable;
 
     IR::Instr *         m_headInstr;
@@ -995,7 +994,6 @@ public:
     void MarkConstantAddressSyms(BVSparse<JitArenaAllocator> * bv);
     void DisableConstandAddressLoadHoist() { canHoistConstantAddressLoad = false; }
 
-    void AddSlotArrayCheck(IR::SymOpnd *fieldOpnd);
     void AddFrameDisplayCheck(IR::SymOpnd *fieldOpnd, uint32 slotId = (uint32)-1);
 
     void EnsureStackArgWithFormalsTracker();

lib/Backend/IRBuilder.cpp

@@ -314,56 +314,6 @@ IRBuilder::AddEnvOpndForInnerFrameDisplay(IR::Instr *instr, uint offset)
     }
 }
 
-bool
-IRBuilder::DoSlotArrayCheck(IR::SymOpnd *fieldOpnd, bool doDynamicCheck)
-{
-    if (PHASE_OFF(Js::ClosureRangeCheckPhase, m_func))
-    {
-        return true;
-    }
-
-    PropertySym *propertySym = fieldOpnd->m_sym->AsPropertySym();
-    IR::Instr *instrDef = propertySym->m_stackSym->m_instrDef;
-    IR::Opnd *allocOpnd = nullptr;
-
-    if (instrDef == nullptr)
-    {
-        if (doDynamicCheck)
-        {
-            return false;
-        }
-        Js::Throw::FatalInternalError();
-    }
-    switch(instrDef->m_opcode)
-    {
-    case Js::OpCode::NewScopeSlots:
-    case Js::OpCode::NewStackScopeSlots:
-    case Js::OpCode::NewScopeSlotsWithoutPropIds:
-        allocOpnd = instrDef->GetSrc1();
-        break;
-
-    case Js::OpCode::LdSlot:
-    case Js::OpCode::LdSlotArr:
-        if (doDynamicCheck)
-        {
-            return false;
-        }
-        // fall through
-    default:
-        Js::Throw::FatalInternalError();
-    }
-
-    uint32 allocCount = allocOpnd->AsIntConstOpnd()->AsUint32();
-    uint32 slotId = (uint32)propertySym->m_propertyId;
-
-    if (slotId >= allocCount)
-    {
-        Js::Throw::FatalInternalError();
-    }
-
-    return true;
-}
-
 ///----------------------------------------------------------------------------
 ///
 /// IRBuilder::Build
@@ -904,40 +854,6 @@ IRBuilder::Build()
 void
 IRBuilder::EmitClosureRangeChecks()
 {
-    // Emit closure range checks
-    if (m_func->slotArrayCheckTable)
-    {
-        // Local slot array checks, should only be necessary in jitted loop bodies.
-        FOREACH_HASHTABLE_ENTRY(uint32, bucket, m_func->slotArrayCheckTable)
-        {
-            uint32 slotId = bucket.element;
-            Assert(slotId != (uint32)-1 && slotId >= Js::ScopeSlots::FirstSlotIndex);
-
-            if (slotId > Js::ScopeSlots::FirstSlotIndex)
-            {
-                // Emit a SlotArrayCheck instruction, chained to the instruction (LdSlot) that defines the pointer.
-                StackSym *stackSym = m_func->m_symTable->FindStackSym(bucket.value);
-                Assert(stackSym && stackSym->m_instrDef);
-
-                IR::Instr *instrDef = stackSym->m_instrDef;
-                IR::Instr *insertInstr = instrDef->m_next;
-                IR::RegOpnd *dstOpnd = instrDef->UnlinkDst()->AsRegOpnd();
-                IR::Instr *instr = IR::Instr::New(Js::OpCode::SlotArrayCheck, dstOpnd, m_func);
-
-                dstOpnd = IR::RegOpnd::New(TyVar, m_func);
-                instrDef->SetDst(dstOpnd);
-                instr->SetSrc1(dstOpnd);
-
-                // Attach the slot ID to the check instruction.
-                IR::IntConstOpnd *slotIdOpnd = IR::IntConstOpnd::New(bucket.element, TyUint32, m_func);
-                instr->SetSrc2(slotIdOpnd);
-
-                insertInstr->InsertBefore(instr);
-            }
-        }
-        NEXT_HASHTABLE_ENTRY;
-    }
-
     if (m_func->frameDisplayCheckTable)
     {
         // Frame display checks. Again, chain to the instruction (LdEnv/LdSlot).
@@ -3571,8 +3487,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
                 if (IsLoopBody())
                 {
                     fieldOpnd = this->BuildFieldOpnd(Js::OpCode::LdSlotArr, closureSym->m_id, slotId, (Js::PropertyIdIndexType)-1, PropertyKindSlotArray);
-                    // Need a dynamic check on the size of the local slot array.
-                    m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
                 }
             }
             else if (IsLoopBody())
@@ -3594,11 +3508,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
             }
             this->AddInstr(instr, offset);
 
-            if (!m_func->DoStackFrameDisplay() && IsLoopBody())
-            {
-                // Need a dynamic check on the size of the local slot array.
-                m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
-            }
             break;
 
         case Js::OpCode::LdParamObjSlot:
@@ -3676,8 +3585,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
                 if (IsLoopBody())
                 {
                     fieldOpnd = this->BuildFieldOpnd(Js::OpCode::LdSlotArr, closureSym->m_id, slotId, (Js::PropertyIdIndexType)-1, PropertyKindSlotArray);
-                    // Need a dynamic check on the size of the local slot array.
-                    m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
                 }
             }
             else
@@ -3697,11 +3604,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
                 instr->SetSrc2(fieldOpnd);
             }
 
-            if (!m_func->DoStackFrameDisplay() && IsLoopBody())
-            {
-                // Need a dynamic check on the size of the local slot array.
-                m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
-            }
             break;
 
         case Js::OpCode::StParamObjSlot:
@@ -4013,11 +3915,6 @@ IRBuilder::BuildElementSlotI2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
             else
             {
                 fieldOpnd = this->BuildFieldOpnd(Js::OpCode::StSlot, slotId1, slotId2, (Js::PropertyIdIndexType)-1, PropertyKindSlots);
-                if (!this->DoSlotArrayCheck(fieldOpnd, IsLoopBody()))
-                {
-                    // Need a dynamic check on the size of the local slot array.
-                    m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
-                }
             }
             newOpcode = 
                 newOpcode == Js::OpCode::StInnerObjSlot || newOpcode == Js::OpCode::StInnerSlot ?
@@ -4056,11 +3953,6 @@ IRBuilder::BuildElementSlotI2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
             else
             {
                 fieldOpnd = this->BuildFieldOpnd(Js::OpCode::LdSlot, slotId1, slotId2, (Js::PropertyIdIndexType)-1, PropertyKindSlots);
-                if (!this->DoSlotArrayCheck(fieldOpnd, IsLoopBody()))
-                {
-                    // Need a dynamic check on the size of the local slot array.
-                    m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
-                }
             }
             regOpnd = this->BuildDstOpnd(regSlot);
             instr = IR::Instr::New(Js::OpCode::LdSlot, regOpnd, fieldOpnd, m_func);

lib/Backend/IRBuilder.h

@@ -292,7 +292,6 @@ private:
     Js::RegSlot         GetEnvRegForEvalCode() const;
     Js::RegSlot         GetEnvRegForInnerFrameDisplay() const;
     void                AddEnvOpndForInnerFrameDisplay(IR::Instr *instr, uint offset);
-    bool                DoSlotArrayCheck(IR::SymOpnd *fieldOpnd, bool doDynamicCheck);
     void                EmitClosureRangeChecks();
     void                DoClosureRegCheck(Js::RegSlot reg);
     void                BuildInitCachedScope(int auxOffset, int offset);

lib/Backend/Lower.cpp

@@ -2251,13 +2251,20 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
             break;
 
         case Js::OpCode::StSlot:
+        {
+            PropertySym *propertySym = instr->GetDst()->AsSymOpnd()->m_sym->AsPropertySym();
+            instrPrev = AddSlotArrayCheck(propertySym, instr);
             this->LowerStSlot(instr);
             break;
+        }
 
         case Js::OpCode::StSlotChkUndecl:
+        {
+            PropertySym *propertySym = instr->GetDst()->AsSymOpnd()->m_sym->AsPropertySym();
+            instrPrev = AddSlotArrayCheck(propertySym, instr);
             this->LowerStSlotChkUndecl(instr);
             break;
-
+        }
         case Js::OpCode::ProfiledLoopStart:
         {
             Assert(m_func->DoSimpleJitDynamicProfile());
@@ -2432,6 +2439,10 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
 #endif
 
         case Js::OpCode::LdSlot:
+        {
+            PropertySym *propertySym = instr->GetSrc1()->AsSymOpnd()->m_sym->AsPropertySym();
+            instrPrev = AddSlotArrayCheck(propertySym, instr);
+        }
         case Js::OpCode::LdSlotArr:
         {
             Js::ProfileId profileId;
@@ -10889,6 +10900,78 @@ Lowerer::CreateOpndForSlotAccess(IR::Opnd * opnd)
     return indirOpnd;
 }
 
+IR::Instr* Lowerer::AddSlotArrayCheck(PropertySym *propertySym, IR::Instr* instr)
+{
+    if (propertySym->m_stackSym != m_func->GetLocalClosureSym() || PHASE_OFF(Js::ClosureRangeCheckPhase, m_func))
+    {
+        return instr->m_prev;
+    }
+
+    IR::Instr *instrDef = propertySym->m_stackSym->m_instrDef;
+
+    bool doDynamicCheck = this->m_func->IsLoopBody();
+    bool insertSlotArrayCheck = false;
+    uint32 slotId = (uint32)propertySym->m_propertyId;
+
+    if (instrDef)
+    {
+        switch (instrDef->m_opcode)
+        {
+        case Js::OpCode::NewScopeSlots:
+        case Js::OpCode::NewStackScopeSlots:
+        case Js::OpCode::NewScopeSlotsWithoutPropIds:
+        {
+            IR::Opnd *allocOpnd = allocOpnd = instrDef->GetSrc1();
+            uint32 allocCount = allocOpnd->AsIntConstOpnd()->AsUint32();
+
+            if (slotId >= allocCount)
+            {
+                Js::Throw::FatalInternalError();
+            }
+            break;
+        }
+        case Js::OpCode::ArgIn_A:
+            break;
+        case Js::OpCode::LdSlot:
+        case Js::OpCode::LdSlotArr:
+        {
+            if (doDynamicCheck && slotId > Js::ScopeSlots::FirstSlotIndex)
+            {
+                insertSlotArrayCheck = true;
+            }
+            break;
+        }
+        case Js::OpCode::SlotArrayCheck:
+        {
+            uint32 currentSlotId = instrDef->GetSrc2()->AsIntConstOpnd()->AsInt32();
+            if (slotId > currentSlotId)
+            {
+                instrDef->ReplaceSrc2(IR::IntConstOpnd::New(slotId, TyUint32, m_func));
+            }
+            break;
+        }
+        default:
+            Js::Throw::FatalInternalError();
+        }
+    }
+    if (insertSlotArrayCheck)
+    {
+        IR::Instr *insertInstr = instrDef->m_next;
+        IR::RegOpnd *dstOpnd = instrDef->UnlinkDst()->AsRegOpnd();
+        IR::Instr *checkInstr = IR::Instr::New(Js::OpCode::SlotArrayCheck, dstOpnd, m_func);
+
+        dstOpnd = IR::RegOpnd::New(TyVar, m_func);
+        instrDef->SetDst(dstOpnd);
+        checkInstr->SetSrc1(dstOpnd);
+
+        // Attach the slot ID to the check instruction.
+        IR::IntConstOpnd *slotIdOpnd = IR::IntConstOpnd::New(slotId, TyUint32, m_func);
+        checkInstr->SetSrc2(slotIdOpnd);
+        insertInstr->InsertBefore(checkInstr);
+    }
+    return instr->m_prev;
+}
+
 IR::Instr *
 Lowerer::LowerStSlot(IR::Instr *instr)
 {

lib/Backend/Lower.h

@@ -229,6 +229,7 @@ private:
     IR::Instr *     LowerDeleteElemI(IR::Instr *instr, bool strictMode);
     IR::Instr *     LowerStElemC(IR::Instr *instr);
     void            LowerLdArrHead(IR::Instr *instr);
+    IR::Instr*      AddSlotArrayCheck(PropertySym *propertySym, IR::Instr* instr);
     IR::Instr *     LowerStSlot(IR::Instr *instr);
     IR::Instr *     LowerStSlotChkUndecl(IR::Instr *instr);
     void            LowerStLoopBodyCount(IR::Instr* instr);

test/Bugs/loopcrash.js

@@ -0,0 +1,32 @@
+//------------------------------------------------------------------------------------------------------- 
+// 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 i32 = new Int32Array(1); 
+  { 
+    class class0 { 
+    } 
+    class class8 { 
+    } 
+    class class17 {
+      static func91(argMath135) {
+        if (new class0() * h) {  
+        }   
+      }  
+      static func94() { 
+        return class8.func78; 
+      } 
+    } 
+    for (var _strvar2 in i32) { 
+      continue;	     
+      try { 
+      } catch (ex) { 
+        class8; 
+      } 
+    } 
+  } 
+} 
+test0(); 
+test0(); 
+print("Passed");

test/Bugs/rlexe.xml

@@ -557,4 +557,10 @@
       <compile-flags>-esdynamicimport -mutehosterrormsg -args summary -endargs</compile-flags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>loopcrash.js</files>
+      <compile-flags>-maxinterpretcount:1 -maxsimplejitruncount:1 -MinMemOpCount:0 -werexceptionsupport  -bgjit- -loopinterpretcount:1</compile-flags>
+    </default>
+  </test>
 </regress-exe>