Fix crash with ObjectStructureWithMap and inline cache

If inline cache refers ObjectStructure, ObjectStructure should keep its contents. Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
Samsung · Jul 20, 2023 · 87fda52 · 87fda52
1 parent 74735f9
commit 87fda52
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 23 deletions.
diff --git a/src/interpreter/ByteCodeInterpreter.cpp b/src/interpreter/ByteCodeInterpreter.cpp
@@ -2410,6 +2410,7 @@ NEVER_INLINE void InterpreterSlowPath::getObjectPrecomputedCaseOperation(Executi
 
     while (true) {
         auto s = obj->structure();
+        s->markReferencedByInlineCache();
         cachedhiddenClassChain.push_back(s);
         auto result = s->findProperty(propertyName);
 
@@ -4139,6 +4140,8 @@ NEVER_INLINE void InterpreterSlowPath::objectDefineOwnPropertyWithNameOperation(
                     byteCodeBlock->m_otherLiteralData.push_back(newStructure);
                     code->m_inlineCachedStructureBefore = oldStructure;
                     code->m_inlineCachedStructureAfter = newStructure;
+                    oldStructure->markReferencedByInlineCache();
+                    newStructure->markReferencedByInlineCache();
                 } else {
                     // failed to cache
                 }

diff --git a/src/runtime/ObjectStructure.cpp b/src/runtime/ObjectStructure.cpp
@@ -108,16 +108,24 @@ ObjectStructure* ObjectStructureWithoutTransition::addProperty(const ObjectStruc
     bool nameIsSymbol = m_hasSymbolPropertyName ? true : name.isSymbol();
     bool hasNonAtomicName = m_hasNonAtomicPropertyName ? true : !name.hasAtomicString();
     bool hasEnumerableProperty = m_hasEnumerableProperty ? true : desc.isEnumerable();
+
     ObjectStructure* newStructure;
-    m_properties->push_back(newItem);
+    ObjectStructureItemVector* propertiesForNewStructure;
+    if (m_isReferencedByInlineCache) {
+        propertiesForNewStructure = new ObjectStructureItemVector(*m_properties, newItem);
+    } else {
+        m_properties->push_back(newItem);
+        propertiesForNewStructure = m_properties;
+        m_properties = nullptr;
+    }
 
-    if (m_properties->size() + 1 > ESCARGOT_OBJECT_STRUCTURE_ACCESS_CACHE_BUILD_MIN_SIZE) {
-        newStructure = new ObjectStructureWithMap(m_properties, ObjectStructureWithMap::createPropertyNameMap(m_properties), nameIsIndexString, nameIsSymbol, hasEnumerableProperty);
+
+    if (propertiesForNewStructure->size() > ESCARGOT_OBJECT_STRUCTURE_ACCESS_CACHE_BUILD_MIN_SIZE) {
+        newStructure = new ObjectStructureWithMap(propertiesForNewStructure, ObjectStructureWithMap::createPropertyNameMap(propertiesForNewStructure), nameIsIndexString, nameIsSymbol, hasEnumerableProperty);
     } else {
-        newStructure = new ObjectStructureWithoutTransition(m_properties, nameIsIndexString, nameIsSymbol, hasNonAtomicName, hasEnumerableProperty);
+        newStructure = new ObjectStructureWithoutTransition(propertiesForNewStructure, nameIsIndexString, nameIsSymbol, hasNonAtomicName, hasEnumerableProperty);
     }
 
-    m_properties = nullptr;
     return newStructure;
 }
 
@@ -145,16 +153,23 @@ ObjectStructure* ObjectStructureWithoutTransition::removeProperty(size_t pIndex)
     }
 
     auto newStructure = new ObjectStructureWithoutTransition(newProperties, hasIndexString, hasSymbol, hasNonAtomicName, hasEnumerableProperty);
-    m_properties = nullptr;
+    if (!m_isReferencedByInlineCache) {
+        m_properties = nullptr;
+    }
     return newStructure;
 }
 
 ObjectStructure* ObjectStructureWithoutTransition::replacePropertyDescriptor(size_t idx, const ObjectStructurePropertyDescriptor& newDesc)
 {
-    m_properties->at(idx).m_descriptor = newDesc;
-    auto newStructure = new ObjectStructureWithoutTransition(m_properties, m_hasIndexPropertyName, m_hasSymbolPropertyName, m_hasNonAtomicPropertyName, m_hasEnumerableProperty);
-    m_properties = nullptr;
-    return newStructure;
+    ObjectStructureItemVector* newProperties = m_properties;
+
+    if (m_isReferencedByInlineCache) {
+        newProperties = new ObjectStructureItemVector(*m_properties);
+    } else {
+        m_properties = nullptr;
+    }
+    newProperties->at(idx).m_descriptor = newDesc;
+    return new ObjectStructureWithoutTransition(newProperties, m_hasIndexPropertyName, m_hasSymbolPropertyName, m_hasNonAtomicPropertyName, m_hasEnumerableProperty);
 }
 
 ObjectStructure* ObjectStructureWithoutTransition::convertToNonTransitionStructure()
@@ -381,11 +396,20 @@ ObjectStructure* ObjectStructureWithMap::addProperty(const ObjectStructureProper
     bool hasSymbol = m_hasSymbolPropertyName ? true : name.isSymbol();
     bool hasEnumerableProperty = m_hasEnumerableProperty ? true : desc.isEnumerable();
 
-    m_propertyNameMap->insert(std::make_pair(name, m_properties->size()));
-    m_properties->push_back(newItem);
-    ObjectStructure* newStructure = new ObjectStructureWithMap(m_properties, m_propertyNameMap, nameIsIndexString, hasSymbol, hasEnumerableProperty);
-    m_properties = nullptr;
-    m_propertyNameMap = nullptr;
+    ObjectStructureItemVector* newProperties = m_properties;
+    PropertyNameMap* newPropertyNameMap = m_propertyNameMap;
+
+    if (m_isReferencedByInlineCache) {
+        newProperties = new ObjectStructureItemVector(*m_properties);
+        newPropertyNameMap = new (GC) PropertyNameMap(*m_propertyNameMap);
+    } else {
+        m_properties = nullptr;
+        m_propertyNameMap = nullptr;
+    }
+
+    newPropertyNameMap->insert(std::make_pair(name, newProperties->size()));
+    newProperties->push_back(newItem);
+    ObjectStructure* newStructure = new ObjectStructureWithMap(newProperties, newPropertyNameMap, nameIsIndexString, hasSymbol, hasEnumerableProperty);
     return newStructure;
 }
 
@@ -412,8 +436,10 @@ ObjectStructure* ObjectStructureWithMap::removeProperty(size_t pIndex)
         newIdx++;
     }
 
