[webkit-changes] [257728] trunk

Mon, 02 Mar 2020 12:58:37 -0800

Title: [257728] trunk
Revision
257728
Author
[email protected]
Date
2020-03-02 12:58:22 -0800 (Mon, 02 Mar 2020)

Log Message

Delete by val caching does not keep the subscript alive
https://bugs.webkit.org/show_bug.cgi?id=208393

Reviewed by Yusuke Suzuki.

JSTests:

* stress/delete-property-ic-stress.js: Added.

Source/_javascript_Core:

Before, the provided test case crashed with asan because we did not keep deleteByVal
subscripts alive. This patch changes CacheableIdentifier to make this mistake harder
to make again, by making the constructor calls more explicit when CacheableIdentifier
will not keep an Identifier alive.

* jit/JITOperations.cpp:
* jit/Repatch.cpp:
(JSC::tryCachePutByID):
(JSC::tryCacheDeleteBy):
(JSC::repatchDeleteBy):
(JSC::tryCacheInByID):
(JSC::tryCacheInstanceOf):
(JSC::tryCacheDelBy): Deleted.
(JSC::repatchDelBy): Deleted.
* jit/Repatch.h:
* runtime/CacheableIdentifier.h:
* runtime/CacheableIdentifierInlines.h:
(JSC::CacheableIdentifier::createFromIdentifierOwnedByCodeBlock):
(JSC::CacheableIdentifier::createFromCell):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (257727 => 257728)


--- trunk/JSTests/ChangeLog	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/JSTests/ChangeLog	2020-03-02 20:58:22 UTC (rev 257728)
@@ -1,3 +1,12 @@
+2020-03-02  Justin Michaud  <[email protected]>
+
+        Delete by val caching does not keep the subscript alive
+        https://bugs.webkit.org/show_bug.cgi?id=208393
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/delete-property-ic-stress.js: Added.
+
 2020-02-27  Justin Michaud  <[email protected]>
 
         Poly proto should work with property delete transitions

Added: trunk/JSTests/stress/delete-property-ic-stress.js (0 => 257728)


--- trunk/JSTests/stress/delete-property-ic-stress.js	                        (rev 0)
+++ trunk/JSTests/stress/delete-property-ic-stress.js	2020-03-02 20:58:22 UTC (rev 257728)
@@ -0,0 +1,5 @@
+let x = {};
+for (let i=0; i<1000; ++i) {
+    gc();
+    delete x["h" + 5];
+}

Modified: trunk/Source/_javascript_Core/ChangeLog (257727 => 257728)


--- trunk/Source/_javascript_Core/ChangeLog	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-03-02 20:58:22 UTC (rev 257728)
@@ -1,3 +1,30 @@
+2020-03-02  Justin Michaud  <[email protected]>
+
+        Delete by val caching does not keep the subscript alive
+        https://bugs.webkit.org/show_bug.cgi?id=208393
+
+        Reviewed by Yusuke Suzuki.
+
+        Before, the provided test case crashed with asan because we did not keep deleteByVal
+        subscripts alive. This patch changes CacheableIdentifier to make this mistake harder
+        to make again, by making the constructor calls more explicit when CacheableIdentifier
+        will not keep an Identifier alive.
+
+        * jit/JITOperations.cpp:
+        * jit/Repatch.cpp:
+        (JSC::tryCachePutByID):
+        (JSC::tryCacheDeleteBy):
+        (JSC::repatchDeleteBy):
+        (JSC::tryCacheInByID):
+        (JSC::tryCacheInstanceOf):
+        (JSC::tryCacheDelBy): Deleted.
+        (JSC::repatchDelBy): Deleted.
+        * jit/Repatch.h:
+        * runtime/CacheableIdentifier.h:
+        * runtime/CacheableIdentifierInlines.h:
+        (JSC::CacheableIdentifier::createFromIdentifierOwnedByCodeBlock):
+        (JSC::CacheableIdentifier::createFromCell):
+
 2020-03-02  Paulo Matos  <[email protected]>
 
         Fix JSC 32bit alignment increase gcc warning

Modified: trunk/Source/_javascript_Core/jit/JITOperations.cpp (257727 => 257728)


--- trunk/Source/_javascript_Core/jit/JITOperations.cpp	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/Source/_javascript_Core/jit/JITOperations.cpp	2020-03-02 20:58:22 UTC (rev 257728)
@@ -214,7 +214,7 @@
 
     CodeBlock* codeBlock = callFrame->codeBlock();
     if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()) && !slot.isTaintedByOpaqueObject() && (slot.isCacheableValue() || slot.isCacheableGetter() || slot.isUnset()))
-        repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::Try);
+        repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::Try);
 
     return JSValue::encode(slot.getPureResult());
 }
@@ -270,7 +270,7 @@
 
     CodeBlock* codeBlock = callFrame->codeBlock();
     if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
