Changeset 257728 in webkit


Ignore:
Timestamp:
Mar 2, 2020 12:58:22 PM (4 years ago)
Author:
Justin Michaud
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/JavaScriptCore:

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):

Location:
trunk
Files:
1 added
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r257605 r257728  
     12020-03-02  Justin Michaud  <justin_michaud@apple.com>
     2
     3        Delete by val caching does not keep the subscript alive
     4        https://bugs.webkit.org/show_bug.cgi?id=208393
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        * stress/delete-property-ic-stress.js: Added.
     9
    1102020-02-27  Justin Michaud  <justin_michaud@apple.com>
    211
  • trunk/Source/JavaScriptCore/ChangeLog

    r257721 r257728  
     12020-03-02  Justin Michaud  <justin_michaud@apple.com>
     2
     3        Delete by val caching does not keep the subscript alive
     4        https://bugs.webkit.org/show_bug.cgi?id=208393
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        Before, the provided test case crashed with asan because we did not keep deleteByVal
     9        subscripts alive. This patch changes CacheableIdentifier to make this mistake harder
     10        to make again, by making the constructor calls more explicit when CacheableIdentifier
     11        will not keep an Identifier alive.
     12
     13        * jit/JITOperations.cpp:
     14        * jit/Repatch.cpp:
     15        (JSC::tryCachePutByID):
     16        (JSC::tryCacheDeleteBy):
     17        (JSC::repatchDeleteBy):
     18        (JSC::tryCacheInByID):
     19        (JSC::tryCacheInstanceOf):
     20        (JSC::tryCacheDelBy): Deleted.
     21        (JSC::repatchDelBy): Deleted.
     22        * jit/Repatch.h:
     23        * runtime/CacheableIdentifier.h:
     24        * runtime/CacheableIdentifierInlines.h:
     25        (JSC::CacheableIdentifier::createFromIdentifierOwnedByCodeBlock):
     26        (JSC::CacheableIdentifier::createFromCell):
     27
    1282020-03-02  Paulo Matos  <pmatos@igalia.com>
    229
  • trunk/Source/JavaScriptCore/jit/JITOperations.cpp

    r257399 r257728  
    215215    CodeBlock* codeBlock = callFrame->codeBlock();
    216216    if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()) && !slot.isTaintedByOpaqueObject() && (slot.isCacheableValue() || slot.isCacheableGetter() || slot.isUnset()))
    217         repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::Try);
     217        repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::Try);
    218218
    219219    return JSValue::encode(slot.getPureResult());
     
    271271    CodeBlock* codeBlock = callFrame->codeBlock();
    272272    if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
    273         repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::Direct);
     273        repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::Direct);
    274274
    275275    RELEASE_AND_RETURN(scope, JSValue::encode(found ? slot.getValue(globalObject, ident) : jsUndefined()));
     
    331331        CodeBlock* codeBlock = callFrame->codeBlock();
    332332        if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
    333             repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::Normal);
     333            repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::Normal);
    334334        return found ? slot.getValue(globalObject, ident) : jsUndefined();
    335335    }));
     
    388388        CodeBlock* codeBlock = callFrame->codeBlock();
    389389        if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
    390             repatchGetBy(globalObject, codeBlock, baseValue, ident, slot, *stubInfo, GetByKind::WithThis);
     390            repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot, *stubInfo, GetByKind::WithThis);
    391391        return found ? slot.getValue(globalObject, ident) : jsUndefined();
    392392    }));
     
    20232023               
    20242024                if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull(), propertyName.impl()))
    2025                     repatchGetBy(globalObject, codeBlock, baseValue, subscript.asCell(), slot, *stubInfo, GetByKind::NormalByVal);
     2025                    repatchGetBy(globalObject, codeBlock, baseValue, CacheableIdentifier::createFromCell(subscript.asCell()), slot, *stubInfo, GetByKind::NormalByVal);
    20262026                return found ? slot.getValue(globalObject, propertyName) : jsUndefined();
    20272027            }));
     
    21732173
    21742174    if (baseValue.isObject()) {
    2175         const Identifier propertyName = Identifier::fromUid(vm, uid);
     2175        const Identifier ident = Identifier::fromUid(vm, uid);
    21762176        RETURN_IF_EXCEPTION(scope, encodedJSValue());
    21772177
    2178         if (!parseIndex(propertyName)) {
     2178        if (!parseIndex(ident)) {
    21792179            CodeBlock* codeBlock = callFrame->codeBlock();
    21802180            if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
    2181                 repatchDelBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, *stubInfo, DelByKind::Normal);
     2181                repatchDeleteBy(globalObject, codeBlock, slot, baseValue, oldStructure, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), *stubInfo, DelByKind::Normal);
    21822182        }
    21832183    }
     
    22412241            CodeBlock* codeBlock = callFrame->codeBlock();
    22422242            if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))
    2243                 repatchDelBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, *stubInfo, DelByKind::NormalByVal);
     2243                repatchDeleteBy(globalObject, codeBlock, slot, baseValue, oldStructure, CacheableIdentifier::createFromCell(subscript.asCell()), *stubInfo, DelByKind::NormalByVal);
    22442244        }
    22452245    }
  • trunk/Source/JavaScriptCore/jit/Repatch.cpp

    r257605 r257728  
    593593                }
    594594
    595                 newCase = AccessCase::create(vm, codeBlock, AccessCase::Replace, ident, slot.cachedOffset(), oldStructure);
     595                newCase = AccessCase::create(vm, codeBlock, AccessCase::Replace, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), slot.cachedOffset(), oldStructure);
    596596            } else {
    597597                ASSERT(slot.type() == PutPropertySlot::NewProperty);
     
    642642                }
    643643
    644                 newCase = AccessCase::createTransition(vm, codeBlock, ident, offset, oldStructure, newStructure, conditionSet, WTFMove(prototypeAccessChain));
     644                newCase = AccessCase::createTransition(vm, codeBlock, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), offset, oldStructure, newStructure, conditionSet, WTFMove(prototypeAccessChain));
    645645            }
    646646        } else if (slot.isCacheableCustom() || slot.isCacheableSetter()) {
     
    671671
    672672                newCase = GetterSetterAccessCase::create(
    673                     vm, codeBlock, slot.isCustomAccessor() ? AccessCase::CustomAccessorSetter : AccessCase::CustomValueSetter, oldStructure, ident,
     673                    vm, codeBlock, slot.isCustomAccessor() ? AccessCase::CustomAccessorSetter : AccessCase::CustomValueSetter, oldStructure, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident),
    674674                    invalidOffset, conditionSet, WTFMove(prototypeAccessChain), slot.customSetter(), slot.base() != baseValue ? slot.base() : nullptr);
    675675            } else {
     
    708708
    709709                newCase = GetterSetterAccessCase::create(
    710                     vm, codeBlock, AccessCase::Setter, oldStructure, ident, offset, conditionSet, WTFMove(prototypeAccessChain));
     710                    vm, codeBlock, AccessCase::Setter, oldStructure, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), offset, conditionSet, WTFMove(prototypeAccessChain));
    711711            }
    712712        }
     
    714714        LOG_IC((ICEvent::PutByIdAddAccessCase, oldStructure->classInfo(), ident, slot.base() == baseValue));
    715715       
    716         result = stubInfo.addAccessCase(locker, codeBlock, ident, WTFMove(newCase));
     716        result = stubInfo.addAccessCase(locker, codeBlock, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), WTFMove(newCase));
    717717
    718718        if (result.generatedSomeCode()) {
     
    738738}
    739739
    740 static InlineCacheAction tryCacheDelBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, const Identifier& propertyName, StructureStubInfo& stubInfo, DelByKind)
     740static InlineCacheAction tryCacheDeleteBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, CacheableIdentifier propertyName, StructureStubInfo& stubInfo, DelByKind)
    741741{
    742742    VM& vm = globalObject->vm();
     
    760760        if (slot.isDeleteHit()) {
    761761            PropertyOffset newOffset = invalidOffset;
    762             Structure* newStructure = Structure::removePropertyTransitionFromExistingStructureConcurrently(oldStructure, propertyName, newOffset);
     762            Structure* newStructure = Structure::removePropertyTransitionFromExistingStructureConcurrently(oldStructure, propertyName.uid(), newOffset);
    763763            if (!newStructure)
    764764                return RetryCacheLater;
     
    770770            ASSERT(newStructure->isObject());
    771771            ASSERT(isValidOffset(newOffset));
    772             newCase = AccessCase::createDelete(vm, codeBlock, CacheableIdentifier(propertyName), newOffset, oldStructure, newStructure);
     772            newCase = AccessCase::createDelete(vm, codeBlock, propertyName, newOffset, oldStructure, newStructure);
    773773        } else if (!codeBlock->isStrictMode()) {
    774774            if (slot.isNonconfigurable())
    775                 newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteNonConfigurable, CacheableIdentifier(propertyName), invalidOffset, oldStructure, { }, nullptr);
     775                newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteNonConfigurable, propertyName, invalidOffset, oldStructure, { }, nullptr);
    776776            else
    777                 newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteMiss, CacheableIdentifier(propertyName), invalidOffset, oldStructure, { }, nullptr);
     777                newCase = AccessCase::create(vm, codeBlock, AccessCase::DeleteMiss, propertyName, invalidOffset, oldStructure, { }, nullptr);
    778778        }
    779779
     
    782782        if (result.generatedSomeCode()) {
    783783            RELEASE_ASSERT(result.code());
    784             LOG_IC((ICEvent::DelByReplaceWithJump, oldStructure->classInfo(), propertyName));
     784            LOG_IC((ICEvent::DelByReplaceWithJump, oldStructure->classInfo(), Identifier::fromUid(vm, propertyName.uid())));
    785785            InlineAccess::rewireStubAsJump(stubInfo, CodeLocationLabel<JITStubRoutinePtrTag>(result.code()));
    786786        }
     
    792792}
    793793
    794 void repatchDelBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, const Identifier& propertyName, StructureStubInfo& stubInfo, DelByKind kind)
     794void repatchDeleteBy(JSGlobalObject* globalObject, CodeBlock* codeBlock, DeletePropertySlot& slot, JSValue baseValue, Structure* oldStructure, CacheableIdentifier propertyName, StructureStubInfo& stubInfo, DelByKind kind)
    795795{
    796796    SuperSamplerScope superSamplerScope(false);
    797 
    798     if (tryCacheDelBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, stubInfo, kind) == GiveUpOnCache) {
    799         LOG_IC((ICEvent::DelByReplaceWithGeneric, baseValue.classInfoOrNull(globalObject->vm()), propertyName));
     797    VM& vm = globalObject->vm();
     798
     799    if (tryCacheDeleteBy(globalObject, codeBlock, slot, baseValue, oldStructure, propertyName, stubInfo, kind) == GiveUpOnCache) {
     800        LOG_IC((ICEvent::DelByReplaceWithGeneric, baseValue.classInfoOrNull(globalObject->vm()), Identifier::fromUid(vm, propertyName.uid())));
    800801        if (kind == DelByKind::Normal)
    801802            ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation, operationDeleteByIdGeneric);
     
    892893
    893894        std::unique_ptr<AccessCase> newCase = AccessCase::create(
    894             vm, codeBlock, wasFound ? AccessCase::InHit : AccessCase::InMiss, ident, wasFound ? slot.cachedOffset() : invalidOffset, structure, conditionSet, WTFMove(prototypeAccessChain));
    895 
    896         result = stubInfo.addAccessCase(locker, codeBlock, ident, WTFMove(newCase));
     895            vm, codeBlock, wasFound ? AccessCase::InHit : AccessCase::InMiss, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), wasFound ? slot.cachedOffset() : invalidOffset, structure, conditionSet, WTFMove(prototypeAccessChain));
     896
     897        result = stubInfo.addAccessCase(locker, codeBlock, CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(ident), WTFMove(newCase));
    897898
    898899        if (result.generatedSomeCode()) {
     
    958959       
    959960        if (!newCase)
    960             newCase = AccessCase::create(vm, codeBlock, AccessCase::InstanceOfGeneric, Identifier());
     961            newCase = AccessCase::create(vm, codeBlock, AccessCase::InstanceOfGeneric, CacheableIdentifier());
    961962       
    962963        LOG_IC((ICEvent::InstanceOfAddAccessCase, structure->classInfo(), Identifier()));
  • trunk/Source/JavaScriptCore/jit/Repatch.h

    r257399 r257728  
    5050void repatchGetBy(JSGlobalObject*, CodeBlock*, JSValue, CacheableIdentifier, const PropertySlot&, StructureStubInfo&, GetByKind);
    5151void repatchPutByID(JSGlobalObject*, CodeBlock*, JSValue, Structure*, const Identifier&, const PutPropertySlot&, StructureStubInfo&, PutKind);
    52 void repatchDelBy(JSGlobalObject*, CodeBlock*, DeletePropertySlot&, JSValue, Structure*, const Identifier&, StructureStubInfo&, DelByKind);
     52void repatchDeleteBy(JSGlobalObject*, CodeBlock*, DeletePropertySlot&, JSValue, Structure*, CacheableIdentifier, StructureStubInfo&, DelByKind);
    5353void repatchInByID(JSGlobalObject*, CodeBlock*, JSObject*, const Identifier&, bool wasFound, const PropertySlot&, StructureStubInfo&);
    5454void repatchInstanceOf(JSGlobalObject*, CodeBlock*, JSValue value, JSValue prototype, StructureStubInfo&, bool wasFound);
  • trunk/Source/JavaScriptCore/runtime/CacheableIdentifier.h

    r254464 r257728  
    4747public:
    4848    CacheableIdentifier() = default;
    49     inline CacheableIdentifier(const Identifier&);
    50     inline CacheableIdentifier(JSCell* identifier);
     49
     50    static inline CacheableIdentifier createFromCell(JSCell* identifier);
     51    static inline CacheableIdentifier createFromIdentifierOwnedByCodeBlock(const Identifier&);
    5152
    5253    CacheableIdentifier(const CacheableIdentifier&) = default;
     
    8485
    8586private:
     87    explicit inline CacheableIdentifier(const Identifier&);
     88    explicit inline CacheableIdentifier(JSCell* identifier);
     89
    8690    inline void setCellBits(JSCell*);
    8791    inline void setUidBits(UniquedStringImpl*);
  • trunk/Source/JavaScriptCore/runtime/CacheableIdentifierInlines.h

    r257399 r257728  
    3535
    3636namespace JSC {
     37
     38inline CacheableIdentifier CacheableIdentifier::createFromIdentifierOwnedByCodeBlock(const Identifier& i)
     39{
     40    return CacheableIdentifier(i);
     41}
     42
     43inline CacheableIdentifier CacheableIdentifier::createFromCell(JSCell* i)
     44{
     45    return CacheableIdentifier(i);
     46}
    3747
    3848inline CacheableIdentifier::CacheableIdentifier(const Identifier& identifier)
Note: See TracChangeset for help on using the changeset viewer.