Bug 126093 – Repatch write barrier slow path call doesn't align the stack in the presence of saved registers

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

[Mark Hahnenberg]

This is sort of related to (and is blocked by) bug 126092, but it's related more to the alignment of the stack at the time of the call on x86. x86 needs two extra slots to store arguments during the call. It also needs SP to be 16-byte aligned after the call pushes the return address onto the stack. Right now we're just subtracting 3 * sizeof(void*), which gives us space for the arguments and the correct alignment assuming SP was correctly aligned to start with. Unfortunately, when the ScratchRegisterAllocator saves reused registers it breaks the correct starting alignment guarantee.

[Mark Hahnenberg]

Correction to the above: the stack must be aligned at the time of the call (i.e. 16-byte aligned, then return address is pushed).

[Mark Hahnenberg]

Created attachment 220535 [details]
Patch

[Geoffrey Garen]

Comment on attachment 220535 [details]
Patch

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

r=me

> Source/JavaScriptCore/jit/Repatch.cpp:798
> +    unsigned desiredAlignment = 16;

Please use StackAlignment.h instead.

> Source/JavaScriptCore/jit/Repatch.cpp:897
>  #if ENABLE(GGC)
> -    MacroAssembler::Call writeBarrierOperation = writeBarrier(stubJit, baseGPR, scratchGPR1, scratchGPR2, allocator);
> +    MacroAssembler::Call writeBarrierOperation = writeBarrier(stubJit, baseGPR, scratchGPR1, scratchGPR2, callFrameRegister, allocator);
>  #endif

For #ifdefs like this, I prefer to make the call site unconditional, and make the implementation of writeBarrier() conditional on #ENABLE(GGC). That removes distractions from the code.

[Mark Hahnenberg]

> For #ifdefs like this, I prefer to make the call site unconditional, and make the implementation of writeBarrier() conditional on #ENABLE(GGC). That removes distractions from the code.

My main reasoning for putting it here rather than in the body is to make so as much code compiles when GGC is disabled.

[Mark Hahnenberg]

(In reply to comment #4)
> My main reasoning for putting it here rather than in the body is to make so as much code compiles when GGC is disabled.

...as possible?

[Mark Hahnenberg]

Committed r161450: <http://trac.webkit.org/changeset/161450>