Bug 126093

Summary: Repatch write barrier slow path call doesn't align the stack in the presence of saved registers
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 121074    
Attachments:
Description Flags
Patch ggaren: review+

Mark Hahnenberg
Reported 2013-12-20 16:16:35 PST
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.
Attachments
Patch (8.54 KB, patch)
2014-01-07 11:14 PST, Mark Hahnenberg 		ggaren: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1 2014-01-07 10:00:52 PST
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
Comment 2 2014-01-07 11:14:52 PST
Created attachment 220535 [details] Patch
Geoffrey Garen
Comment 3 2014-01-07 11:18:23 PST
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
Comment 4 2014-01-07 11:20:15 PST
> 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
Comment 5 2014-01-07 11:20:58 PST
(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
Comment 6 2014-01-07 13:01:37 PST
Committed r161450: <http://trac.webkit.org/changeset/161450>
Note You need to log in before you can comment on or make changes to this bug.