[JSC] mustHandleValues for dead bytecode locals should be ignored in DFG phases
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Feb 2019 06:25:23 +0000 (06:25 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Feb 2019 06:25:23 +0000 (06:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195144
<rdar://problem/47595961>

Reviewed by Mark Lam.

JSTests:

* stress/read-dead-bytecode-locals-in-must-handle-values1.js: Added.
(bar):
(foo):
* stress/read-dead-bytecode-locals-in-must-handle-values2.js: Added.
(bar):
(foo):

Source/JavaScriptCore:

DFGMaximalFlushInsertionPhase inserts Flush for all the locals at the end of basic blocks. This enlarges the live ranges of
locals in DFG, and it sometimes makes DFG value live while it is dead in bytecode. The issue happens when we use mustHandleValues
to widen AbstractValue in CFAPhase. At that time, DFG tells "this value is live in DFG", but it may be dead in the bytecode level.
At that time, we attempt to merge AbstractValue with dead mustHandleValue, which is cleared as jsUndefined() in
DFG::Plan::cleanMustHandleValuesIfNecessary before start compilation, and crash because jsUndefined() may be irrelevant to the FlushFormat
in VariableAccessData.

This patch makes the type of mustHandleValues Operands<Optional<JSValue>>. We clear dead JSValues in DFG::Plan::cleanMustHandleValuesIfNecessary.
And we skip handling dead mustHandleValue in DFG phases.

* bytecode/Operands.h:
(JSC::Operands::isLocal const):
(JSC::Operands::isVariable const): Deleted.
* dfg/DFGCFAPhase.cpp:
(JSC::DFG::CFAPhase::injectOSR):
* dfg/DFGDriver.cpp:
(JSC::DFG::compileImpl):
(JSC::DFG::compile):
* dfg/DFGDriver.h:
* dfg/DFGJITCode.cpp:
(JSC::DFG::JITCode::reconstruct):
* dfg/DFGJITCode.h:
* dfg/DFGOperations.cpp:
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::Plan):
(JSC::DFG::Plan::checkLivenessAndVisitChildren):
(JSC::DFG::Plan::cleanMustHandleValuesIfNecessary):
* dfg/DFGPlan.h:
(JSC::DFG::Plan::mustHandleValues const):
* dfg/DFGPredictionInjectionPhase.cpp:
(JSC::DFG::PredictionInjectionPhase::run):
* dfg/DFGTypeCheckHoistingPhase.cpp:
(JSC::DFG::TypeCheckHoistingPhase::disableHoistingAcrossOSREntries):
* ftl/FTLOSREntry.cpp:
(JSC::FTL::prepareOSREntry):
* jit/JITOperations.cpp:

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

17 files changed:
JSTests/ChangeLog
JSTests/stress/read-dead-bytecode-locals-in-must-handle-values1.js [new file with mode: 0644]
JSTests/stress/read-dead-bytecode-locals-in-must-handle-values2.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/Operands.h
Source/JavaScriptCore/dfg/DFGCFAPhase.cpp
Source/JavaScriptCore/dfg/DFGDriver.cpp
Source/JavaScriptCore/dfg/DFGDriver.h
Source/JavaScriptCore/dfg/DFGJITCode.cpp
Source/JavaScriptCore/dfg/DFGJITCode.h
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/dfg/DFGPlan.cpp
Source/JavaScriptCore/dfg/DFGPlan.h
Source/JavaScriptCore/dfg/DFGPredictionInjectionPhase.cpp
Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp
Source/JavaScriptCore/ftl/FTLOSREntry.cpp
Source/JavaScriptCore/jit/JITOperations.cpp

index 11b22e6..1ba24d5 100644 (file)
@@ -1,3 +1,18 @@
+2019-02-27  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] mustHandleValues for dead bytecode locals should be ignored in DFG phases
+        https://bugs.webkit.org/show_bug.cgi?id=195144
+        <rdar://problem/47595961>
+
+        Reviewed by Mark Lam.
+
+        * stress/read-dead-bytecode-locals-in-must-handle-values1.js: Added.
+        (bar):
+        (foo):
+        * stress/read-dead-bytecode-locals-in-must-handle-values2.js: Added.
+        (bar):
+        (foo):
+
 2019-02-26  Yusuke Suzuki  <ysuzuki@apple.com>
 
         REGRESSION: stress/regress-178386.js is timing out on JSC debug bot
