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

[bugzilla-daemon at webkit.org]

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





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

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:159
> +        // We also need to allocate space on the stack for the 4 parameter registers.

nit: s/space/the shadow space/ to match MSVC documentation which calls it "shadow space".

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:161
> +        // TODO: Should we also put the return address next to the frame pointer on the stack?
> +        // The call instruction will not put the return address at the expected place because of the 32 byte stack allocation.

I think we don't need this.  We only keep that convention so that tools can walk the stack.  In this case, it doesn't look like MSVC relies on that anyway since the frame pointer is optional.  Let's remove this comment.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:162
> +        store64(X86Registers::ebp, Address(X86Registers::esp, -16));

Why do we need to preserve the ebp here?  According to http://msdn.microsoft.com/en-us/library/6t169e9c.aspx, ebp is non-volatile.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:163
> +        sub64(TrustedImm32(32), X86Registers::esp);

nit: can you use 4 * sizeof(int64_t) instead of 32?  I think it's more descriptive that way.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:168
> +        add64(TrustedImm32(32), X86Registers::esp);

This only accounts for the 4 shadow space slows for the args above.  Don't you have to also pop the 16 bytes for the ebp, and restore the ebp?  If we don't need to save the ebp above, then there's no issue here.

Also, I'd prefer it if you use 4 * sizeof(int64_t) instead of 32.

> Source/JavaScriptCore/jit/JITStubsMSVC64.asm:35
> +    ; Allocate space for all 4 parameter registers, and align stack pointer to 16 bytes.
> +    sub rsp, 40

I couldn't find any MSVC documentation that describes how the stack must be aligned.  I understand the first 32 bytes is for the arg registers shadow space, but Is the extra 8 byte padding here really appropriate and necessary? Did you encounter an issue without this padding?

> Source/JavaScriptCore/jit/JITStubsMSVC64.asm:37
> +    add rsp, 40

ditto.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:404
> +    jit.subPtr(JSInterfaceJIT::TrustedImm32(32), JSInterfaceJIT::stackPointerRegister);

nit: In nativeForGenerator() above, you used 4 * sizeof(int64_t), which I thought is more descriptive than a literal 32 here.  Would you mind changing this to say 4 * sizeof(int64_t) instead?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:413
> +    jit.addPtr(JSInterfaceJIT::TrustedImm32(32), JSInterfaceJIT::stackPointerRegister);

ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:71
> +        subp 80, sp

32 (args shadow space) + 16 (return value) ==> 48.  Why 80?  What am I missing?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:73
> +        addp 48, t2

Why 48?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:327
> +    elsif X86_64_WIN
> +        # On Win64 we need to manually copy the frame pointer to the stack, since MSVC may not maintain a frame pointer on 64-bit.
> +        # See http://msdn.microsoft.com/en-us/library/9z1stfyw.aspx where it's stated that rbp MAY be used as a frame pointer.
> +        # Also, we need to manually copy the return address to the stack, since before the call instruction we allocated space
> +        # for the 4 parameter registers, and the call instruction won't put the return address at the correct stack location.
> +        move sp, t2
> +        storep cfr, [sp]
> +        call .copyReturnAddressToStack
> +.copyReturnAddressToStack:
> +        pop 8[sp]
> +        # Adjust the return address with the offset to the instruction after the call instruction.
> +        addp 16, 8[sp]

Apart from the "move sp, t2", why do we need to do this?   The rbp is a non-volatile register even if it's not used as a frame pointer.

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