[webkit-reviews] review denied: [Bug 130638] [Win64] ASM LLINT is not enabled. : [Attachment 233634] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 23 16:16:33 PDT 2014
Mark Lam <mark.lam at apple.com> has denied peavo at outlook.com's request for
Bug 130638: [Win64] ASM LLINT is not enabled.
Attachment 233634: Patch
------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233634&action=review
> + // We also need to allocate the shadow space on the stack for the 4
> + // Also, we should allocate 16 bytes for the frame pointer, and
return address (not populated).
> + // In addition, we need to allocate 16 bytes for two more
parameters, since the call can have up to 6 parameters.
> + // Note: POKE_ARGUMENT_OFFSET is adjusted accordingly.
> + sub64(TrustedImm32(8 * sizeof(int64_t)), X86Registers::esp);
I've thought thru this some more, and realized that this is actually wrong.
See my comment on POKE_ARGUMENT_OFFSET below for details. In summary, the
space for the parameter shadow space should have already been reserved in
maxFrameExtentForSlowPathCall in assembler/MaxFrameExtentForSlowPathCall.h, and
it appears to be already reserved there. The calls to the various
setupArguments() (defined in CCallHelpers.h) will poke arguments into the space
that is reserved by maxFrameExtentForSlowPathCall before we store the
CallerFrame pointer above.
The issue that we're really trying to solve here is that the callee is not
expecting the CallerFrame pointer to be on stack. Hence, after "pushing" the
CallerFrame pointer, we need to replicate the arguments that are being passed
to the callee. For the first 4 arguments, this is a simple matter of reserving
some space. For the 5th and 6th argument, we need to copy them here or the
callee will see junk instead.
However, we don't actually know how many arguments are being passed for this
call. Fortunately, we can just be conservative and reserve the worst case
amount of space needed. That said, we still need to conservatively copy the
5th and 6th arguments over as if they exists. Please add that code.
> -#if CPU(MIPS) || (OS(WINDOWS) && CPU(X86_64))
> +#if CPU(MIPS)
> #define POKE_ARGUMENT_OFFSET 4
> +#elif CPU(X86_64) && OS(WINDOWS)
> +#define POKE_ARGUMENT_OFFSET -4
This is wrong. Please undo.
At the moment when the various setupArgument()s are called, the sp should be
pointing to a spot on the stack where we're ready to push the callee's return
address and previous frame pointer:
| prev frame* goes here |
| ret addr goes here |
| | <---- sp points here
| reserved space |
| for max callee |
| args & other |
| stuff. |
| | ^
|---------------------| | Stack grows in this direction.
| Current JS frame | |
In the Win64 case, this means POKE_ARGUMENT_OFFSET should be 4 to reserve 4
slots for the parameter shadow space. When we "push" the 5th argument, it gets
poke'd to sp + POKE_ARGUMENT_OFFSET i.e. sp as expected.
> + elsif X86_64_WIN
> + # Note: this implementation is only correct if the return type size
is > 8 bytes.
> + # See macro cCall2Void for an implementation when the return type <=
> + # On Win64, when the return type is larger than 8 bytes, we need to
allocate space on the stack for the return value.
> + # On entry rcx (t2), should contain a pointer to this stack space.
The other parameters are shifted to the right,
> + # rdx (t1) should contain the first argument, and r8 (t6) should
contain the second argument.
> + # On return, rax contains a pointer to this stack value, and we then
need to copy the 16 byte return value into rax (t0) and rdx (t1)
> + # since the return value is expected to be split between the two.
> + # See http://msdn.microsoft.com/en-us/library/7572ztz4.aspx
> + move arg1, t1
> + move arg2, t6
> + subp 48, sp
> + move sp, t2
> + addp 32, t2
> + call function
> + addp 48, sp
> + move 8[t0], t1
> + move [t0], t0
cCall2() is used to call slow path functions that may recurse and call JS
functions again. Doesn't that mean you also need to write the CallerFrame
pointer to the stack here?
As for cCall2Void() below, a cursory scan of its callers tells me that we don't
currently use it to call something that can recurse into JS. But we don't know
if we may in the future. Do you think it needs to store the CallerFrame
pointer there too?
More information about the webkit-reviews