Changeset 173282 in webkit

Timestamp:

Sep 4, 2014 2:23:38 PM (10 years ago)

Author:

msaboff@apple.com

Message:

REGRESSION(r173031): crashes during run-layout-jsc on x86/Linux
​https://bugs.webkit.org/show_bug.cgi?id=136436

Reviewed by Geoffrey Garen.

Instead of trying to calculate a stack pointer that allows for possible
stacked argument space, just use the "home" stack pointer location.
That stack pointer provides space for the worst case number of stacked
arguments on architectures that use stacked arguments. It also provides
stack space so that the return PC and caller frame pointer that are stored
as part of making the call to operationCallEval will not override any part
of the callee frame created on the stack.

Changed compileCallEval() to use the stackPointer value of the calling
function. That stack pointer is calculated to have enough space for
outgoing stacked arguments. By moving the stack pointer to its "home"
position, the caller frame and return PC are not set as part of making
the call to operationCallEval(). Moved the explicit setting of the
callerFrame field of the callee CallFrame from operationCallEval() to
compileCallEval() since it has been the artifact of making a call for
most architectures. Simplified the exception logic in compileCallEval()
as a result of the change. To be compliant with the stack state
expected by virtualCallThunkGenerator(), moved the stack pointer to
point above the CallerFrameAndPC of the callee CallFrame.

• jit/JIT.h: Changed callOperationNoExceptionCheck(J_JITOperation_EE, ...)

to callOperation(J_JITOperation_EE, ...) as it now can do a typical exception
check.

• jit/JITCall.cpp & jit/JITCall32_64.cpp:

(JSC::JIT::compileCallEval): Use the home stack pointer when making the call
to operationCallEval. Since the stack pointer adjustment no longer needs
to be done after making the call to operationCallEval(), the exception check
logic can be simplified.
(JSC::JIT::compileCallEvalSlowCase): Restored the stack pointer to point
to above the calleeFrame as this is what the generated thunk expects.

• jit/JITInlines.h:

(JSC::JIT::callOperation): Refactor of callOperationNoExceptionCheck
with the addition of a standard exception check.
(JSC::JIT::callOperationNoExceptionCheck): Deleted.

• jit/JITOperations.cpp:

(JSC::operationCallEval): Eliminated the explicit setting of caller frame
as that is now done in the code generated by compileCallEval().

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• jit/JIT.h (modified) (2 diffs)
• jit/JITCall.cpp (modified) (2 diffs)
• jit/JITCall32_64.cpp (modified) (2 diffs)
• jit/JITInlines.h (modified) (2 diffs)
• jit/JITOperations.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-09-04  Michael Saboff  <msaboff@apple.com>
+
+        REGRESSION(r173031): crashes during run-layout-jsc on x86/Linux
+        https://bugs.webkit.org/show_bug.cgi?id=136436
+
+        Reviewed by Geoffrey Garen.
+
+        Instead of trying to calculate a stack pointer that allows for possible
+        stacked argument space, just use the "home" stack pointer location.
+        That stack pointer provides space for the worst case number of stacked
+        arguments on architectures that use stacked arguments.  It also provides
+        stack space so that the return PC and caller frame pointer that are stored
+        as part of making the call to operationCallEval will not override any part
+        of the callee frame created on the stack.
+
+        Changed compileCallEval() to use the stackPointer value of the calling
+        function.  That stack pointer is calculated to have enough space for
+        outgoing stacked arguments.  By moving the stack pointer to its "home"
+        position, the caller frame and return PC are not set as part of making
+        the call to operationCallEval().  Moved the explicit setting of the
+        callerFrame field of the callee CallFrame from operationCallEval() to
+        compileCallEval() since it has been the artifact of making a call for
+        most architectures.  Simplified the exception logic in compileCallEval()
+        as a result of the change.  To be compliant with the stack state
+        expected by virtualCallThunkGenerator(), moved the stack pointer to
+        point above the CallerFrameAndPC of the callee CallFrame.
+
+        * jit/JIT.h: Changed callOperationNoExceptionCheck(J_JITOperation_EE, ...)
+        to callOperation(J_JITOperation_EE, ...) as it now can do a typical exception
+        check.
+        * jit/JITCall.cpp & jit/JITCall32_64.cpp:
+        (JSC::JIT::compileCallEval): Use the home stack pointer when making the call
+        to operationCallEval.  Since the stack pointer adjustment no longer needs
+        to be done after making the call to operationCallEval(), the exception check
+        logic can be simplified.
+        (JSC::JIT::compileCallEvalSlowCase): Restored the stack pointer to point
+        to above the calleeFrame as this is what the generated thunk expects.
+        * jit/JITInlines.h:
+        (JSC::JIT::callOperation): Refactor of callOperationNoExceptionCheck
+        with the addition of a standard exception check.
+        (JSC::JIT::callOperationNoExceptionCheck): Deleted.
+        * jit/JITOperations.cpp:
+        (JSC::operationCallEval): Eliminated the explicit setting of caller frame
+        as that is now done in the code generated by compileCallEval().
+
 2014-09-03  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/jit/JIT.h

