Title: [258443] trunk/Source/_javascript_Core
Revision
258443
Author
ysuz...@apple.com
Date
2020-03-13 16:20:45 -0700 (Fri, 13 Mar 2020)

Log Message

[JSC] Reload CodeBlock or suppress GC while setting up calls
https://bugs.webkit.org/show_bug.cgi?id=209033
<rdar://problem/58946936>

Reviewed by Saam Barati.

The sequence of Interpreter::execute is the following.

    1. Getting CodeBlock from Executable
    2. Doing a lot of setups
    3. Setting (1)'s CodeBlock to ProtoFrame
    4. Calling code through Executable

During (2), it would be possible that GC happens and it replaces CodeBlock in Executable.
Then, when executing JITCode with CodeBlock in (4), we use new JITCode with old CodeBlock.

In this patch,

For ProgramExecutable, FunctionExecutable, ModuleProgramExecutable, we ensure that no GC happens
after getting CodeBlock by placing DisallowGC. For EvalExecutable, we reload CodeBlock after setting
up environment. It is possible that FunctionExecutable* stored in CodeBlock can be different when
executing a new CodeBlock, but this is OK since this different does not appear and we do not rely on
this: we are touching `name` of FunctionExecutable* which is retrieved from CodeBlock. But this name
will not be changed since this is derived from UnlinkedFunctionExecutable which is shared by multiple
CodeBlocks. And FunctionExecutable* generation ordering must be the same for every CodeBlock generation
from the same UnlinkedCodeBlock.

* bytecode/CodeBlock.h:
(JSC::ScriptExecutable::prepareForExecution):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::executeProgram):
(JSC::Interpreter::executeCall):
(JSC::Interpreter::executeConstruct):
(JSC::Interpreter::execute):
(JSC::Interpreter::executeModuleProgram):
* interpreter/InterpreterInlines.h:
(JSC::Interpreter::execute):
* runtime/DisallowScope.h:
(JSC::DisallowScope::disable):
* runtime/StringPrototype.cpp:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (258442 => 258443)


--- trunk/Source/_javascript_Core/ChangeLog	2020-03-13 23:15:31 UTC (rev 258442)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-03-13 23:20:45 UTC (rev 258443)
@@ -1,3 +1,46 @@
+2020-03-13  Yusuke Suzuki  <ysuz...@apple.com>
+
+        [JSC] Reload CodeBlock or suppress GC while setting up calls
+        https://bugs.webkit.org/show_bug.cgi?id=209033
+        <rdar://problem/58946936>
+
+        Reviewed by Saam Barati.
+
+        The sequence of Interpreter::execute is the following.
+
+            1. Getting CodeBlock from Executable
+            2. Doing a lot of setups
+            3. Setting (1)'s CodeBlock to ProtoFrame
+            4. Calling code through Executable
+
+        During (2), it would be possible that GC happens and it replaces CodeBlock in Executable.
+        Then, when executing JITCode with CodeBlock in (4), we use new JITCode with old CodeBlock.
+
+        In this patch,
+
+        For ProgramExecutable, FunctionExecutable, ModuleProgramExecutable, we ensure that no GC happens
+        after getting CodeBlock by placing DisallowGC. For EvalExecutable, we reload CodeBlock after setting
+        up environment. It is possible that FunctionExecutable* stored in CodeBlock can be different when
+        executing a new CodeBlock, but this is OK since this different does not appear and we do not rely on
+        this: we are touching `name` of FunctionExecutable* which is retrieved from CodeBlock. But this name
+        will not be changed since this is derived from UnlinkedFunctionExecutable which is shared by multiple
+        CodeBlocks. And FunctionExecutable* generation ordering must be the same for every CodeBlock generation
+        from the same UnlinkedCodeBlock.
+
+        * bytecode/CodeBlock.h:
+        (JSC::ScriptExecutable::prepareForExecution):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::executeProgram):
+        (JSC::Interpreter::executeCall):
+        (JSC::Interpreter::executeConstruct):
+        (JSC::Interpreter::execute):
+        (JSC::Interpreter::executeModuleProgram):
+        * interpreter/InterpreterInlines.h:
+        (JSC::Interpreter::execute):
+        * runtime/DisallowScope.h:
+        (JSC::DisallowScope::disable):
+        * runtime/StringPrototype.cpp:
+
 2020-03-12  Yusuke Suzuki  <ysuz...@apple.com>
 
         [JSC] Delete IC creation should check mayNeedToCheckCell/canCacheDeleteIC regardless of Structure::outOfLineCapacity

Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.h (258442 => 258443)