-        repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::Direct);
+        repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::Direct);
 
     RELEASE_AND_RETURN(scope, JSValue::encode(found ? slot.getValue(globalObject, ident) : jsUndefined()));
 }
@@ -330,7 +330,7 @@
         
         CodeBlock* codeBlock = callFrame->codeBlock();
         if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
-            repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::Normal);
+            repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::Normal);
         return found ? slot.getValue(globalObject, ident) : jsUndefined();
     }));
 }
@@ -387,7 +387,7 @@
         
         CodeBlock* codeBlock = callFrame->codeBlock();
         if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
-            repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::WithThis);
+            repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::WithThis);
         return found ? slot.getValue(globalObject, ident) : jsUndefined();
     }));
 }
@@ -2022,7 +2022,7 @@
                 LOG_IC((ICEvent::OperationGetByValOptimize, baseValue.classInfoOrNull(vm), propertyName, baseValue == slot.slotBase())); 
                 
                 if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull(), propertyName.impl()))
-                    repatchGetBy(globalObject, codeBlock, baseValue, subscript.asCell(), slot, *stubInfo, GetByKind::NormalByVal);
+                    repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromCell(subscript.asCell()), slot, *stubInfo, GetByKind::NormalByVal);
                 return found ? slot.getValue(globalObject, propertyName) : jsUndefined();
             }));
         }
@@ -2172,13 +2172,13 @@
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
     if (baseValue.isObject()) {
-        const Identifier propertyName = Identifier::fromUid(vm, uid);
+        const Identifier ident = Identifier::fromUid(vm, uid);
         RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
-        if (!parseIndex(propertyName)) {
+        if (!parseIndex(ident)) {
             CodeBlock* codeBlock = callFrame->codeBlock();
             if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
-                repatchDelBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, *stubInfo, DelByKind::Normal);
+                repatchDeleteBy(globalObject, codeBlock, slot, baseValue, oldStructure, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), *stubInfo, DelByKind::Normal);
         }
     }
 
@@ -2240,7 +2240,7 @@
         if (subscript.isSymbol() || !parseIndex(propertyName)) {
             CodeBlock* codeBlock = callFrame->codeBlock();
             if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
-                repatchDelBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, *stubInfo, DelByKind::NormalByVal);
+                repatchDeleteBy(globalObject, codeBlock, slot, baseValue, oldStructure, CacheableIdentifier::createFromCell(subscript.asCell()), *stubInfo, DelByKind::NormalByVal);
         }
     }

Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (257727 => 257728)


--- trunk/Source/_javascript_Core/jit/Repatch.cpp	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp	2020-03-02 20:58:22 UTC (rev 257728)
@@ -592,7 +592,7 @@
                     }
                 }
 
-                newCase = AccessCase::create(vm, codeBlock, AccessCase::Replace, ident, slot.cachedOffset(), oldStructure);
+                newCase = AccessCase::create(vm, codeBlock, AccessCase::Replace, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot.cachedOffset(), oldStructure);
             } else {
                 ASSERT(slot.type() == PutPropertySlot::NewProperty);
 
@@ -641,7 +641,7 @@
                     }
                 }
 
-                newCase = AccessCase::createTransition(vm, codeBlock, ident, offset, oldStructure, newStructure, conditionSet, WTFMove(prototypeAccessChain));
+                newCase = AccessCase::createTransition(vm, codeBlock, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), offset, oldStructure, newStructure, conditionSet, WTFMove(prototypeAccessChain));
             }
         } else if (slot.isCacheableCustom() || slot.isCacheableSetter()) {
             if (slot.isCacheableCustom()) {
@@ -670,7 +670,7 @@
                 }
 
                 newCase = GetterSetterAccessCase::create(
-                    vm, codeBlock, slot.isCustomAccessor() ? AccessCase::CustomAccessorSetter : AccessCase::CustomValueSetter, oldStructure, ident,
+                    vm, codeBlock, slot.isCustomAccessor() ? AccessCase::CustomAccessorSetter : AccessCase::CustomValueSetter, oldStructure, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident),
                     invalidOffset, conditionSet, WTFMove(prototypeAccessChain), slot.customSetter(), slot.base() != baseValue ? slot.base() : nullptr);
             } else {
                 ObjectPropertyConditionSet conditionSet;
@@ -707,13 +707,13 @@
                 }
 
                 newCase = GetterSetterAccessCase::create(
-                    vm, codeBlock, AccessCase::Setter, oldStructure, ident, offset, conditionSet, WTFMove(prototypeAccessChain));
+                    vm, codeBlock, AccessCase::Setter, oldStructure, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), offset, conditionSet, WTFMove(prototypeAccessChain));
             }
         }
 
         LOG_IC((ICEvent::PutByIdAddAccessCase, oldStructure->classInfo(), ident, slot.base() == baseValue));
         
