[MERGE #3498 @ricobbe] OS:9682944 Fix bug in which we incorrectly cop…

…y-propagated away a load from a stack slot after boxing locals onto the heap Merge pull request #3498 from ricobbe:stack-copy-prop-fewer-slot-syms Fix a bug that can cause us to generate code that refers to the stack local variable storage even after the variables have been moved to the heap. Ensure that all references to local variables reload the pointer to the local variable storage area after any instructions that modify that pointer.
chakra-core · Aug 15, 2017 · 9ac1422 · 9ac1422
2 parents b7cbc50 + e050ec2
commit 9ac1422
Show file tree

Hide file tree

Showing 7 changed files with 3,140 additions and 5 deletions.
diff --git a/lib/Backend/GlobOpt.cpp b/lib/Backend/GlobOpt.cpp
@@ -360,6 +360,7 @@ GlobOpt::ForwardPass()
     this->byteCodeConstantValueNumbersBv = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
     this->tempBv = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
     this->prePassCopyPropSym = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
+    this->slotSyms = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
     this->byteCodeUses = nullptr;
     this->propertySymUse = nullptr;
 
@@ -5185,6 +5186,18 @@ GlobOpt::ValueNumberDst(IR::Instr **pInstr, Value *src1Val, Value *src2Val)
 
     case Js::OpCode::Typeof:
         return this->NewGenericValue(ValueType::String, dst);
+    case Js::OpCode::InitLocalClosure:
+        Assert(instr->GetDst());
+        Assert(instr->GetDst()->IsRegOpnd());
+        IR::RegOpnd *regOpnd = instr->GetDst()->AsRegOpnd();
+        StackSym *opndStackSym = regOpnd->m_sym;
+        Assert(opndStackSym != nullptr);
+        ObjectSymInfo *objectSymInfo = opndStackSym->m_objectInfo;
+        Assert(objectSymInfo != nullptr);
+        for (PropertySym *localVarSlotList = objectSymInfo->m_propertySymList; localVarSlotList; localVarSlotList = localVarSlotList->m_nextInStackSymList)
+        {
+            this->slotSyms->Set(localVarSlotList->m_id);
+        }
         break;
     }
 

diff --git a/lib/Backend/GlobOpt.h b/lib/Backend/GlobOpt.h
@@ -414,6 +414,11 @@ class GlobOpt
     BVSparse<JitArenaAllocator> *  objectTypeSyms;
     BVSparse<JitArenaAllocator> *  prePassCopyPropSym;  // Symbols that were copy prop'd during loop prepass
 
+    // Symbols that refer to slots in the stack frame.  We still use currentBlock->liveFields to tell us
+    // which of these slots are live; this bit-vector just identifies which entries in liveFields represent
+    // slots, so we can zero them all out quickly.
+    BVSparse<JitArenaAllocator> *  slotSyms;
+
     PropertySym *               propertySymUse;
 
     BVSparse<JitArenaAllocator> *  lengthEquivBv;
@@ -838,7 +843,7 @@ class GlobOpt
     void                    ProcessFieldKills(IR::Instr * instr);
     void                    KillLiveFields(StackSym * stackSym, BVSparse<JitArenaAllocator> * bv);
     void                    KillLiveFields(PropertySym * propertySym, BVSparse<JitArenaAllocator> * bv);
-    void                    KillLiveFields(BVSparse<JitArenaAllocator> *const propertyEquivSet, BVSparse<JitArenaAllocator> *const bv) const;
+    void                    KillLiveFields(BVSparse<JitArenaAllocator> *const fieldsToKill, BVSparse<JitArenaAllocator> *const bv) const;
     void                    KillLiveElems(IR::IndirOpnd * indirOpnd, BVSparse<JitArenaAllocator> * bv, bool inGlobOpt, Func *func);
     void                    KillAllFields(BVSparse<JitArenaAllocator> * bv);
     void                    SetAnyPropertyMayBeWrittenTo();

diff --git a/lib/Backend/GlobOptFields.cpp b/lib/Backend/GlobOptFields.cpp
@@ -332,19 +332,19 @@ GlobOpt::KillLiveFields(PropertySym * propertySym, BVSparse<JitArenaAllocator> *
     KillLiveFields(propertySym->m_propertyEquivSet, bv);
 }
 
-void GlobOpt::KillLiveFields(BVSparse<JitArenaAllocator> *const propertyEquivSet, BVSparse<JitArenaAllocator> *const bv) const
+void GlobOpt::KillLiveFields(BVSparse<JitArenaAllocator> *const fieldsToKill, BVSparse<JitArenaAllocator> *const bv) const
 {
     Assert(bv);
 
-    if (propertyEquivSet)
+    if (fieldsToKill)
     {
-        bv->Minus(propertyEquivSet);
+        bv->Minus(fieldsToKill);
 
         if (this->IsLoopPrePass())
         {
             for (Loop * loop = this->rootLoopPrePass; loop != nullptr; loop = loop->parent)
             {
-                loop->fieldKilled->Or(propertyEquivSet);
+                loop->fieldKilled->Or(fieldsToKill);
             }
         }
     }
@@ -564,6 +564,15 @@ GlobOpt::ProcessFieldKills(IR::Instr *instr, BVSparse<JitArenaAllocator> *bv, bo
         }
         break;
 
+    case Js::OpCode::LdHeapArguments:
+    case Js::OpCode::LdLetHeapArguments:
+    case Js::OpCode::LdHeapArgsCached:
+    case Js::OpCode::LdLetHeapArgsCached:
+        if (inGlobOpt) {
+            this->KillLiveFields(this->slotSyms, bv);
+        }
+        break;
+
     default:
         if (instr->UsesAllFields())
         {