--- trunk/Source/_javascript_Core/bytecode/CodeBlock.h	2020-03-13 23:15:31 UTC (rev 258442)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.h	2020-03-13 23:20:45 UTC (rev 258443)
@@ -1058,16 +1058,23 @@
 Exception* ScriptExecutable::prepareForExecution(VM& vm, JSFunction* function, JSScope* scope, CodeSpecializationKind kind, CodeBlock*& resultCodeBlock)
 {
     if (hasJITCodeFor(kind)) {
-        if (std::is_same<ExecutableType, EvalExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<EvalExecutable*>(this)->codeBlock());
-        else if (std::is_same<ExecutableType, ProgramExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ProgramExecutable*>(this)->codeBlock());
-        else if (std::is_same<ExecutableType, ModuleProgramExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ModuleProgramExecutable*>(this)->codeBlock());
-        else if (std::is_same<ExecutableType, FunctionExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<FunctionExecutable*>(this)->codeBlockFor(kind));
-        else
-            RELEASE_ASSERT_NOT_REACHED();
+        if constexpr (std::is_same<ExecutableType, EvalExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlock());
+            return nullptr;
+        }
+        if constexpr (std::is_same<ExecutableType, ProgramExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlock());
+            return nullptr;
+        }
+        if constexpr (std::is_same<ExecutableType, ModuleProgramExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlock());
+            return nullptr;
+        }
+        if constexpr (std::is_same<ExecutableType, FunctionExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlockFor(kind));
+            return nullptr;
+        }
+        RELEASE_ASSERT_NOT_REACHED();
         return nullptr;
     }
     return prepareForExecutionImpl(vm, function, scope, kind, resultCodeBlock);

Modified: trunk/Source/_javascript_Core/interpreter/Interpreter.cpp (258442 => 258443)


--- trunk/Source/_javascript_Core/interpreter/Interpreter.cpp	2020-03-13 23:15:31 UTC (rev 258442)
+++ trunk/Source/_javascript_Core/interpreter/Interpreter.cpp	2020-03-13 23:20:45 UTC (rev 258443)
@@ -822,6 +822,15 @@
     if (UNLIKELY(error))
         return checkedReturn(throwException(globalObject, throwScope, error));
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    if (scope->structure(vm)->isUncacheableDictionary())
+        scope->flattenDictionaryObject(vm);
+
     ProgramCodeBlock* codeBlock;
     {
         CodeBlock* tempCodeBlock;
@@ -830,25 +839,21 @@
         if (UNLIKELY(error))
             return checkedReturn(error);
         codeBlock = jsCast<ProgramCodeBlock*>(tempCodeBlock);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
     }
 
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-    }
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
 
-    if (scope->structure(vm)->isUncacheableDictionary())
-        scope->flattenDictionaryObject(vm);
+    RefPtr<JITCode> jitCode = program->generatedJITCode();
 
-    ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
-
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(codeBlock, globalObject, globalCallee, thisObj, 1);
 
     // Execute the code:
+    disallowGC.disable();
     throwScope.release();
-    JSValue result = program->generatedJITCode()->execute(&vm, &protoCallFrame);
+    ASSERT(jitCode == program->generatedJITCode().ptr());
+    JSValue result = jitCode->execute(&vm, &protoCallFrame);
     return checkedReturn(result);
 }
 
@@ -864,7 +869,6 @@
 
     bool isJSCall = (callType == CallType::JS);
     JSScope* scope = nullptr;
-    CodeBlock* newCodeBlock;
     size_t argsCount = 1 + args.size(); // implicit "this" parameter
 
     JSGlobalObject* globalObject;
@@ -881,6 +885,13 @@
     if (UNLIKELY(!vm.isSafeToRecurseSoft() || args.size() > maxArguments))
         return checkedReturn(throwStackOverflowError(globalObject, throwScope));
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    CodeBlock* newCodeBlock = nullptr;
     if (isJSCall) {
         // Compile the callee:
         Exception* compileError = callData.js.functionExecutable->prepareForExecution<FunctionExecutable>(vm, jsCast<JSFunction*>(function), scope, CodeForCall, newCodeBlock);
@@ -890,15 +901,12 @@
 
         ASSERT(!!newCodeBlock);
         newCodeBlock->m_shouldAlwaysBeInlined = false;
-    } else
-        newCodeBlock = 0;
-
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
     }
 
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+
+    RefPtr<JITCode> jitCode = callData.js.functionExecutable->generatedJITCodeForCall();
+
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(newCodeBlock, globalObject, function, thisValue, argsCount, args.data());
 
