[JSC] Introduce DisposableCallSiteIndex to enforce type-safety
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Jun 2019 18:49:03 +0000 (18:49 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Jun 2019 18:49:03 +0000 (18:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197378

Reviewed by Saam Barati.

JSTests:

* stress/disposable-call-site-index-with-call-and-this.js: Added.
(foo):
(bar):
* stress/disposable-call-site-index.js: Added.
(foo):
(bar):

Source/JavaScriptCore:

Some of CallSiteIndex are disposable. This is because some of CallSiteIndex are allocated and freed at runtime (not DFG/FTL compile time).
The example is CallSiteIndex for exception handler in GCAwareJITStubRoutineWithExceptionHandler. If we do not allocate and free CallSiteIndex,
we will create a new CallSiteIndex continuously and leak memory.

The other CallSiteIndex are not simply disposable because the ownership model is not unique one. They can be shared between multiple clients.
But not disposing them is OK because they are static one: they are allocated when compiling DFG/FTL, and we do not allocate such CallSiteIndex
at runtime.

To make this difference explicit and avoid disposing non-disposable CallSiteIndex accidentally, we introduce DisposableCallSiteIndex type, and
enforce type-safety to some degree.

We also correctly update the DisposableCallSiteIndex => CodeOrigin table when we are reusing the previously used DisposableCallSiteIndex.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::newExceptionHandlingCallSiteIndex):
(JSC::CodeBlock::removeExceptionHandlerForCallSite):
* bytecode/CodeBlock.h:
* bytecode/PolymorphicAccess.cpp:
(JSC::AccessGenerationState::callSiteIndexForExceptionHandling):
(JSC::PolymorphicAccess::regenerate):
* bytecode/PolymorphicAccess.h:
(JSC::AccessGenerationState::callSiteIndexForExceptionHandling): Deleted.
* dfg/DFGCommonData.cpp:
(JSC::DFG::CommonData::addUniqueCallSiteIndex):
(JSC::DFG::CommonData::addDisposableCallSiteIndex):
(JSC::DFG::CommonData::removeDisposableCallSiteIndex):
(JSC::DFG::CommonData::removeCallSiteIndex): Deleted.
* dfg/DFGCommonData.h:
* interpreter/CallFrame.h:
(JSC::DisposableCallSiteIndex::DisposableCallSiteIndex):
(JSC::DisposableCallSiteIndex::fromCallSiteIndex):
* jit/GCAwareJITStubRoutine.cpp:
(JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
(JSC::GCAwareJITStubRoutineWithExceptionHandler::observeZeroRefCount):
(JSC::createJITStubRoutine):
* jit/GCAwareJITStubRoutine.h:
* jit/JITInlineCacheGenerator.h:

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

14 files changed:
JSTests/ChangeLog
JSTests/stress/disposable-call-site-index-with-call-and-this.js [new file with mode: 0644]
JSTests/stress/disposable-call-site-index.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
Source/JavaScriptCore/bytecode/PolymorphicAccess.h
Source/JavaScriptCore/dfg/DFGCommonData.cpp
Source/JavaScriptCore/dfg/DFGCommonData.h
Source/JavaScriptCore/interpreter/CallFrame.h
Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp
Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h

index 0a79ce8..5a9f182 100644 (file)
@@ -1,3 +1,17 @@
+2019-06-17  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Introduce DisposableCallSiteIndex to enforce type-safety
+        https://bugs.webkit.org/show_bug.cgi?id=197378
+
+        Reviewed by Saam Barati.
+
+        * stress/disposable-call-site-index-with-call-and-this.js: Added.
+        (foo):
+        (bar):
+        * stress/disposable-call-site-index.js: Added.
+        (foo):
+        (bar):
+
 2019-06-17  Justin Michaud  <justin_michaud@apple.com>
 
         [WASM-References] Add support for Funcref in parameters and return types
diff --git a/JSTests/stress/disposable-call-site-index-with-call-and-this.js b/JSTests/stress/disposable-call-site-index-with-call-and-this.js
new file mode 100644 (file)
index 0000000..5eba622
--- /dev/null
@@ -0,0 +1,26 @@
+var ia = new Int8Array(1024);
+
+function foo(o) {
+    return o.f;
+}
+
+function bar(o) {
+
+    try {
+        o.f = 0x1;
+        Uint8Array.prototype.find.call(ia, function () {
+            o.f = 0x1;
+        }, this);
+    } catch (e) {
+    }
+
+    foo(o);
+}
+
+var o = new Object();
+o.__defineGetter__("f", function () { });
+
+for (var i = 0; i < 1000; ++i) {
+    bar({});
+    bar(o);
+}
diff --git a/JSTests/stress/disposable-call-site-index.js b/JSTests/stress/disposable-call-site-index.js
new file mode 100644 (file)
index 0000000..50f7dff
--- /dev/null
@@ -0,0 +1,28 @@
+//@ runDefault("--useConcurrentJIT=0", "--useConcurrentGC=0", "--thresholdForJITAfterWarmUp=10", "--thresholdForOptimizeAfterWarmUp=100", "--thresholdForOptimizeAfterLongWarmUp=100", "--thresholdForOptimizeAfterLongWarmUp=100")
+
+var ia = new Int8Array(1024);
+
+function foo(o) {
+    return o.f;
+}
+
+function bar(o) {
+
+    try {
+        o.f = 0x1;
+        Uint8Array.prototype.find.call(ia, function () {
+            o.f = 0x1;
+        });
+    } catch (e) {
+    }
+
+    foo(o);
+}
+
+var o = new Object();
+o.__defineGetter__("f", function () { });
+
+for (var i = 0; i < 1000; ++i) {
+    bar({});
+    bar(o);
+}
index b56953b..99c24f8 100644 (file)
@@ -1,3 +1,48 @@
+2019-06-17  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Introduce DisposableCallSiteIndex to enforce type-safety
+        https://bugs.webkit.org/show_bug.cgi?id=197378
+
+        Reviewed by Saam Barati.
+
+        Some of CallSiteIndex are disposable. This is because some of CallSiteIndex are allocated and freed at runtime (not DFG/FTL compile time).
+        The example is CallSiteIndex for exception handler in GCAwareJITStubRoutineWithExceptionHandler. If we do not allocate and free CallSiteIndex,
+        we will create a new CallSiteIndex continuously and leak memory.
+
+        The other CallSiteIndex are not simply disposable because the ownership model is not unique one. They can be shared between multiple clients.
+        But not disposing them is OK because they are static one: they are allocated when compiling DFG/FTL, and we do not allocate such CallSiteIndex
+        at runtime.
+
+        To make this difference explicit and avoid disposing non-disposable CallSiteIndex accidentally, we introduce DisposableCallSiteIndex type, and
+        enforce type-safety to some degree.
+
+        We also correctly update the DisposableCallSiteIndex => CodeOrigin table when we are reusing the previously used DisposableCallSiteIndex.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::newExceptionHandlingCallSiteIndex):
+        (JSC::CodeBlock::removeExceptionHandlerForCallSite):
+        * bytecode/CodeBlock.h:
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::AccessGenerationState::callSiteIndexForExceptionHandling):
+        (JSC::PolymorphicAccess::regenerate):
+        * bytecode/PolymorphicAccess.h:
+        (JSC::AccessGenerationState::callSiteIndexForExceptionHandling): Deleted.
+        * dfg/DFGCommonData.cpp:
+        (JSC::DFG::CommonData::addUniqueCallSiteIndex):
+        (JSC::DFG::CommonData::addDisposableCallSiteIndex):
+        (JSC::DFG::CommonData::removeDisposableCallSiteIndex):
+        (JSC::DFG::CommonData::removeCallSiteIndex): Deleted.
+        * dfg/DFGCommonData.h:
+        * interpreter/CallFrame.h:
+        (JSC::DisposableCallSiteIndex::DisposableCallSiteIndex):
+        (JSC::DisposableCallSiteIndex::fromCallSiteIndex):
+        * jit/GCAwareJITStubRoutine.cpp:
+        (JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
+        (JSC::GCAwareJITStubRoutineWithExceptionHandler::observeZeroRefCount):
+        (JSC::createJITStubRoutine):
+        * jit/GCAwareJITStubRoutine.h:
+        * jit/JITInlineCacheGenerator.h:
+
 2019-06-17  Justin Michaud  <justin_michaud@apple.com>
 
         [WASM-References] Add support for Funcref in parameters and return types
index 4a9bb32..dacae67 100644 (file)
@@ -1725,20 +1725,20 @@ HandlerInfo* CodeBlock::handlerForIndex(unsigned index, RequiredHandler required
     return HandlerInfo::handlerForIndex(m_rareData->m_exceptionHandlers, index, requiredHandler);
 }
 
-CallSiteIndex CodeBlock::newExceptionHandlingCallSiteIndex(CallSiteIndex originalCallSite)
+DisposableCallSiteIndex CodeBlock::newExceptionHandlingCallSiteIndex(CallSiteIndex originalCallSite)
 {
 #if ENABLE(DFG_JIT)
     RELEASE_ASSERT(JITCode::isOptimizingJIT(jitType()));
     RELEASE_ASSERT(canGetCodeOrigin(originalCallSite));
     ASSERT(!!handlerForIndex(originalCallSite.bits()));
     CodeOrigin originalOrigin = codeOrigin(originalCallSite);
-    return m_jitCode->dfgCommon()->addUniqueCallSiteIndex(originalOrigin);
+    return m_jitCode->dfgCommon()->addDisposableCallSiteIndex(originalOrigin);
 #else
     // We never create new on-the-fly exception handling
     // call sites outside the DFG/FTL inline caches.
     UNUSED_PARAM(originalCallSite);
     RELEASE_ASSERT_NOT_REACHED();
-    return CallSiteIndex(0u);
+    return DisposableCallSiteIndex(0u);
 #endif
 }
 
@@ -1808,7 +1808,7 @@ void CodeBlock::ensureCatchLivenessIsComputedForBytecodeOffsetSlow(const OpCatch
     }
 }
 