diff --git a/JSTests/stress/read-dead-bytecode-locals-in-must-handle-values1.js b/JSTests/stress/read-dead-bytecode-locals-in-must-handle-values1.js
new file mode 100644 (file)
index 0000000..9e2ae38
--- /dev/null
@@ -0,0 +1,18 @@
+//@ runDefault("--useMaximalFlushInsertionPhase=1", "--useConcurrentJIT=0")
+function bar(x) {
+  if (x) {
+    return;
+  }
+  x = x ** 0;
+  x = x * 2;
+}
+
+function foo() {
+  bar();
+  for (let i = 0; i < 10; ++i) {
+  }
+}
+
+for (var i = 0; i < 10000; ++i) {
+  foo();
+}
diff --git a/JSTests/stress/read-dead-bytecode-locals-in-must-handle-values2.js b/JSTests/stress/read-dead-bytecode-locals-in-must-handle-values2.js
new file mode 100644 (file)
index 0000000..99137f9
--- /dev/null
@@ -0,0 +1,19 @@
+//@ runDefault("--useMaximalFlushInsertionPhase=1", "--useConcurrentJIT=0")
+function bar(c) {
+    if (c > 1) {
+        bar(parseInt(c * 2))
+    }
+    c *= 2;
+}
+
+function foo() {
+    var i = 0;
+    if (''.match(/^/)) {
+        while(i < 1e8) {
+            bar(2);
+            ++i;
+        }
+    }
+}
+
+foo();
index e16f5d0..12621d3 100644 (file)
@@ -1,3 +1,48 @@
+2019-02-27  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] mustHandleValues for dead bytecode locals should be ignored in DFG phases
+        https://bugs.webkit.org/show_bug.cgi?id=195144
+        <rdar://problem/47595961>
+
+        Reviewed by Mark Lam.
+
+        DFGMaximalFlushInsertionPhase inserts Flush for all the locals at the end of basic blocks. This enlarges the live ranges of
+        locals in DFG, and it sometimes makes DFG value live while it is dead in bytecode. The issue happens when we use mustHandleValues
+        to widen AbstractValue in CFAPhase. At that time, DFG tells "this value is live in DFG", but it may be dead in the bytecode level.
+        At that time, we attempt to merge AbstractValue with dead mustHandleValue, which is cleared as jsUndefined() in
+        DFG::Plan::cleanMustHandleValuesIfNecessary before start compilation, and crash because jsUndefined() may be irrelevant to the FlushFormat
+        in VariableAccessData.
+
+        This patch makes the type of mustHandleValues Operands<Optional<JSValue>>. We clear dead JSValues in DFG::Plan::cleanMustHandleValuesIfNecessary.
+        And we skip handling dead mustHandleValue in DFG phases.
+
+        * bytecode/Operands.h:
+        (JSC::Operands::isLocal const):
+        (JSC::Operands::isVariable const): Deleted.
+        * dfg/DFGCFAPhase.cpp:
+        (JSC::DFG::CFAPhase::injectOSR):
+        * dfg/DFGDriver.cpp:
+        (JSC::DFG::compileImpl):
+        (JSC::DFG::compile):
+        * dfg/DFGDriver.h:
+        * dfg/DFGJITCode.cpp:
+        (JSC::DFG::JITCode::reconstruct):
+        * dfg/DFGJITCode.h:
+        * dfg/DFGOperations.cpp:
+        * dfg/DFGPlan.cpp:
+        (JSC::DFG::Plan::Plan):
+        (JSC::DFG::Plan::checkLivenessAndVisitChildren):
+        (JSC::DFG::Plan::cleanMustHandleValuesIfNecessary):
+        * dfg/DFGPlan.h:
+        (JSC::DFG::Plan::mustHandleValues const):
+        * dfg/DFGPredictionInjectionPhase.cpp:
+        (JSC::DFG::PredictionInjectionPhase::run):
+        * dfg/DFGTypeCheckHoistingPhase.cpp:
+        (JSC::DFG::TypeCheckHoistingPhase::disableHoistingAcrossOSREntries):
+        * ftl/FTLOSREntry.cpp:
+        (JSC::FTL::prepareOSREntry):
+        * jit/JITOperations.cpp:
+
 2019-02-27  Simon Fraser  <simon.fraser@apple.com>
 
         Roll out r242014; it caused crashes in compositing logging (webkit.org/b/195141)