-        result = stubInfo.addAccessCase(locker, codeBlock, ident, WTFMove(newCase));
+        result = stubInfo.addAccessCase(locker, codeBlock, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), WTFMove(newCase));
 
         if (result.generatedSomeCode()) {
             LOG_IC((ICEvent::PutByIdReplaceWithJump, oldStructure->classInfo(), ident, slot.base() == baseValue));
@@ -737,7 +737,7 @@
         ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, appropriateGenericPutByIdFunction(slot, putKind));
 }
 
-static InlineCacheAction tryCacheDelBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, const Identifier& propertyName, StructureStubInfo& stubInfo, DelByKind)
+static InlineCacheAction tryCacheDeleteBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, CacheableIdentifier propertyName, StructureStubInfo& stubInfo, DelByKind)
 {
     VM& vm = globalObject->vm();
     AccessGenerationResult result;
@@ -759,7 +759,7 @@
 
         if (slot.isDeleteHit()) {
             PropertyOffset newOffset = invalidOffset;
-            Structure* newStructure = Structure::removePropertyTransitionFromExistingStructureConcurrently(oldStructure, propertyName, newOffset);
+            Structure* newStructure = Structure::removePropertyTransitionFromExistingStructureConcurrently(oldStructure, propertyName.uid(), newOffset);
             if (!newStructure)
                 return RetryCacheLater;
             if (!newStructure->propertyAccessesAreCacheable() || newStructure->isDictionary())
@@ -769,12 +769,12 @@
             ASSERT(newStructure->isPropertyDeletionTransition());
             ASSERT(newStructure->isObject());
             ASSERT(isValidOffset(newOffset));
-            newCase = AccessCase::createDelete(vm, codeBlock, CacheableIdentifier(propertyName), newOffset, oldStructure, newStructure);
+            newCase = AccessCase::createDelete(vm, codeBlock, propertyName, newOffset, oldStructure, newStructure);
         } else if (!codeBlock->isStrictMode()) {
             if (slot.isNonconfigurable())
-                newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteNonConfigurable, CacheableIdentifier(propertyName), invalidOffset, oldStructure, { }, nullptr);
+                newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteNonConfigurable, propertyName, invalidOffset, oldStructure, { }, nullptr);
             else
-                newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteMiss, CacheableIdentifier(propertyName), invalidOffset, oldStructure, { }, nullptr);
+                newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteMiss, propertyName, invalidOffset, oldStructure, { }, nullptr);
         }
 
         result = stubInfo.addAccessCase(locker, codeBlock, propertyName, WTFMove(newCase));
@@ -781,7 +781,7 @@
 
         if (result.generatedSomeCode()) {
             RELEASE_ASSERT(result.code());
-            LOG_IC((ICEvent::DelByReplaceWithJump, oldStructure->classInfo(), propertyName));
+            LOG_IC((ICEvent::DelByReplaceWithJump, oldStructure->classInfo(), Identifier::fromUid(vm, propertyName.uid())));
             InlineAccess::rewireStubAsJump(stubInfo, CodeLocationLabel<JITStubRoutinePtrTag>(result.code()));
         }
     }
@@ -791,12 +791,13 @@
     return result.shouldGiveUpNow() ? GiveUpOnCache : RetryCacheLater;
 }
 
-void repatchDelBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, const Identifier& propertyName, StructureStubInfo& stubInfo, DelByKind kind)
+void repatchDeleteBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, CacheableIdentifier propertyName, StructureStubInfo& stubInfo, DelByKind kind)
 {
     SuperSamplerScope superSamplerScope(false);
+    VM& vm = globalObject->vm();
 
-    if (tryCacheDelBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, stubInfo, kind) == GiveUpOnCache) {
-        LOG_IC((ICEvent::DelByReplaceWithGeneric, baseValue.classInfoOrNull(globalObject->vm()), propertyName));
+    if (tryCacheDeleteBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, stubInfo, kind) == GiveUpOnCache) {
+        LOG_IC((ICEvent::DelByReplaceWithGeneric, baseValue.classInfoOrNull(globalObject->vm()), Identifier::fromUid(vm, propertyName.uid())));
         if (kind == DelByKind::Normal)
             ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, operationDeleteByIdGeneric);
         else
@@ -891,9 +892,9 @@
         LOG_IC((ICEvent::InAddAccessCase, structure->classInfo(), ident, slot.slotBase() == base));
 
         std::unique_ptr<AccessCase> newCase = AccessCase::create(
-            vm, codeBlock, wasFound ? AccessCase::InHit : AccessCase::InMiss, ident, wasFound ? slot.cachedOffset() : invalidOffset, structure, conditionSet, WTFMove(prototypeAccessChain));
+            vm, codeBlock, wasFound ? AccessCase::InHit : AccessCase::InMiss, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), wasFound ? slot.cachedOffset() : invalidOffset, structure, conditionSet, WTFMove(prototypeAccessChain));
 