-void CodeBlock::removeExceptionHandlerForCallSite(CallSiteIndex callSiteIndex)
+void CodeBlock::removeExceptionHandlerForCallSite(DisposableCallSiteIndex callSiteIndex)
 {
     RELEASE_ASSERT(m_rareData);
     Vector<HandlerInfo>& exceptionHandlers = m_rareData->m_exceptionHandlers;
index 4d3279e..6648fc5 100644 (file)
@@ -241,7 +241,7 @@ public:
 
     HandlerInfo* handlerForBytecodeOffset(unsigned bytecodeOffset, RequiredHandler = RequiredHandler::AnyHandler);
     HandlerInfo* handlerForIndex(unsigned, RequiredHandler = RequiredHandler::AnyHandler);
-    void removeExceptionHandlerForCallSite(CallSiteIndex);
+    void removeExceptionHandlerForCallSite(DisposableCallSiteIndex);
     unsigned lineNumberForBytecodeOffset(unsigned bytecodeOffset);
     unsigned columnNumberForBytecodeOffset(unsigned bytecodeOffset);
     void expressionRangeForBytecodeOffset(unsigned bytecodeOffset, int& divot,
@@ -867,7 +867,7 @@ public:
         m_rareData->m_exceptionHandlers.append(handler);
     }
 
-    CallSiteIndex newExceptionHandlingCallSiteIndex(CallSiteIndex originalCallSite);
+    DisposableCallSiteIndex newExceptionHandlingCallSiteIndex(CallSiteIndex originalCallSite);
 
     void ensureCatchLivenessIsComputedForBytecodeOffset(InstructionStream::Offset bytecodeOffset);
 
index c695f7e..22f4558 100644 (file)
@@ -163,6 +163,14 @@ CallSiteIndex AccessGenerationState::callSiteIndexForExceptionHandlingOrOriginal
     return m_callSiteIndex;
 }
 