index 8c5b57b..9944c05 100644 (file)
@@ -220,7 +220,7 @@ public:
     T& operator[](size_t index) { return at(index); }
 
     bool isArgument(size_t index) const { return index < m_numArguments; }
-    bool isVariable(size_t index) const { return !isArgument(index); }
+    bool isLocal(size_t index) const { return !isArgument(index); }
     int operandForIndex(size_t index) const
     {
         if (index < numberOfArguments())
index 27232f8..f6f9506 100644 (file)
@@ -163,10 +163,15 @@ private:
             dataLog("   Found must-handle block: ", *block, "\n");
         
         bool changed = false;
-        const Operands<JSValue>& mustHandleValues = m_graph.m_plan.mustHandleValues();
+        const Operands<Optional<JSValue>>& mustHandleValues = m_graph.m_plan.mustHandleValues();
         for (size_t i = mustHandleValues.size(); i--;) {
             int operand = mustHandleValues.operandForIndex(i);
-            JSValue value = mustHandleValues[i];
+            Optional<JSValue> value = mustHandleValues[i];
+            if (!value) {
+                if (m_verbose)
+                    dataLog("   Not live in bytecode: ", VirtualRegister(operand), "\n");
+                continue;
+            }
             Node* node = block->variablesAtHead.operand(operand);
             if (!node) {
                 if (m_verbose)
@@ -175,12 +180,12 @@ private:
             }
             
             if (m_verbose)
-                dataLog("   Widening ", VirtualRegister(operand), " with ", value, "\n");
+                dataLog("   Widening ", VirtualRegister(operand), " with ", value.value(), "\n");
             
             AbstractValue& target = block->valuesAtHead.operand(operand);
-            changed |= target.mergeOSREntryValue(m_graph, value);
+            changed |= target.mergeOSREntryValue(m_graph, value.value());
             target.fixTypeForRepresentation(
-                m_graph, resultFor(node->variableAccessData()->flushFormat()));
+                m_graph, resultFor(node->variableAccessData()->flushFormat()), node);
         }
         
         if (changed || !block->cfaHasVisited) {
index b56cf6d..4dda2f5 100644 (file)
@@ -70,7 +70,7 @@ static FunctionWhitelist& ensureGlobalDFGWhitelist()
 
 static CompilationResult compileImpl(
     VM& vm, CodeBlock* codeBlock, CodeBlock* profiledDFGCodeBlock, CompilationMode mode,
-    unsigned osrEntryBytecodeIndex, const Operands<JSValue>& mustHandleValues,
+    unsigned osrEntryBytecodeIndex, const Operands<Optional<JSValue>>& mustHandleValues,
     Ref<DeferredCompilationCallback>&& callback)
 {
     if (!Options::bytecodeRangeToDFGCompile().isInRange(codeBlock->instructionCount())
@@ -116,7 +116,7 @@ static CompilationResult compileImpl(
 }
 #else // ENABLE(DFG_JIT)
 static CompilationResult compileImpl(
-    VM&, CodeBlock*, CodeBlock*, CompilationMode, unsigned, const Operands<JSValue>&,
+    VM&, CodeBlock*, CodeBlock*, CompilationMode, unsigned, const Operands<Optional<JSValue>>&,
     Ref<DeferredCompilationCallback>&&)
 {
     return CompilationFailed;
@@ -125,7 +125,7 @@ static CompilationResult compileImpl(
 
 CompilationResult compile(
     VM& vm, CodeBlock* codeBlock, CodeBlock* profiledDFGCodeBlock, CompilationMode mode,
-    unsigned osrEntryBytecodeIndex, const Operands<JSValue>& mustHandleValues,
+    unsigned osrEntryBytecodeIndex, const Operands<Optional<JSValue>>& mustHandleValues,
     Ref<DeferredCompilationCallback>&& callback)
 {
     CompilationResult result = compileImpl(
index b2ef2cb..01f74ee 100644 (file)
@@ -42,7 +42,7 @@ JS_EXPORT_PRIVATE unsigned getNumCompilations();
 // compile. Even if we do a synchronous compile, we call the callback with the result.
 CompilationResult compile(
     VM&, CodeBlock*, CodeBlock* profiledDFGCodeBlock, CompilationMode,
-    unsigned osrEntryBytecodeIndex, const Operands<JSValue>& mustHandleValues,
+    unsigned osrEntryBytecodeIndex, const Operands<Optional<JSValue>>& mustHandleValues,
     Ref<DeferredCompilationCallback>&&);
 
 } } // namespace JSC::DFG
index 9780130..740fc8f 100644 (file)
@@ -78,12 +78,12 @@ void JITCode::reconstruct(
 
 void JITCode::reconstruct(
     ExecState* exec, CodeBlock* codeBlock, CodeOrigin codeOrigin, unsigned streamIndex,
-    Operands<JSValue>& result)
+    Operands<Optional<JSValue>>& result)
 {
     Operands<ValueRecovery> recoveries;
     reconstruct(codeBlock, codeOrigin, streamIndex, recoveries);
     
-    result = Operands<JSValue>(OperandsLike, recoveries);
+    result = Operands<Optional<JSValue>>(OperandsLike, recoveries);
     for (size_t i = result.size(); i--;)
         result[i] = recoveries[i].recover(exec);
 }
index 262e04f..af825ed 100644 (file)
@@ -98,7 +98,7 @@ public:
     // stack. Currently, it also has the restriction that the values must be in their
     // bytecode-designated stack slots.
     void reconstruct(
-        ExecState*, CodeBlock*, CodeOrigin, unsigned streamIndex, Operands<JSValue>& result);
+        ExecState*, CodeBlock*, CodeOrigin, unsigned streamIndex, Operands<Optional<JSValue>>& result);
 
 #if ENABLE(FTL_JIT)
     // NB. All of these methods take CodeBlock* because they may want to use
index 029b091..44c4135 100644 (file)
@@ -3108,7 +3108,7 @@ static void triggerFTLReplacementCompile(VM* vm, CodeBlock* codeBlock, JITCode*
     // We need to compile the code.
     compile(
         *vm, codeBlock->newReplacement(), codeBlock, FTLMode, UINT_MAX,
-        Operands<JSValue>(), ToFTLDeferredCompilationCallback::create());
+        Operands<Optional<JSValue>>(), ToFTLDeferredCompilationCallback::create());
 
     // If we reached here, the counter has not be reset. Do that now.
     jitCode->setOptimizationThresholdBasedOnCompilationResult(
@@ -3346,7 +3346,7 @@ static char* tierUpCommon(ExecState* exec, unsigned originBytecodeIndex, bool ca
 
     JITCode::TriggerReason* triggerAddress = &(triggerIterator->value);
 
-    Operands<JSValue> mustHandleValues;
+    Operands<Optional<JSValue>> mustHandleValues;
     unsigned streamIndex = jitCode->bytecodeIndexToStreamIndex.get(originBytecodeIndex);
     jitCode->reconstruct(
         exec, codeBlock, CodeOrigin(originBytecodeIndex), streamIndex, mustHandleValues);
index c5e899b..5733fb3 100644 (file)
@@ -135,7 +135,7 @@ Profiler::CompilationKind profilerCompilationKindForMode(CompilationMode mode)
 
 Plan::Plan(CodeBlock* passedCodeBlock, CodeBlock* profiledDFGCodeBlock,
     CompilationMode mode, unsigned osrEntryBytecodeIndex,
-    const Operands<JSValue>& mustHandleValues)
+    const Operands<Optional<JSValue>>& mustHandleValues)
     : m_vm(passedCodeBlock->vm())
     , m_codeBlock(passedCodeBlock)
     , m_profiledDFGCodeBlock(profiledDFGCodeBlock)
@@ -639,8 +639,11 @@ void Plan::checkLivenessAndVisitChildren(SlotVisitor& visitor)
         return;
 
     cleanMustHandleValuesIfNecessary();
-    for (unsigned i = m_mustHandleValues.size(); i--;)
-        visitor.appendUnbarriered(m_mustHandleValues[i]);
+    for (unsigned i = m_mustHandleValues.size(); i--;) {
+        Optional<JSValue> value = m_mustHandleValues[i];
+        if (value)
+            visitor.appendUnbarriered(value.value());
+    }
 
     m_recordedStatuses.markIfCheap(visitor);
 
@@ -715,7 +718,7 @@ void Plan::cleanMustHandleValuesIfNecessary()
 
     for (unsigned local = m_mustHandleValues.numberOfLocals(); local--;) {
         if (!liveness[local])
-            m_mustHandleValues.local(local) = jsUndefined();
+            m_mustHandleValues.local(local) = WTF::nullopt;
     }
 }
 
index 9afd458..d322e3b 100644 (file)
@@ -57,7 +57,7 @@ public:
     Plan(
         CodeBlock* codeBlockToCompile, CodeBlock* profiledDFGCodeBlock,
         CompilationMode, unsigned osrEntryBytecodeIndex,
-        const Operands<JSValue>& mustHandleValues);
+        const Operands<Optional<JSValue>>& mustHandleValues);
     ~Plan();
 
     void compileInThread(ThreadData*);
@@ -88,8 +88,7 @@ public:
     bool isFTL() const { return DFG::isFTL(m_mode); }
     CompilationMode mode() const { return m_mode; }
     unsigned osrEntryBytecodeIndex() const { return m_osrEntryBytecodeIndex; }
-    const Operands<JSValue>& mustHandleValues() const { return m_mustHandleValues; }
-
+    const Operands<Optional<JSValue>>& mustHandleValues() const { return m_mustHandleValues; }
     ThreadData* threadData() const { return m_threadData; }
     Profiler::Compilation* compilation() const { return m_compilation.get(); }
 
@@ -138,7 +137,7 @@ private:
 
     CompilationMode m_mode;
     const unsigned m_osrEntryBytecodeIndex;
-    Operands<JSValue> m_mustHandleValues;
+    Operands<Optional<JSValue>> m_mustHandleValues;
     bool m_mustHandleValuesMayIncludeGarbage { true };
     Lock m_mustHandleValueCleaningLock;
 
index d0c720d..447e24e 100644 (file)
@@ -70,15 +70,17 @@ public:
                 continue;
             if (block->bytecodeBegin != m_graph.m_plan.osrEntryBytecodeIndex())
                 continue;
-            const Operands<JSValue>& mustHandleValues = m_graph.m_plan.mustHandleValues();
+            const Operands<Optional<JSValue>>& mustHandleValues = m_graph.m_plan.mustHandleValues();
             for (size_t i = 0; i < mustHandleValues.size(); ++i) {
                 int operand = mustHandleValues.operandForIndex(i);
+                Optional<JSValue> value = mustHandleValues[i];
+                if (!value)
+                    continue;
                 Node* node = block->variablesAtHead.operand(operand);
                 if (!node)
                     continue;
                 ASSERT(node->accessesStack(m_graph));
-                node->variableAccessData()->predict(
-                    speculationFromValue(mustHandleValues[i]));
+                node->variableAccessData()->predict(speculationFromValue(value.value()));
             }
         }
         
index 9ff9e92..abbce49 100644 (file)
@@ -444,7 +444,7 @@ private:
                 continue;
             if (block->bytecodeBegin != m_graph.m_plan.osrEntryBytecodeIndex())
                 continue;
-            const Operands<JSValue>& mustHandleValues = m_graph.m_plan.mustHandleValues();
+            const Operands<Optional<JSValue>>& mustHandleValues = m_graph.m_plan.mustHandleValues();
             for (size_t i = 0; i < mustHandleValues.size(); ++i) {
                 int operand = mustHandleValues.operandForIndex(i);
                 Node* node = block->variablesAtHead.operand(operand);
@@ -456,8 +456,8 @@ private:
                     continue;
                 if (!TypeCheck::isValidToHoist(iter->value))
                     continue;
-                JSValue value = mustHandleValues[i];
-                if (!value || !value.isCell() || TypeCheck::isContravenedByValue(iter->value, value)) {
+                Optional<JSValue> value = mustHandleValues[i];
+                if (!value || !value.value() || !value.value().isCell() || TypeCheck::isContravenedByValue(iter->value, value.value())) {
                     TypeCheck::disableHoisting(iter->value);
                     continue;
                 }
index 626c128..0021bae 100644 (file)
@@ -64,7 +64,7 @@ void* prepareOSREntry(
         return 0;
     }
     
-    Operands<JSValue> values;
+    Operands<Optional<JSValue>> values;
     dfgCode->reconstruct(
         exec, dfgCodeBlock, CodeOrigin(bytecodeIndex), streamIndex, values);
     
@@ -73,8 +73,8 @@ void* prepareOSREntry(
     
     for (int argument = values.numberOfArguments(); argument--;) {
         JSValue valueOnStack = exec->r(virtualRegisterForArgument(argument).offset()).asanUnsafeJSValue();
-        JSValue reconstructedValue = values.argument(argument);
-        if (valueOnStack == reconstructedValue || !argument)
+        Optional<JSValue> reconstructedValue = values.argument(argument);
+        if ((reconstructedValue && valueOnStack == reconstructedValue.value()) || !argument)
             continue;
         dataLog("Mismatch between reconstructed values and the value on the stack for argument arg", argument, " for ", *entryCodeBlock, " at bc#", bytecodeIndex, ":\n");
         dataLog("    Value on stack: ", valueOnStack, "\n");
@@ -88,8 +88,13 @@ void* prepareOSREntry(
     EncodedJSValue* scratch = static_cast<EncodedJSValue*>(
         entryCode->entryBuffer()->dataBuffer());
     
-    for (int local = values.numberOfLocals(); local--;)
-        scratch[local] = JSValue::encode(values.local(local));
+    for (int local = values.numberOfLocals(); local--;) {
+        Optional<JSValue> value = values.local(local);
+        if (value)
+            scratch[local] = JSValue::encode(value.value());
+        else
+            scratch[local] = JSValue::encode(JSValue());
+    }
     
     int stackFrameSize = entryCode->common.requiredRegisterCountForExecutionAndExit();
     if (UNLIKELY(!vm.ensureStackCapacityFor(&exec->registers()[virtualRegisterForLocal(stackFrameSize - 1).offset()]))) {
index a58e5d7..5ffe8e0 100644 (file)
@@ -1542,7 +1542,7 @@ SlowPathReturnType JIT_OPERATION operationOptimize(ExecState* exec, uint32_t byt
             numVarsWithValues = codeBlock->numCalleeLocals();
         else
             numVarsWithValues = 0;
-        Operands<JSValue> mustHandleValues(codeBlock->numParameters(), numVarsWithValues);
+        Operands<Optional<JSValue>> mustHandleValues(codeBlock->numParameters(), numVarsWithValues);
         int localsUsedForCalleeSaves = static_cast<int>(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters());
         for (size_t i = 0; i < mustHandleValues.size(); ++i) {
             int operand = mustHandleValues.operandForIndex(i);