Bug 126020 – CStack Branch: Fix call eval in baseline JIT

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

Currently the baseline JIT doesn't handle eval calls due to the stack pointer not being set up for the callee.

[Michael Saboff]

Created attachment 219684 [details]
Patch

[Geoffrey Garen]

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.

[Geoffrey Garen]

Comment on attachment 219684 [details]
Patch

r=me because this patch looks not incorrect, but I think it needs some refinement.

[Michael Saboff]

(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.

[Michael Saboff]

Committed r160867: <http://trac.webkit.org/changeset/160867>

[Geoffrey Garen]

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.

[Michael Saboff]

(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.

[Geoffrey Garen]

> 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.

[Michael Saboff]

(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.