+DisposableCallSiteIndex AccessGenerationState::callSiteIndexForExceptionHandling()
+{
+    RELEASE_ASSERT(m_calculatedRegistersForCallAndExceptionHandling);
+    RELEASE_ASSERT(m_needsToRestoreRegistersIfException);
+    RELEASE_ASSERT(m_calculatedCallSiteIndex);
+    return DisposableCallSiteIndex::fromCallSiteIndex(m_callSiteIndex);
+}
+
 const HandlerInfo& AccessGenerationState::originalExceptionHandler()
 {
     if (!m_calculatedRegistersForCallAndExceptionHandling)
@@ -539,7 +547,7 @@ AccessGenerationResult PolymorphicAccess::regenerate(
     failure.append(jit.jump());
 
     CodeBlock* codeBlockThatOwnsExceptionHandlers = nullptr;
-    CallSiteIndex callSiteIndexForExceptionHandling;
+    DisposableCallSiteIndex callSiteIndexForExceptionHandling;
     if (state.needsToRestoreRegistersIfException() && hasJSGetterSetterCall) {
         // Emit the exception handler.
         // Note that this code is only reachable when doing genericUnwind from a pure JS getter/setter .
@@ -561,7 +569,7 @@ AccessGenerationResult PolymorphicAccess::regenerate(
         CCallHelpers::Jump jumpToOSRExitExceptionHandler = jit.jump();
 
         HandlerInfo oldHandler = state.originalExceptionHandler();
-        CallSiteIndex newExceptionHandlingCallSite = state.callSiteIndexForExceptionHandling();
+        DisposableCallSiteIndex newExceptionHandlingCallSite = state.callSiteIndexForExceptionHandling();
         jit.addLinkTask(
             [=] (LinkBuffer& linkBuffer) {
                 linkBuffer.link(jumpToOSRExitExceptionHandler, oldHandler.nativeCode);
index 3055855..97a4e13 100644 (file)
@@ -241,13 +241,7 @@ struct AccessGenerationState {
     const RegisterSet& liveRegistersForCall();
 
     CallSiteIndex callSiteIndexForExceptionHandlingOrOriginal();
-    CallSiteIndex callSiteIndexForExceptionHandling()
-    {
-        RELEASE_ASSERT(m_calculatedRegistersForCallAndExceptionHandling);
-        RELEASE_ASSERT(m_needsToRestoreRegistersIfException);
-        RELEASE_ASSERT(m_calculatedCallSiteIndex);
-        return m_callSiteIndex;
-    }
+    DisposableCallSiteIndex callSiteIndexForExceptionHandling();
 
     const HandlerInfo& originalExceptionHandler();
 
@@ -271,7 +265,7 @@ private:
     
     RegisterSet m_liveRegistersToPreserveAtExceptionHandlingCallSite;
     RegisterSet m_liveRegistersForCall;
-    CallSiteIndex m_callSiteIndex { CallSiteIndex(std::numeric_limits<unsigned>::max()) };
+    CallSiteIndex m_callSiteIndex;
     SpillState m_spillStateForJSGetterSetter;
     bool m_calculatedRegistersForCallAndExceptionHandling : 1;
     bool m_needsToRestoreRegistersIfException : 1;
index 5b6cf5c..7c80750 100644 (file)
@@ -61,9 +61,6 @@ CallSiteIndex CommonData::addCodeOrigin(CodeOrigin codeOrigin)
 
 CallSiteIndex CommonData::addUniqueCallSiteIndex(CodeOrigin codeOrigin)
 {
-    if (callSiteIndexFreeList.size())
-        return CallSiteIndex(callSiteIndexFreeList.takeAny());
-
     codeOrigins.append(codeOrigin);
     unsigned index = codeOrigins.size() - 1;
     ASSERT(codeOrigins[index] == codeOrigin);
@@ -76,10 +73,26 @@ CallSiteIndex CommonData::lastCallSite() const
     return CallSiteIndex(codeOrigins.size() - 1);
 }
 
-void CommonData::removeCallSiteIndex(CallSiteIndex callSite)
+DisposableCallSiteIndex CommonData::addDisposableCallSiteIndex(CodeOrigin codeOrigin)
+{
+    if (callSiteIndexFreeList.size()) {
+        unsigned index = callSiteIndexFreeList.takeAny();
+        codeOrigins[index] = codeOrigin;
+        return DisposableCallSiteIndex(index);
+    }
+
+    codeOrigins.append(codeOrigin);
+    unsigned index = codeOrigins.size() - 1;
+    ASSERT(codeOrigins[index] == codeOrigin);
+    return DisposableCallSiteIndex(index);
+}
+
+
+void CommonData::removeDisposableCallSiteIndex(DisposableCallSiteIndex callSite)
 {
     RELEASE_ASSERT(callSite.bits() < codeOrigins.size());
     callSiteIndexFreeList.add(callSite.bits());
+    codeOrigins[callSite.bits()] = CodeOrigin();
 }
 
 void CommonData::shrinkToFit()
index 5fda5f5..4b2167f 100644 (file)
@@ -83,7 +83,9 @@ public:
     CallSiteIndex addCodeOrigin(CodeOrigin);
     CallSiteIndex addUniqueCallSiteIndex(CodeOrigin);
     CallSiteIndex lastCallSite() const;
-    void removeCallSiteIndex(CallSiteIndex);
+
+    DisposableCallSiteIndex addDisposableCallSiteIndex(CodeOrigin);
+    void removeDisposableCallSiteIndex(DisposableCallSiteIndex);
     
     void shrinkToFit();
     
index 6b07ba4..a46703d 100644 (file)
@@ -43,11 +43,9 @@ namespace JSC  {
 
     typedef ExecState CallFrame;
 
-    struct CallSiteIndex {
-        CallSiteIndex()
-            : m_bits(UINT_MAX)
-        {
-        }
+    class CallSiteIndex {
+    public:
+        CallSiteIndex() = default;
         
         explicit CallSiteIndex(uint32_t bits)
             : m_bits(bits)
@@ -64,7 +62,22 @@ namespace JSC  {
         inline uint32_t bits() const { return m_bits; }
 
     private:
-        uint32_t m_bits;
+        uint32_t m_bits { UINT_MAX };
+    };
+
+    class DisposableCallSiteIndex : public CallSiteIndex {
+    public:
+        DisposableCallSiteIndex() = default;
+
+        explicit DisposableCallSiteIndex(uint32_t bits)
+            : CallSiteIndex(bits)
+        {
+        }
+
+        static DisposableCallSiteIndex fromCallSiteIndex(CallSiteIndex callSiteIndex)
+        {
+            return DisposableCallSiteIndex(callSiteIndex.bits());
+        }
     };
 
     // arm64_32 expects caller frame and return pc to use 8 bytes 
index 195ff02..d501d91 100644 (file)
@@ -102,7 +102,7 @@ void MarkingGCAwareJITStubRoutine::markRequiredObjectsInternal(SlotVisitor& visi
 
 GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler(
     const MacroAssemblerCodeRef<JITStubRoutinePtrTag>& code, VM& vm,  const JSCell* owner, const Vector<JSCell*>& cells,
-    CodeBlock* codeBlockForExceptionHandlers, CallSiteIndex exceptionHandlerCallSiteIndex)
+    CodeBlock* codeBlockForExceptionHandlers, DisposableCallSiteIndex exceptionHandlerCallSiteIndex)
     : MarkingGCAwareJITStubRoutine(code, vm, owner, cells)
     , m_codeBlockWithExceptionHandler(codeBlockForExceptionHandlers)
     , m_exceptionHandlerCallSiteIndex(exceptionHandlerCallSiteIndex)
@@ -120,7 +120,7 @@ void GCAwareJITStubRoutineWithExceptionHandler::observeZeroRefCount()
 {
 #if ENABLE(DFG_JIT)
     if (m_codeBlockWithExceptionHandler) {
-        m_codeBlockWithExceptionHandler->jitCode()->dfgCommon()->removeCallSiteIndex(m_exceptionHandlerCallSiteIndex);
+        m_codeBlockWithExceptionHandler->jitCode()->dfgCommon()->removeDisposableCallSiteIndex(m_exceptionHandlerCallSiteIndex);
         m_codeBlockWithExceptionHandler->removeExceptionHandlerForCallSite(m_exceptionHandlerCallSiteIndex);
         m_codeBlockWithExceptionHandler = nullptr;
     }
@@ -137,7 +137,7 @@ Ref<JITStubRoutine> createJITStubRoutine(
     bool makesCalls,
     const Vector<JSCell*>& cells,
     CodeBlock* codeBlockForExceptionHandlers,
-    CallSiteIndex exceptionHandlerCallSiteIndex)
+    DisposableCallSiteIndex exceptionHandlerCallSiteIndex)
 {
     if (!makesCalls)
         return adoptRef(*new JITStubRoutine(code));
index 5dedc6a..471a675 100644 (file)
@@ -89,19 +89,19 @@ private:
 
 // The stub has exception handlers in it. So it clears itself from exception
 // handling table when it dies. It also frees space in CodeOrigin table
-// for new exception handlers to use the same CallSiteIndex.
+// for new exception handlers to use the same DisposableCallSiteIndex.
 class GCAwareJITStubRoutineWithExceptionHandler : public MarkingGCAwareJITStubRoutine {
 public:
     typedef GCAwareJITStubRoutine Base;
 
-    GCAwareJITStubRoutineWithExceptionHandler(const MacroAssemblerCodeRef<JITStubRoutinePtrTag>&, VM&, const JSCell* owner, const Vector<JSCell*>&, CodeBlock*, CallSiteIndex);
+    GCAwareJITStubRoutineWithExceptionHandler(const MacroAssemblerCodeRef<JITStubRoutinePtrTag>&, VM&, const JSCell* owner, const Vector<JSCell*>&, CodeBlock*, DisposableCallSiteIndex);
 
     void aboutToDie() override;
     void observeZeroRefCount() override;
 
 private:
     CodeBlock* m_codeBlockWithExceptionHandler;
-    CallSiteIndex m_exceptionHandlerCallSiteIndex;
+    DisposableCallSiteIndex m_exceptionHandlerCallSiteIndex;
 };
 
 // Helper for easily creating a GC-aware JIT stub routine. For the varargs,
@@ -126,7 +126,7 @@ private:
 Ref<JITStubRoutine> createJITStubRoutine(
     const MacroAssemblerCodeRef<JITStubRoutinePtrTag>&, VM&, const JSCell* owner, bool makesCalls,
     const Vector<JSCell*>& = { }, 
-    CodeBlock* codeBlockForExceptionHandlers = nullptr, CallSiteIndex exceptionHandlingCallSiteIndex = CallSiteIndex(std::numeric_limits<unsigned>::max()));
+    CodeBlock* codeBlockForExceptionHandlers = nullptr, DisposableCallSiteIndex exceptionHandlingCallSiteIndex = DisposableCallSiteIndex());
 
 } // namespace JSC
 
index b747b86..f857127 100644 (file)
 
 namespace JSC {
 
+class CallSiteIndex;
 class CodeBlock;
 class StructureStubInfo;
 
-struct CallSiteIndex;
-
 enum class AccessType : int8_t;
 
 class JITInlineCacheGenerator {