@@ -905,9 +913,11 @@
     JSValue result;
     {
         // Execute the code:
+        disallowGC.disable();
         if (isJSCall) {
             throwScope.release();
-            result = callData.js.functionExecutable->generatedJITCodeForCall()->execute(&vm, &protoCallFrame);
+            ASSERT(jitCode == callData.js.functionExecutable->generatedJITCodeForCall().ptr());
+            result = jitCode->execute(&vm, &protoCallFrame);
         } else {
             result = JSValue::decode(vmEntryToNative(callData.native.function.rawPointer(), &vm, &protoCallFrame));
             RETURN_IF_EXCEPTION(throwScope, JSValue());
@@ -933,7 +943,6 @@
 
     bool isJSConstruct = (constructType == ConstructType::JS);
     JSScope* scope = nullptr;
-    CodeBlock* newCodeBlock;
     size_t argsCount = 1 + args.size(); // implicit "this" parameter
 
     JSGlobalObject* globalObject;
@@ -952,6 +961,13 @@
         return nullptr;
     }
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, nullptr);
+    }
+
+    CodeBlock* newCodeBlock = nullptr;
     if (isJSConstruct) {
         // Compile the callee:
         Exception* compileError = constructData.js.functionExecutable->prepareForExecution<FunctionExecutable>(vm, jsCast<JSFunction*>(constructor), scope, CodeForConstruct, newCodeBlock);
@@ -961,15 +977,12 @@
 
         ASSERT(!!newCodeBlock);
         newCodeBlock->m_shouldAlwaysBeInlined = false;
-    } else
-        newCodeBlock = 0;
-
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, nullptr);
     }
 
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+
+    RefPtr<JITCode> jitCode = constructData.js.functionExecutable->generatedJITCodeForConstruct();
+
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(newCodeBlock, globalObject, constructor, newTarget, argsCount, args.data());
 
@@ -976,9 +989,11 @@
     JSValue result;
     {
         // Execute the code.
-        if (isJSConstruct)
-            result = constructData.js.functionExecutable->generatedJITCodeForConstruct()->execute(&vm, &protoCallFrame);
-        else {
+        disallowGC.disable();
+        if (isJSConstruct) {
+            ASSERT(jitCode == constructData.js.functionExecutable->generatedJITCodeForConstruct().ptr());
+            result = jitCode->execute(&vm, &protoCallFrame);
+        } else {
             result = JSValue::decode(vmEntryToNative(constructData.native.function.rawPointer(), &vm, &protoCallFrame));
 
             if (LIKELY(!throwScope.exception()))
@@ -1058,14 +1073,29 @@
         }
     }
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    auto loadCodeBlock = [&](Exception*& compileError) -> EvalCodeBlock* {
+        CodeBlock* tempCodeBlock;
+        compileError = eval->prepareForExecution<EvalExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
+        EXCEPTION_ASSERT(throwScope.exception() == compileError);
+        if (UNLIKELY(!!compileError))
+            return nullptr;
+        return jsCast<EvalCodeBlock*>(tempCodeBlock);
+    };
+
     EvalCodeBlock* codeBlock;
     {
-        CodeBlock* tempCodeBlock;
-        Exception* compileError = eval->prepareForExecution<EvalExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
+        Exception* compileError = nullptr;
+        codeBlock = loadCodeBlock(compileError);
         EXCEPTION_ASSERT(throwScope.exception() == compileError);
         if (UNLIKELY(!!compileError))
             return checkedReturn(compileError);
-        codeBlock = jsCast<EvalCodeBlock*>(tempCodeBlock);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
     }
     UnlinkedEvalCodeBlock* unlinkedCodeBlock = codeBlock->unlinkedEvalCodeBlock();
 
@@ -1148,25 +1178,34 @@
         }
     }
 
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-    }
-
-    ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
-
     JSCallee* callee = nullptr;
     if (scope == globalObject->globalScope())
         callee = globalObject->globalCallee();
     else
         callee = JSCallee::create(vm, globalObject, scope);
+
+    // Reload CodeBlock. It is possible that we replaced CodeBlock while setting up the environment.
+    {
+        Exception* compileError = nullptr;
+        codeBlock = loadCodeBlock(compileError);
+        EXCEPTION_ASSERT(throwScope.exception() == compileError);
+        if (UNLIKELY(!!compileError))
+            return checkedReturn(compileError);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
+    }
+
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+
+    RefPtr<JITCode> jitCode = eval->generatedJITCode();
+
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(codeBlock, globalObject, callee, thisValue, 1);
 
     // Execute the code:
+    disallowGC.disable();
     throwScope.release();
-    JSValue result = eval->generatedJITCode()->execute(&vm, &protoCallFrame);
+    ASSERT(jitCode == eval->generatedJITCode().ptr());
+    JSValue result = jitCode->execute(&vm, &protoCallFrame);
 
     return checkedReturn(result);
 }