-    m_properties = nullptr;
-    m_propertyNameMap = nullptr;
+    if (!m_isReferencedByInlineCache) {
+        m_properties = nullptr;
+        m_propertyNameMap = nullptr;
+    }
     if (newProperties->size() > ESCARGOT_OBJECT_STRUCTURE_ACCESS_CACHE_BUILD_MIN_SIZE) {
         return new ObjectStructureWithMap(newProperties, ObjectStructureWithMap::createPropertyNameMap(newProperties), hasIndexString, hasSymbol, hasEnumerableProperty);
     } else {
@@ -423,11 +449,19 @@ ObjectStructure* ObjectStructureWithMap::removeProperty(size_t pIndex)
 
 ObjectStructure* ObjectStructureWithMap::replacePropertyDescriptor(size_t idx, const ObjectStructurePropertyDescriptor& newDesc)
 {
-    m_properties->at(idx).m_descriptor = newDesc;
-    ObjectStructure* newStructure = new ObjectStructureWithMap(m_properties, m_propertyNameMap, m_hasIndexPropertyName, m_hasSymbolPropertyName, m_hasEnumerableProperty);
-    m_properties = nullptr;
-    m_propertyNameMap = nullptr;
-    return newStructure;
+    ObjectStructureItemVector* newProperties = m_properties;
+    PropertyNameMap* newPropertyNameMap = m_propertyNameMap;
+
+    if (m_isReferencedByInlineCache) {
+        newProperties = new ObjectStructureItemVector(*m_properties);
+        newPropertyNameMap = new (GC) PropertyNameMap(*m_propertyNameMap);
+    } else {
+        m_properties = nullptr;
+        m_propertyNameMap = nullptr;
+    }
+
+    newProperties->at(idx).m_descriptor = newDesc;
+    return new ObjectStructureWithMap(newProperties, newPropertyNameMap, m_hasIndexPropertyName, m_hasSymbolPropertyName, m_hasEnumerableProperty);
 }
 
 ObjectStructure* ObjectStructureWithMap::convertToNonTransitionStructure()

diff --git a/src/runtime/ObjectStructure.h b/src/runtime/ObjectStructure.h
@@ -148,6 +148,11 @@ class ObjectStructure : public gc {
     virtual ObjectStructure* convertToNonTransitionStructure() = 0;
     virtual bool inTransitionMode() = 0;
 
+    void markReferencedByInlineCache()
+    {
+        m_isReferencedByInlineCache = true;
+    }
+
     bool hasIndexPropertyName() const
     {
         return m_hasIndexPropertyName;
@@ -163,6 +168,11 @@ class ObjectStructure : public gc {
         return m_hasEnumerableProperty;
     }
 
+    bool isReferencedByInlineCache() const
+    {
+        return m_isReferencedByInlineCache;
+    }
+
 protected:
     ObjectStructure(bool hasIndexPropertyName,
                     bool hasSymbolPropertyName, bool hasEnumerableProperty)
@@ -171,6 +181,7 @@ class ObjectStructure : public gc {
         , m_hasSymbolPropertyName(hasSymbolPropertyName)
         , m_hasNonAtomicPropertyName(false)
         , m_hasEnumerableProperty(hasEnumerableProperty)
+        , m_isReferencedByInlineCache(false)
         , m_transitionTableVectorBufferSize(0)
         , m_transitionTableVectorBufferCapacity(0)
     {
@@ -183,6 +194,7 @@ class ObjectStructure : public gc {
         , m_hasSymbolPropertyName(hasSymbolPropertyName)
         , m_hasNonAtomicPropertyName(hasNonAtomicPropertyName)
         , m_hasEnumerableProperty(hasEnumerableProperty)
+        , m_isReferencedByInlineCache(false)
         , m_transitionTableVectorBufferSize(0)
         , m_transitionTableVectorBufferCapacity(0)
     {
@@ -195,6 +207,7 @@ class ObjectStructure : public gc {
     bool m_hasSymbolPropertyName : 1;
     bool m_hasNonAtomicPropertyName : 1;
     bool m_hasEnumerableProperty : 1;
+    bool m_isReferencedByInlineCache : 1;
     uint8_t m_transitionTableVectorBufferSize : 8;
     uint8_t m_transitionTableVectorBufferCapacity : 8;
 };

diff --git a/test/vendortest b/test/vendortest