Changeset 209673 in webkit

Timestamp:

Dec 10, 2016, 1:04:05 PM (9 years ago)

Author:

msaboff@apple.com

Message:

REGRESSION(r209653) Crash in CallFrameShuffler::snapshot()
​https://bugs.webkit.org/show_bug.cgi?id=165728

Reviewed by Filip Pizlo.

JSTests:

New regression test.

• stress/regress-165728.js: Added.

(sum1):
(sum2):
(tailCaller):
(test):

Source/JavaScriptCore:

It can be the case that a JSValueReg's CachedRecovery is the source for mutliple
GPRs. We only store the CachedRecovery in one slot of m_newRegisters to simplify
the recovery process. This is also done for the case where the recovery source
and destination are the same GPR.

In light of this change, snapshot needs to be taught that one CacheRecovery is
the source for multiple registers. This is done by using a two step process.
First find all the argument CachedRecovery's and create a vector mapping all of
the target GPRs and the source recovery. Then use that vector to get the
recovery for each register.

• jit/CallFrameShuffler.h:

(JSC::CallFrameShuffler::snapshot):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/regress-165728.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/jit/CallFrameShuffler.h (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2016-12-10  Michael Saboff  <msaboff@apple.com>
+
+        REGRESSION(r209653) Crash in CallFrameShuffler::snapshot()
+        https://bugs.webkit.org/show_bug.cgi?id=165728
+
+        Reviewed by Filip Pizlo.
+
+        New regression test.
+
+        * stress/regress-165728.js: Added.
+        (sum1):
+        (sum2):
+        (tailCaller):
+        (test):
+
 2016-12-10  Keith Miller  <keith_miller@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-12-10  Michael Saboff  <msaboff@apple.com>
+
+        REGRESSION(r209653) Crash in CallFrameShuffler::snapshot()
+        https://bugs.webkit.org/show_bug.cgi?id=165728
+
+        Reviewed by Filip Pizlo.
+
+        It can be the case that a JSValueReg's CachedRecovery is the source for mutliple
+        GPRs. We only store the CachedRecovery in one slot of m_newRegisters to simplify
+        the recovery process. This is also done for the case where the recovery source
+        and destination are the same GPR.
+
+        In light of this change, snapshot needs to be taught that one CacheRecovery is
+        the source for multiple registers.  This is done by using a two step process.
+        First find all the argument CachedRecovery's and create a vector mapping all of
+        the target GPRs and the source recovery.  Then use that vector to get the
+        recovery for each register.
+
+        * jit/CallFrameShuffler.h:
+        (JSC::CallFrameShuffler::snapshot):
+
 2016-12-10  Keith Miller  <keith_miller@apple.com>
 

trunk/Source/JavaScriptCore/jit/CallFrameShuffler.h

@@ -114 +114 @@
         }
         data.args.resize(argCount());
+
+        Vector<ValueRecovery> registerArgRecoveries;
+#if USE(JSVALUE64)
+        // Find cached recoveries for all argument registers.
+        // We do this here, because a cached recovery may be the source for multiple
+        // argument registers, but it is only stored in one m_newRegister index.
+        if (data.argumentsInRegisters) {
+            unsigned maxArgumentRegister = std::min(static_cast<unsigned>(argCount()), NUMBER_OF_JS_FUNCTION_ARGUMENT_REGISTERS);
+            registerArgRecoveries.resize(maxArgumentRegister);
+            for (size_t i = 0; i < maxArgumentRegister; ++i) {
+                Reg reg { argumentRegisterForFunctionArgument(i) };
+                CachedRecovery* cachedRecovery { m_newRegisters[reg] };
+                if (cachedRecovery) {
+                    for (auto jsValueReg : cachedRecovery->gprTargets())
+                        registerArgRecoveries[jsFunctionArgumentForArgumentRegister(jsValueReg.gpr())] = cachedRecovery->recovery();
+                }
+            }
+        }
+#endif
+        
         for (size_t i = 0; i < argCount(); ++i) {
             if (argumentsLocation == StackArgs || i >= NUMBER_OF_JS_FUNCTION_ARGUMENT_REGISTERS)
@@ -119 +139 @@
             else {
                 Reg reg { argumentRegisterForFunctionArgument(i) };
-                CachedRecovery* cachedRecovery { m_newRegisters[reg] };
-                data.args[i] = cachedRecovery->recovery();
+                ASSERT(registerArgRecoveries[i]);
+                data.args[i] = registerArgRecoveries[i];
             }
         }
@@ -441 +461 @@
 #endif
 
-    // This stores, for each register, information about the recovery
-    // for the value that should eventually go into that register. The
-    // only registers that have a target recovery will be callee-save
-    // registers, as well as possibly one JSValueRegs for holding the
-    // callee.
+    // This stores information about the recovery for the value that
+    // should eventually go into that register. In some cases there
+    // are recoveries that have multiple targets. For those recoveries,
+    // only the first target register in the map has the recovery.
+    // We optimize the case where there are multiple targets for one
+    // recovery where one of those targets is also the source register.
+    // Restoring the first target becomes a nop and simplifies the logic
+    // of restoring the remaining targets.
     //
     // Once the correct value has been put into the registers, and
     // contrary to what we do with m_newFrame, we keep the entry in
     // m_newRegisters to simplify spilling.
+    //
+    // If a recovery has multiple target registers, we copy the value
+    // from the first target register to the remaining target registers
+    // at the end of the shuffling process.
     RegisterMap<CachedRecovery*> m_newRegisters;