Make it clear that regenerating ICs are holding the CodeBlock's lock by passing the...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Jun 2017 18:42:44 +0000 (18:42 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Jun 2017 18:42:44 +0000 (18:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173609

Reviewed by Keith Miller.

This patch makes many of the IC generating functions require a locker as
a parameter. We do this in other places in JSC to indicate that
a particular API is only valid while a particular lock is held.
This is the case when generating ICs. This patch just makes it
explicit in the IC generating interface.

* bytecode/PolymorphicAccess.cpp:
(JSC::PolymorphicAccess::addCases):
(JSC::PolymorphicAccess::addCase):
(JSC::PolymorphicAccess::commit):
(JSC::PolymorphicAccess::regenerate):
* bytecode/PolymorphicAccess.h:
* bytecode/StructureStubInfo.cpp:
(JSC::StructureStubInfo::addAccessCase):
(JSC::StructureStubInfo::initStub): Deleted.
* bytecode/StructureStubInfo.h:
* jit/Repatch.cpp:
(JSC::tryCacheGetByID):
(JSC::repatchGetByID):
(JSC::tryCachePutByID):
(JSC::repatchPutByID):
(JSC::tryRepatchIn):
(JSC::repatchIn):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@218641 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
Source/JavaScriptCore/bytecode/PolymorphicAccess.h
Source/JavaScriptCore/bytecode/StructureStubInfo.cpp
Source/JavaScriptCore/bytecode/StructureStubInfo.h
Source/JavaScriptCore/jit/Repatch.cpp

index 8580356..ac7ef62 100644 (file)
@@ -1,3 +1,34 @@
+2017-06-21  Saam Barati  <sbarati@apple.com>
+
+        Make it clear that regenerating ICs are holding the CodeBlock's lock by passing the locker as a parameter
+        https://bugs.webkit.org/show_bug.cgi?id=173609
+
+        Reviewed by Keith Miller.
+
+        This patch makes many of the IC generating functions require a locker as
+        a parameter. We do this in other places in JSC to indicate that
+        a particular API is only valid while a particular lock is held.
+        This is the case when generating ICs. This patch just makes it
+        explicit in the IC generating interface.
+
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::PolymorphicAccess::addCases):
+        (JSC::PolymorphicAccess::addCase):
+        (JSC::PolymorphicAccess::commit):
+        (JSC::PolymorphicAccess::regenerate):
+        * bytecode/PolymorphicAccess.h:
+        * bytecode/StructureStubInfo.cpp:
+        (JSC::StructureStubInfo::addAccessCase):
+        (JSC::StructureStubInfo::initStub): Deleted.
+        * bytecode/StructureStubInfo.h:
+        * jit/Repatch.cpp:
+        (JSC::tryCacheGetByID):
+        (JSC::repatchGetByID):
+        (JSC::tryCachePutByID):
+        (JSC::repatchPutByID):
+        (JSC::tryRepatchIn):
+        (JSC::repatchIn):
+
 2017-06-20  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Disable font variations on macOS Sierra and iOS 10
 2017-06-20  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Disable font variations on macOS Sierra and iOS 10