@@ -1188,6 +1227,16 @@
     if (UNLIKELY(!vm.isSafeToRecurseSoft()))
         return checkedReturn(throwStackOverflowError(globalObject, throwScope));
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    if (scope->structure(vm)->isUncacheableDictionary())
+        scope->flattenDictionaryObject(vm);
+
+    JSCallee* callee = JSCallee::create(vm, globalObject, scope);
     ModuleProgramCodeBlock* codeBlock;
     {
         CodeBlock* tempCodeBlock;
@@ -1196,28 +1245,24 @@
         if (UNLIKELY(!!compileError))
             return checkedReturn(compileError);
         codeBlock = jsCast<ModuleProgramCodeBlock*>(tempCodeBlock);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
     }
 
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-    }
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
 
-    if (scope->structure(vm)->isUncacheableDictionary())
-        scope->flattenDictionaryObject(vm);
+    RefPtr<JITCode> jitCode = executable->generatedJITCode();
 
-    ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
-
     // The |this| of the module is always `undefined`.
     // http://www.ecma-international.org/ecma-262/6.0/#sec-module-environment-records-hasthisbinding
     // http://www.ecma-international.org/ecma-262/6.0/#sec-module-environment-records-getthisbinding
     ProtoCallFrame protoCallFrame;
-    protoCallFrame.init(codeBlock, globalObject, JSCallee::create(vm, globalObject, scope), jsUndefined(), 1);
+    protoCallFrame.init(codeBlock, globalObject, callee, jsUndefined(), 1);
 
     // Execute the code:
+    disallowGC.disable();
     throwScope.release();
-    JSValue result = executable->generatedJITCode()->execute(&vm, &protoCallFrame);
+    ASSERT(jitCode == executable->generatedJITCode().ptr());
+    JSValue result = jitCode->execute(&vm, &protoCallFrame);
 
     return checkedReturn(result);
 }

Modified: trunk/Source/_javascript_Core/interpreter/InterpreterInlines.h (258442 => 258443)


--- trunk/Source/_javascript_Core/interpreter/InterpreterInlines.h	2020-03-13 23:15:31 UTC (rev 258442)
+++ trunk/Source/_javascript_Core/interpreter/InterpreterInlines.h	2020-03-13 23:20:45 UTC (rev 258443)
@@ -28,10 +28,13 @@
 
 #include "CallFrameClosure.h"
 #include "Exception.h"
+#include "FunctionCodeBlock.h"
+#include "FunctionExecutable.h"
 #include "Instruction.h"
 #include "Interpreter.h"
 #include "JSCPtrTag.h"
 #include "LLIntData.h"
+#include "ProtoCallFrameInlines.h"
 #include "UnlinkedCodeBlock.h"
 #include <wtf/UnalignedAccess.h>
 
@@ -80,6 +83,17 @@
         RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
     }
 
+    // Reload CodeBlock since GC can replace CodeBlock owned by Executable.
+    CodeBlock* codeBlock;
+    Exception* error = closure.functionExecutable->prepareForExecution<FunctionExecutable>(vm, closure.function, closure.scope, CodeForCall, codeBlock);
+    EXCEPTION_ASSERT(throwScope.exception() == error);
+    if (UNLIKELY(error))
+        return checkedReturn(error);
+    codeBlock->m_shouldAlwaysBeInlined = false;
+    {
+        DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+        closure.protoCallFrame->setCodeBlock(codeBlock);
+    }
     // Execute the code:
     throwScope.release();
     JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);

Modified: trunk/Source/_javascript_Core/runtime/DisallowScope.h (258442 => 258443)


--- trunk/Source/_javascript_Core/runtime/DisallowScope.h	2020-03-13 23:15:31 UTC (rev 258442)
+++ trunk/Source/_javascript_Core/runtime/DisallowScope.h	2020-03-13 23:20:45 UTC (rev 258443)
@@ -41,6 +41,7 @@
     ALWAYS_INLINE ~DisallowScope() { }
     ALWAYS_INLINE static bool isInEffectOnCurrentThread() { return false; }
     ALWAYS_INLINE void enable() { }
+    ALWAYS_INLINE void disable() { }
 
 #else // not NDEBUG
 

Modified: trunk/Source/_javascript_Core/runtime/StringPrototype.cpp (258442 => 258443)


--- trunk/Source/_javascript_Core/runtime/StringPrototype.cpp	2020-03-13 23:15:31 UTC (rev 258442)
+++ trunk/Source/_javascript_Core/runtime/StringPrototype.cpp	2020-03-13 23:20:45 UTC (rev 258443)
@@ -27,6 +27,7 @@
 #include "ButterflyInlines.h"
 #include "CachedCall.h"
 #include "Error.h"
+#include "ExecutableBaseInlines.h"
 #include "FrameTracers.h"
 #include "InterpreterInlines.h"
 #include "IntlCollator.h"
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to