Bug 148564 – ScratchRegisterAllocator::preserveReusedRegistersByPushing() should allow room for C helper calls and keep sp properly aligned.

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

[Mark Lam]

ScratchRegisterAllocator::preserveReusedRegistersByPushing() pushes registers on the stack in order to preserve them.  But emitPutTransitionStub() which uses preserveReusedRegistersByPushing() may also emit a call to a C helper function to flush the heap write barrier buffer.  The code for emitting C helper calls expects the stack pointer (sp) to already be pointing to a location on the stack where there's adequate space reserved for storing the arguments that the C helper expects, and that space is expected to be at the top of the stack.  Hence, there is a conflict of expectations.  As a result, the arguments for the C helper will overwrite and corrupt the values that are pushed on the stack by preserveReusedRegistersByPushing().

In addition, JIT compiled functions always position the sp such that it will be aligned (according to platform ABI dictates) after a C call is made (i.e. after the frame pointer and return address is pushed on to the stack).  preserveReusedRegistersByPushing()'s arbitrary pushing of a number of saved register values may mess up this alignment.

The fix is to have preserveReusedRegistersByPushing(), after it has pushed the saved register values, adjust the sp to reserve an additional amount of stack space needed for C call helpers plus any padding needed to restore proper sp alignment.  The stack's ReservedZone will ensure that we have enough stack space for this.  ScratchRegisterAllocator::restoreReusedRegistersByPopping() also needs to updated to perform the complement of this behavior.

[Mark Lam]

Created attachment 260136 [details]
the fix.

[Mark Lam]

<rdar://problem/22218598>

[Saam Barati]

Comment on attachment 260136 [details]
the fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=260136&action=review

r=me

> Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp:121
> +    jit.subPtr(MacroAssembler::TrustedImm32(numberOfPaddingBytes + maxFrameExtentForSlowPathCall), MacroAssembler::stackPointerRegister);

You're adding maxFrameExtenetFowSlowPathCall twice: here and line 117.

> Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp:131
> +    jit.addPtr(MacroAssembler::TrustedImm32(numberOfPaddingBytes + maxFrameExtentForSlowPathCall), MacroAssembler::stackPointerRegister);

ditto. I think this should just be numberOfPaddingBytes.

[Mark Lam]

(In reply to comment #3)
> You're adding maxFrameExtenetFowSlowPathCall twice: here and line 117.

Thanks for catching that bug.  Fixed locally and landed in r189103: <http://trac.webkit.org/r189103>.

[Filip Pizlo]

Comment on attachment 260136 [details]
the fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=260136&action=review

> Source/JavaScriptCore/tests/stress/regress-148564.js:1
> +//@ run("regress", "--enableAccessInlining=false")

You shouldn't use the run() method directly, since doing so disables the FTL and enables concurrent JIT.  That means you're not guaranteed that "test()" will actually get compiled by the DFG, and it guarantees that it won't get compiled by the FTL.

It would have been better to use the existing "runNoCJITNoAccessInlining" method in this case, if you don't want FTL testing (which I'm assuming you don't).

[Mark Lam]

(In reply to comment #5)
> It would have been better to use the existing "runNoCJITNoAccessInlining"
> method in this case, if you don't want FTL testing (which I'm assuming you
> don't).

Applied this in a follow up patch in r189120: <http://trac.webkit.org/r189120>.

[Filip Pizlo]

I think that this was the wrong fix.  The bug here is that the barrier slow path isn't calling restoreReusedRegistersByPopping() prior to preserveUsedRegistersToScratchBufferForCall() the way that the allocation slow path does.