index 9a255c0..d2d89d1 100644 (file)
@@ -210,8 +210,8 @@ PolymorphicAccess::PolymorphicAccess() { }
 PolymorphicAccess::~PolymorphicAccess() { }
 
 AccessGenerationResult PolymorphicAccess::addCases(
 PolymorphicAccess::~PolymorphicAccess() { }
 
 AccessGenerationResult PolymorphicAccess::addCases(
-    VM& vm, CodeBlock* codeBlock, StructureStubInfo& stubInfo, const Identifier& ident,
-    Vector<std::unique_ptr<AccessCase>, 2> originalCasesToAdd)
+    const GCSafeConcurrentJSLocker& locker, VM& vm, CodeBlock* codeBlock, StructureStubInfo& stubInfo,
+    const Identifier& ident, Vector<std::unique_ptr<AccessCase>, 2> originalCasesToAdd)
 {
     SuperSamplerScope superSamplerScope(false);
     
 {
     SuperSamplerScope superSamplerScope(false);
     
@@ -258,7 +258,7 @@ AccessGenerationResult PolymorphicAccess::addCases(
     // Now add things to the new list. Note that at this point, we will still have old cases that
     // may be replaced by the new ones. That's fine. We will sort that out when we regenerate.
     for (auto& caseToAdd : casesToAdd) {
     // Now add things to the new list. Note that at this point, we will still have old cases that
     // may be replaced by the new ones. That's fine. We will sort that out when we regenerate.
     for (auto& caseToAdd : casesToAdd) {
-        commit(vm, m_watchpoints, codeBlock, stubInfo, ident, *caseToAdd);
+        commit(locker, vm, m_watchpoints, codeBlock, stubInfo, ident, *caseToAdd);
         m_list.append(WTFMove(caseToAdd));
     }
     
         m_list.append(WTFMove(caseToAdd));
     }
     
@@ -269,12 +269,12 @@ AccessGenerationResult PolymorphicAccess::addCases(
 }
 
 AccessGenerationResult PolymorphicAccess::addCase(
 }
 
 AccessGenerationResult PolymorphicAccess::addCase(
-    VM& vm, CodeBlock* codeBlock, StructureStubInfo& stubInfo, const Identifier& ident,
-    std::unique_ptr<AccessCase> newAccess)
+    const GCSafeConcurrentJSLocker& locker, VM& vm, CodeBlock* codeBlock, StructureStubInfo& stubInfo,
+    const Identifier& ident, std::unique_ptr<AccessCase> newAccess)
 {
     Vector<std::unique_ptr<AccessCase>, 2> newAccesses;
     newAccesses.append(WTFMove(newAccess));
 {
     Vector<std::unique_ptr<AccessCase>, 2> newAccesses;
     newAccesses.append(WTFMove(newAccess));
-    return addCases(vm, codeBlock, stubInfo, ident, WTFMove(newAccesses));
+    return addCases(locker, vm, codeBlock, stubInfo, ident, WTFMove(newAccesses));
 }
 
 bool PolymorphicAccess::visitWeak(VM& vm) const
 }
 
 bool PolymorphicAccess::visitWeak(VM& vm) const
@@ -310,7 +310,7 @@ void PolymorphicAccess::dump(PrintStream& out) const
 }
 
 void PolymorphicAccess::commit(
 }
 
 void PolymorphicAccess::commit(
-    VM& vm, std::unique_ptr<WatchpointsOnStructureStubInfo>& watchpoints, CodeBlock* codeBlock,
+    const GCSafeConcurrentJSLocker&, VM& vm, std::unique_ptr<WatchpointsOnStructureStubInfo>& watchpoints, CodeBlock* codeBlock,
     StructureStubInfo& stubInfo, const Identifier& ident, AccessCase& accessCase)
 {
     // NOTE: We currently assume that this is relatively rare. It mainly arises for accesses to
     StructureStubInfo& stubInfo, const Identifier& ident, AccessCase& accessCase)
 {
     // NOTE: We currently assume that this is relatively rare. It mainly arises for accesses to
@@ -329,7 +329,7 @@ void PolymorphicAccess::commit(
 }
 
 AccessGenerationResult PolymorphicAccess::regenerate(
 }
 
 AccessGenerationResult PolymorphicAccess::regenerate(
-    VM& vm, CodeBlock* codeBlock, StructureStubInfo& stubInfo, const Identifier& ident)
+    const GCSafeConcurrentJSLocker& locker, VM& vm, CodeBlock* codeBlock, StructureStubInfo& stubInfo, const Identifier& ident)
 {
     SuperSamplerScope superSamplerScope(false);
     
 {
     SuperSamplerScope superSamplerScope(false);
     
@@ -410,7 +410,7 @@ AccessGenerationResult PolymorphicAccess::regenerate(
     bool allGuardedByStructureCheck = true;
     bool hasJSGetterSetterCall = false;
     for (auto& newCase : cases) {
     bool allGuardedByStructureCheck = true;
     bool hasJSGetterSetterCall = false;
     for (auto& newCase : cases) {
-        commit(vm, state.watchpoints, codeBlock, stubInfo, ident, *newCase);
+        commit(locker, vm, state.watchpoints, codeBlock, stubInfo, ident, *newCase);
         allGuardedByStructureCheck &= newCase->guardedByStructureCheck();
         if (newCase->type() == AccessCase::Getter || newCase->type() == AccessCase::Setter)
             hasJSGetterSetterCall = true;
         allGuardedByStructureCheck &= newCase->guardedByStructureCheck();
         if (newCase->type() == AccessCase::Getter || newCase->type() == AccessCase::Setter)
             hasJSGetterSetterCall = true;
index a246d76..43128dd 100644 (file)
@@ -125,12 +125,12 @@ public:
     // When this fails (returns GaveUp), this will leave the old stub intact but you should not try
     // to call this method again for that PolymorphicAccess instance.
     AccessGenerationResult addCases(
     // When this fails (returns GaveUp), this will leave the old stub intact but you should not try
     // to call this method again for that PolymorphicAccess instance.
     AccessGenerationResult addCases(
-        VM&, CodeBlock*, StructureStubInfo&, const Identifier&, Vector<std::unique_ptr<AccessCase>, 2>);
+        const GCSafeConcurrentJSLocker&, VM&, CodeBlock*, StructureStubInfo&, const Identifier&, Vector<std::unique_ptr<AccessCase>, 2>);
 
     AccessGenerationResult addCase(
 
     AccessGenerationResult addCase(
-        VM&, CodeBlock*, StructureStubInfo&, const Identifier&, std::unique_ptr<AccessCase>);
+        const GCSafeConcurrentJSLocker&, VM&, CodeBlock*, StructureStubInfo&, const Identifier&, std::unique_ptr<AccessCase>);
     
     
-    AccessGenerationResult regenerate(VM&, CodeBlock*, StructureStubInfo&, const Identifier&);
+    AccessGenerationResult regenerate(const GCSafeConcurrentJSLocker&, VM&, CodeBlock*, StructureStubInfo&, const Identifier&);
     
     bool isEmpty() const { return m_list.isEmpty(); }
     unsigned size() const { return m_list.size(); }
     
     bool isEmpty() const { return m_list.isEmpty(); }
     unsigned size() const { return m_list.size(); }
@@ -164,12 +164,9 @@ private:
     typedef Vector<std::unique_ptr<AccessCase>, 2> ListType;
     
     void commit(
     typedef Vector<std::unique_ptr<AccessCase>, 2> ListType;
     
     void commit(
-        VM&, std::unique_ptr<WatchpointsOnStructureStubInfo>&, CodeBlock*, StructureStubInfo&,
+        const GCSafeConcurrentJSLocker&, VM&, std::unique_ptr<WatchpointsOnStructureStubInfo>&, CodeBlock*, StructureStubInfo&,
         const Identifier&, AccessCase&);
 
         const Identifier&, AccessCase&);
 
-    MacroAssemblerCodePtr regenerate(
-        VM&, CodeBlock*, StructureStubInfo&, const Identifier&, ListType& cases);
-
     ListType m_list;
     RefPtr<JITStubRoutine> m_stubRoutine;
     std::unique_ptr<WatchpointsOnStructureStubInfo> m_watchpoints;
     ListType m_list;
     RefPtr<JITStubRoutine> m_stubRoutine;
     std::unique_ptr<WatchpointsOnStructureStubInfo> m_watchpoints;
index 1db13ac..2187751 100644 (file)
@@ -78,12 +78,6 @@ void StructureStubInfo::initPutByIdReplace(CodeBlock* codeBlock, Structure* base
     u.byIdSelf.offset = offset;
 }
 
     u.byIdSelf.offset = offset;
 }
 
-void StructureStubInfo::initStub(CodeBlock*, std::unique_ptr<PolymorphicAccess> stub)
-{
-    cacheType = CacheType::Stub;
-    u.stub = stub.release();
-}
-
 void StructureStubInfo::deref()
 {
     switch (cacheType) {
 void StructureStubInfo::deref()
 {
     switch (cacheType) {
@@ -117,7 +111,7 @@ void StructureStubInfo::aboutToDie()
 }
 
 AccessGenerationResult StructureStubInfo::addAccessCase(
 }
 
 AccessGenerationResult StructureStubInfo::addAccessCase(
-    CodeBlock* codeBlock, const Identifier& ident, std::unique_ptr<AccessCase> accessCase)
+    const GCSafeConcurrentJSLocker& locker, CodeBlock* codeBlock, const Identifier& ident, std::unique_ptr<AccessCase> accessCase)
 {
     VM& vm = *codeBlock->vm();
     
 {
     VM& vm = *codeBlock->vm();
     
@@ -130,7 +124,7 @@ AccessGenerationResult StructureStubInfo::addAccessCase(
     AccessGenerationResult result;
     
     if (cacheType == CacheType::Stub) {
     AccessGenerationResult result;
     
     if (cacheType == CacheType::Stub) {
-        result = u.stub->addCase(vm, codeBlock, *this, ident, WTFMove(accessCase));
+        result = u.stub->addCase(locker, vm, codeBlock, *this, ident, WTFMove(accessCase));
         
         if (verbose)
             dataLog("Had stub, result: ", result, "\n");
         
         if (verbose)
             dataLog("Had stub, result: ", result, "\n");
@@ -151,7 +145,7 @@ AccessGenerationResult StructureStubInfo::addAccessCase(
         
         accessCases.append(WTFMove(accessCase));
         
         
         accessCases.append(WTFMove(accessCase));
         
-        result = access->addCases(vm, codeBlock, *this, ident, WTFMove(accessCases));
+        result = access->addCases(locker, vm, codeBlock, *this, ident, WTFMove(accessCases));
         
         if (verbose)
             dataLog("Created stub, result: ", result, "\n");
         
         if (verbose)
             dataLog("Created stub, result: ", result, "\n");
@@ -161,7 +155,8 @@ AccessGenerationResult StructureStubInfo::addAccessCase(
             return result;
         }
         
             return result;
         }
         
-        initStub(codeBlock, WTFMove(access));
+        cacheType = CacheType::Stub;
+        u.stub = access.release();
     }
     
     RELEASE_ASSERT(!result.generatedSomeCode());
     }
     
     RELEASE_ASSERT(!result.generatedSomeCode());
@@ -186,7 +181,7 @@ AccessGenerationResult StructureStubInfo::addAccessCase(
     // PolymorphicAccess.
     bufferedStructures.clear();
     
     // PolymorphicAccess.
     bufferedStructures.clear();
     
-    result = u.stub->regenerate(vm, codeBlock, *this, ident);
+    result = u.stub->regenerate(locker, vm, codeBlock, *this, ident);
     
     if (verbose)
         dataLog("Regeneration result: ", result, "\n");
     
     if (verbose)
         dataLog("Regeneration result: ", result, "\n");
index 9a5121d..f529dc3 100644 (file)
@@ -71,9 +71,8 @@ public:
     void initGetByIdSelf(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
     void initArrayLength();
     void initPutByIdReplace(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
     void initGetByIdSelf(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
     void initArrayLength();
     void initPutByIdReplace(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
-    void initStub(CodeBlock*, std::unique_ptr<PolymorphicAccess>);
 
 
-    AccessGenerationResult addAccessCase(CodeBlock*, const Identifier&, std::unique_ptr<AccessCase>);
+    AccessGenerationResult addAccessCase(const GCSafeConcurrentJSLocker&, CodeBlock*, const Identifier&, std::unique_ptr<AccessCase>);
 
     void reset(CodeBlock*);
 
 
     void reset(CodeBlock*);
 
index 66101ff..f35a973 100644 (file)
@@ -157,7 +157,7 @@ inline FunctionPtr appropriateGenericGetByIdFunction(GetByIDKind kind)
     return operationTryGetById;
 }
 
     return operationTryGetById;
 }
 
-static InlineCacheAction tryCacheGetByID(ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PropertySlot& slot, StructureStubInfo& stubInfo, GetByIDKind kind)
+static InlineCacheAction tryCacheGetByID(const GCSafeConcurrentJSLocker& locker, ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PropertySlot& slot, StructureStubInfo& stubInfo, GetByIDKind kind)
 {
     if (forceICFailure(exec))
         return GiveUpOnCache;
 {
     if (forceICFailure(exec))
         return GiveUpOnCache;
@@ -323,7 +323,7 @@ static InlineCacheAction tryCacheGetByID(ExecState* exec, JSValue baseValue, con
 
     LOG_IC((ICEvent::GetByIdAddAccessCase, baseValue.classInfoOrNull(vm), propertyName));
 
 
     LOG_IC((ICEvent::GetByIdAddAccessCase, baseValue.classInfoOrNull(vm), propertyName));
 
-    AccessGenerationResult result = stubInfo.addAccessCase(codeBlock, propertyName, WTFMove(newCase));
+    AccessGenerationResult result = stubInfo.addAccessCase(locker, codeBlock, propertyName, WTFMove(newCase));
 
     if (result.generatedSomeCode()) {
         LOG_IC((ICEvent::GetByIdReplaceWithJump, baseValue.classInfoOrNull(vm), propertyName));
 
     if (result.generatedSomeCode()) {
         LOG_IC((ICEvent::GetByIdReplaceWithJump, baseValue.classInfoOrNull(vm), propertyName));
@@ -340,7 +340,7 @@ void repatchGetByID(ExecState* exec, JSValue baseValue, const Identifier& proper
     SuperSamplerScope superSamplerScope(false);
     GCSafeConcurrentJSLocker locker(exec->codeBlock()->m_lock, exec->vm().heap);
     
     SuperSamplerScope superSamplerScope(false);
     GCSafeConcurrentJSLocker locker(exec->codeBlock()->m_lock, exec->vm().heap);
     
-    if (tryCacheGetByID(exec, baseValue, propertyName, slot, stubInfo, kind) == GiveUpOnCache)
+    if (tryCacheGetByID(locker, exec, baseValue, propertyName, slot, stubInfo, kind) == GiveUpOnCache)
         ftlThunkAwareRepatchCall(exec->codeBlock(), stubInfo.slowPathCallLocation(), appropriateGenericGetByIdFunction(kind));
 }
 
         ftlThunkAwareRepatchCall(exec->codeBlock(), stubInfo.slowPathCallLocation(), appropriateGenericGetByIdFunction(kind));
 }
 
@@ -368,7 +368,7 @@ static V_JITOperation_ESsiJJI appropriateOptimizingPutByIdFunction(const PutProp
     return operationPutByIdNonStrictOptimize;
 }
 
     return operationPutByIdNonStrictOptimize;
 }
 
-static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Structure* structure, const Identifier& ident, const PutPropertySlot& slot, StructureStubInfo& stubInfo, PutKind putKind)
+static InlineCacheAction tryCachePutByID(const GCSafeConcurrentJSLocker& locker, ExecState* exec, JSValue baseValue, Structure* structure, const Identifier& ident, const PutPropertySlot& slot, StructureStubInfo& stubInfo, PutKind putKind)
 {
     if (forceICFailure(exec))
         return GiveUpOnCache;
 {
     if (forceICFailure(exec))
         return GiveUpOnCache;
@@ -476,7 +476,7 @@ static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Str
 
     LOG_IC((ICEvent::PutByIdAddAccessCase, structure->classInfo(), ident));
     
 
     LOG_IC((ICEvent::PutByIdAddAccessCase, structure->classInfo(), ident));
     
-    AccessGenerationResult result = stubInfo.addAccessCase(codeBlock, ident, WTFMove(newCase));
+    AccessGenerationResult result = stubInfo.addAccessCase(locker, codeBlock, ident, WTFMove(newCase));
     
     if (result.generatedSomeCode()) {
         LOG_IC((ICEvent::PutByIdReplaceWithJump, structure->classInfo(), ident));
     
     if (result.generatedSomeCode()) {
         LOG_IC((ICEvent::PutByIdReplaceWithJump, structure->classInfo(), ident));
@@ -494,13 +494,13 @@ void repatchPutByID(ExecState* exec, JSValue baseValue, Structure* structure, co
     SuperSamplerScope superSamplerScope(false);
     GCSafeConcurrentJSLocker locker(exec->codeBlock()->m_lock, exec->vm().heap);
     
     SuperSamplerScope superSamplerScope(false);
     GCSafeConcurrentJSLocker locker(exec->codeBlock()->m_lock, exec->vm().heap);
     
-    if (tryCachePutByID(exec, baseValue, structure, propertyName, slot, stubInfo, putKind) == GiveUpOnCache)
+    if (tryCachePutByID(locker, exec, baseValue, structure, propertyName, slot, stubInfo, putKind) == GiveUpOnCache)
         ftlThunkAwareRepatchCall(exec->codeBlock(), stubInfo.slowPathCallLocation(), appropriateGenericPutByIdFunction(slot, putKind));
 }
 
 static InlineCacheAction tryRepatchIn(
         ftlThunkAwareRepatchCall(exec->codeBlock(), stubInfo.slowPathCallLocation(), appropriateGenericPutByIdFunction(slot, putKind));
 }
 
 static InlineCacheAction tryRepatchIn(
-    ExecState* exec, JSCell* base, const Identifier& ident, bool wasFound,
-    const PropertySlot& slot, StructureStubInfo& stubInfo)
+    const GCSafeConcurrentJSLocker& locker, ExecState* exec, JSCell* base, const Identifier& ident,
+    bool wasFound, const PropertySlot& slot, StructureStubInfo& stubInfo)
 {
     if (forceICFailure(exec))
         return GiveUpOnCache;
 {
     if (forceICFailure(exec))
         return GiveUpOnCache;
@@ -535,7 +535,7 @@ static InlineCacheAction tryRepatchIn(
     std::unique_ptr<AccessCase> newCase = AccessCase::create(
         vm, codeBlock, wasFound ? AccessCase::InHit : AccessCase::InMiss, invalidOffset, structure, conditionSet);
 
     std::unique_ptr<AccessCase> newCase = AccessCase::create(
         vm, codeBlock, wasFound ? AccessCase::InHit : AccessCase::InMiss, invalidOffset, structure, conditionSet);
 
-    AccessGenerationResult result = stubInfo.addAccessCase(codeBlock, ident, WTFMove(newCase));
+    AccessGenerationResult result = stubInfo.addAccessCase(locker, codeBlock, ident, WTFMove(newCase));
     
     if (result.generatedSomeCode()) {
         LOG_IC((ICEvent::InReplaceWithJump, structure->classInfo(), ident));
     
     if (result.generatedSomeCode()) {
         LOG_IC((ICEvent::InReplaceWithJump, structure->classInfo(), ident));
@@ -556,7 +556,7 @@ void repatchIn(
 {
     SuperSamplerScope superSamplerScope(false);
     GCSafeConcurrentJSLocker locker(exec->codeBlock()->m_lock, exec->vm().heap);
 {
     SuperSamplerScope superSamplerScope(false);
     GCSafeConcurrentJSLocker locker(exec->codeBlock()->m_lock, exec->vm().heap);
-    if (tryRepatchIn(exec, base, ident, wasFound, slot, stubInfo) == GiveUpOnCache)
+    if (tryRepatchIn(locker, exec, base, ident, wasFound, slot, stubInfo) == GiveUpOnCache)
         ftlThunkAwareRepatchCall(exec->codeBlock(), stubInfo.slowPathCallLocation(), operationIn);
 }
 
         ftlThunkAwareRepatchCall(exec->codeBlock(), stubInfo.slowPathCallLocation(), operationIn);
 }