[webkit-reviews] review denied: [Bug 124409] [Win] JavaScript crashes on 64-bit with JIT enabled. : [Attachment 217049] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied peavo at outlook.com's request for
review:
Bug 124409: [Win] JavaScript crashes on 64-bit with JIT enabled.
https://bugs.webkit.org/show_bug.cgi?id=124409

Attachment 217049: Patch
https://bugs.webkit.org/attachment.cgi?id=217049&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217049&action=review


Thanks for the work.  Looking pretty good.  It needs a couple of changes.
In addition to what is noted inline, the value that we sub/add to sp in
callToJavaScript / returnFromJavaScript need to be adjusted for the additional
pushes of rbi/rdi and for the space needed to make calls out.  The value should
be large enough for the space needed to call AND result in a 32 byte aligned
SP.  I think that means the new value should be 38h.   In addition to changing
28h -> 38h, update the comment to include that the calling convention requires
space for 4 Dwords.

> Source/JavaScriptCore/jit/JITInlines.h:123
> +
> +#if OS(WINDOWS) && CPU(X86_64)
> +    // According to Windows 64-bit ABI, we need to allocate space on the
stack for the 4 argument registers.
> +    subPtr(TrustedImm32(32), stackPointerRegister);
> +#endif
> +

Instead of making space here, we should increase the amount that we decrement
from SP in callToJavaScript.

> Source/JavaScriptCore/jit/JITInlines.h:142
> +
> +#if OS(WINDOWS) && CPU(X86_64)
> +    // According to Windows 64-bit ABI, we need to allocate space on the
stack for the 4 argument registers.
> +    subPtr(TrustedImm32(32), stackPointerRegister);
> +#endif
> +

Ditto.

> Source/JavaScriptCore/jit/JITStubsMSVC64.asm:36
> -    mov rbp, rax ; Save previous frame pointer
> +    mov rax, rbp ; Save previous frame pointer

This fix was taken care of in r159290:
<http://trac.webkit.org/changeset/159290>