-        result = stubInfo.addAccessCase(locker, codeBlock, ident, WTFMove(newCase));
+        result = stubInfo.addAccessCase(locker, codeBlock, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), WTFMove(newCase));
 
         if (result.generatedSomeCode()) {
             LOG_IC((ICEvent::InReplaceWithJump, structure->classInfo(), ident, slot.slotBase() == base));
@@ -957,7 +958,7 @@
         }
         
         if (!newCase)
-            newCase = AccessCase::create(vm, codeBlock, AccessCase::InstanceOfGeneric, Identifier());
+            newCase = AccessCase::create(vm, codeBlock, AccessCase::InstanceOfGeneric, CacheableIdentifier());
         
         LOG_IC((ICEvent::InstanceOfAddAccessCase, structure->classInfo(), Identifier()));

Modified: trunk/Source/_javascript_Core/jit/Repatch.h (257727 => 257728)


--- trunk/Source/_javascript_Core/jit/Repatch.h	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/Source/_javascript_Core/jit/Repatch.h	2020-03-02 20:58:22 UTC (rev 257728)
@@ -49,7 +49,7 @@
 void repatchArrayGetByVal(JSGlobalObject*, CodeBlock*, JSValue base, JSValue index, StructureStubInfo&);
 void repatchGetBy(JSGlobalObject*, CodeBlock*, JSValue, CacheableIdentifier, const PropertySlot&, StructureStubInfo&, GetByKind);
 void repatchPutByID(JSGlobalObject*, CodeBlock*, JSValue, Structure*, const Identifier&, const PutPropertySlot&, StructureStubInfo&, PutKind);
-void repatchDelBy(JSGlobalObject*, CodeBlock*, DeletePropertySlot&, JSValue, Structure*, const Identifier&, StructureStubInfo&, DelByKind);
+void repatchDeleteBy(JSGlobalObject*, CodeBlock*, DeletePropertySlot&, JSValue, Structure*, CacheableIdentifier, StructureStubInfo&, DelByKind);
 void repatchInByID(JSGlobalObject*, CodeBlock*, JSObject*, const Identifier&, bool wasFound, const PropertySlot&, StructureStubInfo&);
 void repatchInstanceOf(JSGlobalObject*, CodeBlock*, JSValue value, JSValue prototype, StructureStubInfo&, bool wasFound);
 void linkFor(VM&, CallFrame*, CallLinkInfo&, CodeBlock*, JSObject* callee, MacroAssemblerCodePtr<JSEntryPtrTag>);

Modified: trunk/Source/_javascript_Core/runtime/CacheableIdentifier.h (257727 => 257728)


--- trunk/Source/_javascript_Core/runtime/CacheableIdentifier.h	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/Source/_javascript_Core/runtime/CacheableIdentifier.h	2020-03-02 20:58:22 UTC (rev 257728)
@@ -46,9 +46,10 @@
 class CacheableIdentifier {
 public:
     CacheableIdentifier() = default;
-    inline CacheableIdentifier(const Identifier&);
-    inline CacheableIdentifier(JSCell* identifier);
 
+    static inline CacheableIdentifier createFromCell(JSCell* identifier);
+    static inline CacheableIdentifier createFromIdentifierOwnedByCodeBlock(const Identifier&);
+
     CacheableIdentifier(const CacheableIdentifier&) = default;
     CacheableIdentifier(CacheableIdentifier&&) = default;
 
@@ -83,6 +84,9 @@
     JS_EXPORT_PRIVATE void dump(PrintStream&) const;
 
 private:
+    explicit inline CacheableIdentifier(const Identifier&);
+    explicit inline CacheableIdentifier(JSCell* identifier);
+
     inline void setCellBits(JSCell*);
     inline void setUidBits(UniquedStringImpl*);

Modified: trunk/Source/_javascript_Core/runtime/CacheableIdentifierInlines.h (257727 => 257728)


--- trunk/Source/_javascript_Core/runtime/CacheableIdentifierInlines.h	2020-03-02 20:45:16 UTC (rev 257727)
+++ trunk/Source/_javascript_Core/runtime/CacheableIdentifierInlines.h	2020-03-02 20:58:22 UTC (rev 257728)
@@ -35,6 +35,16 @@
 
 namespace JSC {
 
+inline CacheableIdentifier CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(const Identifier& i)
+{
+    return CacheableIdentifier(i);
+}
+
+inline CacheableIdentifier CacheableIdentifier::createFromCell(JSCell* i)
+{
+    return CacheableIdentifier(i);
+}
+
 inline CacheableIdentifier::CacheableIdentifier(const Identifier& identifier)
 {
     setUidBits(identifier.impl());

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to