[Webkit-unassigned] [Bug 130638] [Win64] ASM LLINT is not enabled.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130638


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #233634|review?                     |review-
               Flag|                            |




--- Comment #55 from Mark Lam <mark.lam at apple.com>  2014-06-23 16:16:55 PST ---
(From update of attachment 233634)
View in context: https://bugs.webkit.org/attachment.cgi?id=233634&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:166
> +        // We also need to allocate the shadow space on the stack for the 4 parameter registers.
> +        // 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.

> Source/JavaScriptCore/jit/CCallHelpers.h:786
> -#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
>  #else

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[4] as expected.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:77
> +    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 <= 8 bytes.
> +        # 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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.