[webkit-reviews] review requested: [Bug 127071] CStack Branch: X86-32 Fix LLInt : [Attachment 221432] Updated patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has asked  for review:
Bug 127071: CStack Branch: X86-32 Fix LLInt
https://bugs.webkit.org/show_bug.cgi?id=127071

Attachment 221432: Updated patch
https://bugs.webkit.org/attachment.cgi?id=221432&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #3)
> (From update of attachment 221312 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=221312&action=review
> 
> Michael pointed out that the ABI requirement is to have the sp aligned before
the call instruction, and the cfr as a result will be off by 8 (and need not be
aligned).  Updating / adding some of the review comments based on this new
info.  The other review comments still stand.
> >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:210
> >> +	  callToJavaScriptPrologue()
> > 
> > See comments for callToJavaScriptPrologue() in LowLevelInterpreter.asm
below.	Add the “subp 8, sp” after this line with the comment about this being
the alignment adjustment for the VMEntrySentinelFrame which is only 5 Registers
in size.
> 
> I'm not sure if we need to do any adjustments here.  It depends on what makes
sense to go into pushCalleeSaves().  One requirement is that the sp needs to be
aligned by the time we make the call to _llint_throw_stack_overflow_error()
below.

Changed the move cfr, sp to subp cfr, 8, sp.

> > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2111
> >	     loadp MarkedBlock::m_weakSet + WeakSet::m_vm[t3], t3
> 
> BTW, just above this code, the "load ScopeChain[cfr], t3" is redundant.  The
ScopeChain is already in t1 (loaded in the instructions above it).  You can
remove the "loadp ScopeChain[cfr], t3" and use t1 instead in the 2 andp and
loadp instructions for loading the VM*.
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:-2110
> > -	     subp 16 - 8, sp
> 
> You need to push the cfr here as well.  Hence:
> 
> subp 12, sp
> push cfr

Actually we just did a functionPrologue above, so that isn't needed.

> >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:-2114
> >> -	      addp 16 - 8, sp
> > 
> > The native function expects 1 ExecState* arg.  Hence, before the call, we
need to: subtract the sp by 4 for alignment, and push the cfr.	With this 2 and
the pushed return PC and cfr to follow, the sp will be aligned again in the
callee.  Similarly after the call returns, we need to add 8 to the sp.
> 
> Here, you'll need:
> addp 16, sp

Not needed per prior comment.

> >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:319
> >> +	  end
> > 
> > You should make this part of pushCalleeSaves instead of doing it here. 
Either that or we get rid of pushCalleeSaves and push the registers explicitly
here.  Otherwise, it’s one step removed to be able to see whether we have
retained stack alignment or not.
> > 
> > At first, looking at this code, I thought we need to "subp 4, sp” instead
of “subp 12, sp” because we pushed 3 callee saved regs bringing the total to 12
and needing 4 more bytes for alignment.  And then I realized that we need to
add another 8 to compensate for the fact that the 32-bit CallFrameHeader only
has 5 Registers in size and that we need to push a VMEntrySentinelFrame soon.
> > 
> > The pushCalleeSaves number of words may vary architecture to architecture,
but we’ll always want to subtract the sp by 8 here (for ARM and other 32-bit
archs as well) to compensate for the CallFrameHeader being only 5 Registers in
size and that we need the stack aligned after we push the VMEntrySentinelFrame.
 So, here’s what I propose we do to make things more sane:
> > 
> > 1. After we push cfr, the sp should be aligned (as one would expect in the
ABI).
> > 2. After we push the callee saved registers, let’s keep the sp aligned. 
Hence, in pushCalleeSaves for X86, let’s subtract another 4 bytes from sp.
> > 3. In doCallToJavaScript(), subtract the sp by 8 there before pushing the
VMEntrySentinelFrame.  Also add a comment to explain that that 8 bytes
adjustment is needed because we need to keep the sp aligned after we push the
VMEntrySentinelFrame and the VmEntrySentinelFrame being of the size of a
CallFrameHeader is 8 bytes short of alignment.
> > 
> > This way, it’ll be easier to reason about the stack alignment at each step.
 Each part takes care of their own requirement.  And if pushCalleeSaves vary
for different ports, they only need to worry about their own number of saved
registers and align that.
> 
> Since there is no requirement on the alignment of the cfr, the only
requirement is that the sp is aligned by the time you get to the call to
llint_throw_stack_overflow_error() in doCallToJavaScriptCore().  I think you
should still do all the adjustments in pushCalleeSaves and document the reasons
for any padding you add there for alignment.
>
> Thinking about this some more, I feel that we should have pushCalleeSaves as
a macro in the LLINT asm files.  Currently, having to look into the offline asm
files (in order to figure out what pushCalleeSaves does) makes it harder to
reason about whether we've met the alignment requirement or not.  Can you
retire the pushCalleeSaves and popCalleeSaves instructions and replace them
with LLINT macros instead?

It probably makes sense to break out pushCalleeSaves as individual pushes and
corresponding pops for popCalleeSaves.	That is probably best deferred to after
landing on trunk as there exists the prerequisite changes to ARM64 push and pop
working on pairs of registers.	Files
<https://bugs.webkit.org/show_bug.cgi?id=127155> to track this.

> >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:329
> >> +
> > 
> > You should do this as part of popCalleeSaves. Same as above, add 4 instead
of 12.
> 
> Same … do this as part of a LLINT popCalleeSaves() macro, but I'm not sure
about the amount to align yet.	Need to see what the resultant
pushCalleeSaves() and popCalleeSaves() macros look like.