[MERGE #1530 @akroshg] Address deref issue

Merge pull request #1530 from akroshg:deref During the forward global optimizer pass, given a property store that causes an object layout to go from object-header-inlined to non-object-header-inlined, kill all type syms with object-header-inlined types to protect against aliasing.
chakra-core · Sep 2, 2016 · 4f5fd22 · 4f5fd22
2 parents d38d5e0 + 5ec2d8f
commit 4f5fd22
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 23 deletions.
diff --git a/lib/Backend/GlobOptFields.cpp b/lib/Backend/GlobOptFields.cpp
@@ -2170,7 +2170,7 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
         isObjTypeSpecialized = ProcessPropOpInTypeCheckSeq<true>(instr, opnd, block, updateExistingValue, emitsTypeCheckOut, changesTypeValueOut, &isObjTypeChecked);
     }
 
-    if (opnd == instr->GetDst() && this->objectTypeSyms && !isObjTypeChecked)
+    if (opnd == instr->GetDst() && this->objectTypeSyms)
     {
         if (block == nullptr)
         {
@@ -2180,26 +2180,29 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
         // This is a property store that may change the layout of the object that it stores to. This means that
         // it may change any aliased object. Do two things to address this:
         // - Add all object types in this function to the set that may have had a property added. This will prevent
-        //   final type optimization across this instruction.
+        //   final type optimization across this instruction. (Only needed here for non-specialized stores.)
         // - Kill all type symbols that currently hold object-header-inlined types. Any of them may have their layout
         //   changed by the addition of a property.
 
         SymID opndId = opnd->HasObjectTypeSym() ? opnd->GetObjectTypeSym()->m_id : -1;
-        if (block->globOptData.maybeWrittenTypeSyms == nullptr)
-        {
-            block->globOptData.maybeWrittenTypeSyms = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
-        }
-        if (isObjTypeSpecialized)
+        if (!isObjTypeChecked)
         {
-            // The current object will be protected by a type check, unless no further accesses to it are
-            // protected by this access.
-            Assert(this->objectTypeSyms->Test(opndId));
-            this->objectTypeSyms->Clear(opndId);
-        }
-        block->globOptData.maybeWrittenTypeSyms->Or(this->objectTypeSyms);
-        if (isObjTypeSpecialized)
-        {
-            this->objectTypeSyms->Set(opndId);
+            if (block->globOptData.maybeWrittenTypeSyms == nullptr)
+            {
+                block->globOptData.maybeWrittenTypeSyms = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
+            }
+            if (isObjTypeSpecialized)
+            {
+                // The current object will be protected by a type check, unless no further accesses to it are
+                // protected by this access.
+                Assert(this->objectTypeSyms->Test(opndId));
+                this->objectTypeSyms->Clear(opndId);
+            }
+            block->globOptData.maybeWrittenTypeSyms->Or(this->objectTypeSyms);
+            if (isObjTypeSpecialized)
+            {
+                this->objectTypeSyms->Set(opndId);
+            }
         }
 
         if (!isObjTypeSpecialized || opnd->ChangesObjectLayout())

diff --git a/lib/Backend/Opnd.cpp b/lib/Backend/Opnd.cpp
@@ -846,20 +846,43 @@ PropertySymOpnd::IsObjectHeaderInlined() const
 bool
 PropertySymOpnd::ChangesObjectLayout() const
 {
+    Js::Type *cachedType = this->IsMono() ? this->GetType() : this->GetFirstEquivalentType();
+
     Js::Type *finalType = this->GetFinalType();
-    if (finalType == nullptr || !Js::DynamicType::Is(finalType->GetTypeId()))
+    if (finalType && Js::DynamicType::Is(finalType->GetTypeId()))
+    {
+        // This is the case where final type opt may cause pro-active type transition to take place.
+
+        Assert(cachedType && Js::DynamicType::Is(cachedType->GetTypeId()));
+
+        Js::DynamicTypeHandler * cachedTypeHandler = (static_cast<Js::DynamicType*>(cachedType))->GetTypeHandler();
+        Js::DynamicTypeHandler * finalTypeHandler = (static_cast<Js::DynamicType*>(finalType))->GetTypeHandler();
+
+        return cachedTypeHandler->GetInlineSlotCapacity() != finalTypeHandler->GetInlineSlotCapacity() ||
+            cachedTypeHandler->GetOffsetOfInlineSlots() != finalTypeHandler->GetOffsetOfInlineSlots();
+    }
+
+    if (!this->HasInitialType())
     {
         return false;
     }
 
-    Js::Type *cachedType = this->IsMono() ? this->GetType() : this->GetFirstEquivalentType();
-    Assert(cachedType && Js::DynamicType::Is(cachedType->GetTypeId()));
+    Js::Type *initialType = this->GetInitialType();
+    if (initialType && Js::DynamicType::Is(initialType->GetTypeId()))
+    {
+        // This is the case where the type transition actually occurs. (This is the only case that's detectable
+        // during the loop pre-pass, since final types are not in place yet.)
 
-    Js::DynamicTypeHandler * cachedTypeHandler = (static_cast<Js::DynamicType*>(cachedType))->GetTypeHandler();
-    Js::DynamicTypeHandler * finalTypeHandler = (static_cast<Js::DynamicType*>(finalType))->GetTypeHandler();
+        Assert(cachedType && Js::DynamicType::Is(cachedType->GetTypeId()));
 
-    return cachedTypeHandler->GetInlineSlotCapacity() != finalTypeHandler->GetInlineSlotCapacity() ||
-        cachedTypeHandler->GetOffsetOfInlineSlots() != finalTypeHandler->GetOffsetOfInlineSlots();
+        Js::DynamicTypeHandler * cachedTypeHandler = (static_cast<Js::DynamicType*>(cachedType))->GetTypeHandler();
+        Js::DynamicTypeHandler * initialTypeHandler = (static_cast<Js::DynamicType*>(initialType))->GetTypeHandler();
+
+        return cachedTypeHandler->GetInlineSlotCapacity() != initialTypeHandler->GetInlineSlotCapacity() ||
+            cachedTypeHandler->GetOffsetOfInlineSlots() != initialTypeHandler->GetOffsetOfInlineSlots();
+    }
+
+    return false;
 }
 
 void