Currently the baseline JIT doesn't handle eval calls due to the stack pointer not being set up for the callee.
Created attachment 219684 [details] Patch
Comment on attachment 219684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219684&action=review r=me > Source/JavaScriptCore/jit/JITCall.cpp:126 > + addPtr(TrustedImm32(JSStack::CallerFrameAndPCSize * static_cast<int>(sizeof(Register))), regT1, stackPointerRegister); Why do we need to move SP like this before the call to operationCallEval? I thought that calls to helper functions didn't need to adjust the stack. > Source/JavaScriptCore/jit/JITCall.cpp:130 > + addPtr(TrustedImm32(-frameRegisterCountFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister); You should use stackPointerOffsetFor() here. But same question here: why are we adjusting SP? > Source/JavaScriptCore/jit/JITCall.cpp:145 > + addPtr(TrustedImm32(JSStack::CallerFrameAndPCSize * static_cast<int>(sizeof(Register))), regT1, stackPointerRegister); Why do we need to move SP like this before the call to operationCallEval? > Source/JavaScriptCore/jit/JITCall.cpp:147 > + addPtr(TrustedImm32(-frameRegisterCountFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister); You should use stackPointerOffsetFor() here.
Comment on attachment 219684 [details] Patch r=me because this patch looks not incorrect, but I think it needs some refinement.
(In reply to comment #2) > (From update of attachment 219684 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219684&action=review > > r=me > > > Source/JavaScriptCore/jit/JITCall.cpp:126 > > + addPtr(TrustedImm32(JSStack::CallerFrameAndPCSize * static_cast<int>(sizeof(Register))), regT1, stackPointerRegister); > > Why do we need to move SP like this before the call to operationCallEval? I thought that calls to helper functions didn't need to adjust the stack. We need to move the stack down because that is where the calleeFrame is. regT1 is the calleeFrame. This is just like the normal call processing. > > Source/JavaScriptCore/jit/JITCall.cpp:130 > > + addPtr(TrustedImm32(-frameRegisterCountFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister); > > You should use stackPointerOffsetFor() here. I will make the change. > But same question here: why are we adjusting SP? Same reason. > > Source/JavaScriptCore/jit/JITCall.cpp:145 > > + addPtr(TrustedImm32(JSStack::CallerFrameAndPCSize * static_cast<int>(sizeof(Register))), regT1, stackPointerRegister); > > Why do we need to move SP like this before the call to operationCallEval? Ditto. > > Source/JavaScriptCore/jit/JITCall.cpp:147 > > + addPtr(TrustedImm32(-frameRegisterCountFor(m_codeBlock) * sizeof(Register)), callFrameRegister, stackPointerRegister); > > You should use stackPointerOffsetFor() here. Will do.
Committed r160867: <http://trac.webkit.org/changeset/160867>
I see: operationCallEval is a helper function, which normally wouldn't adjust the stack for, but it calls back into the VM, so we do want to adjust the stack as if we were calling another JS function.
(In reply to comment #6) > I see: operationCallEval is a helper function, which normally wouldn't adjust the stack for, but it calls back into the VM, so we do want to adjust the stack as if we were calling another JS function. Actually we have to adjust even if it didn't call back into the VM. Otherwise the locals in operationCallEval() and its callees will step on the callee frame.
> Actually we have to adjust even if it didn't call back into the VM. Otherwise the locals in operationCallEval() and its callees will step on the callee frame. The adjustment moves SP up before calling operationCallEval(). How does that prevent anything from being stepped on? It seems to do the opposite, and expose two slots to be stepped on by operationCallEval()'s ReturnPC and ReturnBP.
(In reply to comment #8) > > Actually we have to adjust even if it didn't call back into the VM. Otherwise the locals in operationCallEval() and its callees will step on the callee frame. > > The adjustment moves SP up before calling operationCallEval(). How does that prevent anything from being stepped on? It seems to do the opposite, and expose two slots to be stepped on by operationCallEval()'s ReturnPC and ReturnBP. My bad on the prior comment. Your comment #7 is right.