Bug 148564 - ScratchRegisterAllocator::preserveReusedRegistersByPushing() should allow room for C helper calls and keep sp properly aligned.
Summary: ScratchRegisterAllocator::preserveReusedRegistersByPushing() should allow roo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 149030
  Show dependency treegraph
 
Reported: 2015-08-27 23:29 PDT by Mark Lam
Modified: 2015-09-09 19:58 PDT (History)
7 users (show)

See Also:


Attachments
the fix. (12.08 KB, patch)
2015-08-28 01:16 PDT, Mark Lam
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-08-27 23:29:20 PDT
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.
Comment 1 Mark Lam 2015-08-28 01:16:57 PDT
Created attachment 260136 [details]
the fix.
Comment 2 Mark Lam 2015-08-28 01:24:50 PDT
<rdar://problem/22218598>
Comment 3 Saam Barati 2015-08-28 10:48:32 PDT
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.
Comment 4 Mark Lam 2015-08-28 10:53:45 PDT
(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>.
Comment 5 Filip Pizlo 2015-08-28 12:01:01 PDT
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).
Comment 6 Mark Lam 2015-08-28 12:51:17 PDT
(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>.
Comment 7 Filip Pizlo 2015-09-09 19:56:13 PDT
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.