@@ -719 +719 @@
         MacroAssembler::Call callOperation(V_JITOperation_ECC, RegisterID, RegisterID);
         MacroAssembler::Call callOperation(V_JITOperation_ECICC, RegisterID, const Identifier*, RegisterID, RegisterID);
+        MacroAssembler::Call callOperation(J_JITOperation_EE, RegisterID);
         MacroAssembler::Call callOperation(V_JITOperation_EIdJZ, const Identifier*, RegisterID, int32_t);
         MacroAssembler::Call callOperation(V_JITOperation_EJ, RegisterID);
@@ -739 +740 @@
         MacroAssembler::Call callOperation(V_JITOperation_EZ, int32_t);
         MacroAssembler::Call callOperationWithCallFrameRollbackOnException(J_JITOperation_E);
-        MacroAssembler::Call callOperationNoExceptionCheck(J_JITOperation_EE, RegisterID);
         MacroAssembler::Call callOperationWithCallFrameRollbackOnException(V_JITOperation_ECb, CodeBlock*);
         MacroAssembler::Call callOperationWithCallFrameRollbackOnException(Z_JITOperation_E);

trunk/Source/JavaScriptCore/jit/JITCall.cpp

@@ -137 +137 @@
 {
     addPtr(TrustedImm32(-static_cast<ptrdiff_t>(sizeof(CallerFrameAndPC))), stackPointerRegister, regT1);
-    callOperationNoExceptionCheck(operationCallEval, regT1);
-
-    Jump noException = emitExceptionCheck(InvertedExceptionCheck);
-    addPtr(TrustedImm32(stackPointerOffsetFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister);    
-    exceptionCheck(jump());
-
-    noException.link(this);
-    addSlowCase(branch64(Equal, regT0, TrustedImm64(JSValue::encode(JSValue()))));
+    storePtr(callFrameRegister, Address(regT1, CallFrame::callerFrameOffset()));
 
     addPtr(TrustedImm32(stackPointerOffsetFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister);
     checkStackPointerAlignment();
 
+    callOperation(operationCallEval, regT1);
+
+    addSlowCase(branch64(Equal, regT0, TrustedImm64(JSValue::encode(JSValue()))));
+
     sampleCodeBlock(m_codeBlock);
     
@@ -157 +154 @@
 {
     linkSlowCase(iter);
+    int registerOffset = -instruction[4].u.operand;
+
+    addPtr(TrustedImm32(registerOffset * sizeof(Register) + sizeof(CallerFrameAndPC)), callFrameRegister, stackPointerRegister);
 
     load64(Address(stackPointerRegister, sizeof(Register) * JSStack::Callee - sizeof(CallerFrameAndPC)), regT0);

trunk/Source/JavaScriptCore/jit/JITCall32_64.cpp

@@ -221 +221 @@
 {
     addPtr(TrustedImm32(-static_cast<ptrdiff_t>(sizeof(CallerFrameAndPC))), stackPointerRegister, regT1);
-
-    callOperationNoExceptionCheck(operationCallEval, regT1);
-
-    Jump noException = emitExceptionCheck(InvertedExceptionCheck);
+    storePtr(callFrameRegister, Address(regT1, CallFrame::callerFrameOffset()));
+
     addPtr(TrustedImm32(stackPointerOffsetFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister);
-    exceptionCheck(jump());
-
-    noException.link(this);
+
+    callOperation(operationCallEval, regT1);
+
     addSlowCase(branch32(Equal, regT1, TrustedImm32(JSValue::EmptyValueTag)));
 
-    addPtr(TrustedImm32(stackPointerOffsetFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister);
-    checkStackPointerAlignment();
-
     sampleCodeBlock(m_codeBlock);
     
@@ -242 +237 @@
 {
     linkSlowCase(iter);
+
+    int registerOffset = -instruction[4].u.operand;
+
+    addPtr(TrustedImm32(registerOffset * sizeof(Register) + sizeof(CallerFrameAndPC)), callFrameRegister, stackPointerRegister);
 
     loadPtr(Address(stackPointerRegister, sizeof(Register) * JSStack::Callee - sizeof(CallerFrameAndPC)), regT0);

trunk/Source/JavaScriptCore/jit/JITInlines.h

@@ -318 +318 @@
 }
 
+ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(J_JITOperation_EE operation, RegisterID regOp)
+{
+    setupArgumentsWithExecState(regOp);
+    updateTopCallFrame();
+    return appendCallWithExceptionCheck(operation);
+}
+
 ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(V_JITOperation_EPc operation, Instruction* bytecodePC)
 {
@@ -334 +341 @@
     setupArgumentsExecState();
     return appendCallWithCallFrameRollbackOnException(operation);
-}
-
-ALWAYS_INLINE MacroAssembler::Call JIT::callOperationNoExceptionCheck(J_JITOperation_EE operation, RegisterID regOp)
-{
-    setupArgumentsWithExecState(regOp);
-    updateTopCallFrame();
-    return appendCall(operation);
 }
 

trunk/Source/JavaScriptCore/jit/JITOperations.cpp

@@ -613 +613 @@
     execCallee->setScope(exec->scope());
     execCallee->setCodeBlock(0);
-    execCallee->setCallerFrame(exec);
 
     if (!isHostFunction(execCallee->calleeAsValue(), globalFuncEval))