Changeset 209725 in webkit for trunk/Source/JavaScriptCore/jit/CallFrameShuffler.cpp

Timestamp:

Dec 12, 2016, 1:46:45 PM (9 years ago)

Author:

[email protected]

Message:

REGRESSION(r209653): speedometer crashes making virtual slow path tailcalls
​https://bugs.webkit.org/show_bug.cgi?id=165748

Reviewed by Filip Pizlo.

JSTests:

New regression test.

• stress/regress-165748.js: Added.

(sum1):
(sum2):
(sum3):
(sum4):
(sum5):
(sum6):
(tailCaller):
(test):

Source/JavaScriptCore:

The virtual slow path for tailcalls always passes arguments on the stack.
The fix here is to link to the stack argument entrypoint instead of a register
argument entrypoint.

While fixing this bug, I found that we weren't clearing the code origin when
shuffling the call frame for a register argument tailcall.

Also rolling back in r209653, r209654, r209663, and r209673.

• jit/CallFrameShuffler.cpp:

(JSC::CallFrameShuffler::prepareAny):

• jit/ThunkGenerators.cpp:

(JSC::virtualThunkFor):

Source/WTF:

Rolling back in r209653, r209654, r209663, and r209673.

• wtf/Platform.h:

File:

• trunk/Source/JavaScriptCore/jit/CallFrameShuffler.cpp (modified) (9 diffs)

trunk/Source/JavaScriptCore/jit/CallFrameShuffler.cpp

@@ -43 +43 @@
     , m_alignedNewFrameSize(CallFrame::headerSizeInRegisters
         + roundArgumentCountToAlignFrame(data.args.size()))
+#if USE(JSVALUE64)
+    , m_argumentsInRegisters(data.argumentsInRegisters)
+#endif
     , m_frameDelta(m_alignedNewFrameSize - m_alignedOldFrameSize)
     , m_lockedRegisters(RegisterSet::allRegisters())
@@ -55 +58 @@
 
     ASSERT(!data.callee.isInJSStack() || data.callee.virtualRegister().isLocal());
-    addNew(VirtualRegister(CallFrameSlot::callee), data.callee);
-
+#if USE(JSVALUE64)
+    if (data.argumentsInRegisters)
+        addNew(JSValueRegs(argumentRegisterForCallee()), data.callee);
+    else
+#endif
+        addNew(VirtualRegister(CallFrameSlot::callee), data.callee);
+    
     for (size_t i = 0; i < data.args.size(); ++i) {
         ASSERT(!data.args[i].isInJSStack() || data.args[i].virtualRegister().isLocal());
-        addNew(virtualRegisterForArgument(i), data.args[i]);
+#if USE(JSVALUE64)
+        if (data.argumentsInRegisters && i < NUMBER_OF_JS_FUNCTION_ARGUMENT_REGISTERS)
+            addNew(JSValueRegs(argumentRegisterForFunctionArgument(i)), data.args[i]);
+        else
+#endif
+            addNew(virtualRegisterForArgument(i), data.args[i]);
     }
 
@@ -186 +199 @@
         }
 #else
-        if (newCachedRecovery)
+        if (newCachedRecovery) {
             out.print("         ", reg, " <- ", newCachedRecovery->recovery());
+            if (newCachedRecovery->gprTargets().size() > 1) {
+                for (size_t i = 1; i < newCachedRecovery->gprTargets().size(); i++)
+                    out.print(", ", newCachedRecovery->gprTargets()[i].gpr(), " <- ", newCachedRecovery->recovery());
+            }
+        }
 #endif
         out.print("\n");
@@ -497 +515 @@
         || cachedRecovery.recovery().isConstant());
 
-    if (verbose)
+    if (verbose && cachedRecovery.targets().size())
         dataLog("   * Storing ", cachedRecovery.recovery());
     for (size_t i = 0; i < cachedRecovery.targets().size(); ++i) {
@@ -506 +524 @@
         emitStore(cachedRecovery, addressForNew(target));
         setNew(target, nullptr);
-    }
-    if (verbose)
-        dataLog("\n");
+        if (verbose)
+            dataLog("\n");
+    }
     cachedRecovery.clearTargets();
     if (!cachedRecovery.wantedJSValueRegs() && cachedRecovery.wantedFPR() == InvalidFPRReg)
@@ -607 +625 @@
     ASSERT(!isUndecided());
 
-    updateDangerFrontier();
+    initDangerFrontier();
 
     // First, we try to store any value that goes above the danger
@@ -703 +721 @@
     }
 
-#if USE(JSVALUE64)
-    if (m_tagTypeNumber != InvalidGPRReg && m_newRegisters[m_tagTypeNumber])
-        releaseGPR(m_tagTypeNumber);
-#endif
-
     // Handle 2) by loading all registers. We don't have to do any
     // writes, since they have been taken care of above.
+    // Note that we need m_tagTypeNumber to remain locked to box wanted registers.
     if (verbose)
         dataLog("  Loading wanted registers into registers\n");
@@ -743 +757 @@
     // We need to handle 4) first because it implies releasing
     // m_newFrameBase, which could be a wanted register.
+    // Note that we delay setting the argument count register as it needs to be released in step 3.
     if (verbose)
         dataLog("   * Storing the argument count into ", VirtualRegister { CallFrameSlot::argumentCount }, "\n");
+
     m_jit.store32(MacroAssembler::TrustedImm32(0),
         addressForNew(VirtualRegister { CallFrameSlot::argumentCount }).withOffset(TagOffset));
-    m_jit.store32(MacroAssembler::TrustedImm32(argCount()),
-        addressForNew(VirtualRegister { CallFrameSlot::argumentCount }).withOffset(PayloadOffset));
+
+#if USE(JSVALUE64)
+    if (!m_argumentsInRegisters) {
+#endif
+        m_jit.store32(MacroAssembler::TrustedImm32(argCount()),
+            addressForNew(VirtualRegister { CallFrameSlot::argumentCount }).withOffset(PayloadOffset));
+#if USE(JSVALUE64)
+    }
+#endif
 
     if (!isSlowPath()) {
@@ -768 +791 @@
         emitDisplace(*cachedRecovery);
     }
+
+#if USE(JSVALUE64)
+    // For recoveries with multiple register targets, copy the contents of the first target to the
+    // remaining targets.
+    for (Reg reg = Reg::first(); reg <= Reg::last(); reg = reg.next()) {
+        CachedRecovery* cachedRecovery { m_newRegisters[reg] };
+        if (!cachedRecovery || cachedRecovery->gprTargets().size() < 2)
+            continue;
+
+        GPRReg sourceGPR = cachedRecovery->gprTargets()[0].gpr();
+        for (size_t i = 1; i < cachedRecovery->gprTargets().size(); i++)
+            m_jit.move(sourceGPR, cachedRecovery->gprTargets()[i].gpr());
+    }
+
+    if (m_argumentsInRegisters)
+        m_jit.move(MacroAssembler::TrustedImm32(argCount()), argumentRegisterForArgumentCount());
+#endif
 }