[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 #233450|review?                     |review-
               Flag|                            |




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

There's a chance that this patch could work and that the errors in stack pointer adjustments only resulted in a some wasted slots (based on my educated guess of what is happening here).  However, I'd rather not approve this patch based on my guesses.  Please fix the math on all the stack pointer adjustments, or explain what I'm missing.  Thanks.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:157
> +        // JIT relies on that the CallerFrame (frame pointer) is put on the stack,

/is put on the stack,/being on the stack./

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:166
> +        // We also need to allocate the shadow space on the stack for the 4 parameter registers.
> +        // Also, to avoid the potential problem that the callee will overwrite the stored frame pointer
> +        // when it copies it's parameter registers into the shadow space, we should allocate another 32 bytes.
> +        // Note: POKE_ARGUMENT_OFFSET is adjusted accordingly.
> +        sub64(TrustedImm32(8 * sizeof(int64_t)), X86Registers::esp);

I'm still not sure if I understand what you're trying to do here.  Here's what I see needs to be done:

1. Allocate space for the CallerFrame pointer and return address (though not populated) [2 slots]
2. Allocate space for the shadow space for the 4 parameter registers [4 slots]

Each of these slots are 64-bits.  Hence, the stack pointer is still 64-bit (16 byte) aligned after "pushing" the 6 slots.  It doesn't look like you there's any alignment padding needed.

So, I don't understand the following:
1. why are you allocating 8 slots?
2. You said that the callee may copy the parameter registers into the shadow space.  Isn't that what the 4 slots is for?

There are 2 slots that are unexplained and unaccounted for here.  Can you please explain what you're trying to achieve here (with the 2 slots)?  If I've misunderstood your reasoning in my comments above, please correct me.  Thanks.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:333
> +        # We need to allocate 48 bytes on the stack; 32 bytes for the shadow space, and 8 bytes for the frame pointer we put on the stack,
> +        # and 8 bytes for the function return address (which isn't really needed, so we only make room for it here).
> +        subp 48, sp

This is wrong.  It should be "subp 32, sp".

If you look at the "else" case below, you'll see that it adds 2 slots to the sp.  The reason for this is because the sp is already pointing to the base of a CallFrame, which starts with a CallerFrame pointer followed by a return address.  When we call the host function here, we're expecting the callee to push the return address followed by the CallerFrame pointer.  That is why we add 16 (i.e. 2 slots) to the sp.

In your case, the callee will not store the return address nor the CallerFrame pointer.  Hence, there's no need for the "addp 16, sp" adjustment.  It is sufficient to store the cfr in [sp] as you have done above.  Thereafter, you only need to "sub 32, sp" to make space for the shadow space.  There's no need for another 16 bytes there.  This appears to be the same logic issue with your other adjustments above.  Please correct me if I've misunderstood something here.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:335
> +        addp 48, sp

"addp 32, sp" for reasons stated above.

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