[webkit-reviews] review granted: [Bug 175724] Implement 64-bit MacroAssembler::probe support for Windows. : [Attachment 318761] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 175724: Implement 64-bit MacroAssembler::probe support for Windows.
https://bugs.webkit.org/show_bug.cgi?id=175724

Attachment 318761: Patch

https://bugs.webkit.org/attachment.cgi?id=318761&action=review




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 318761
  --> https://bugs.webkit.org/attachment.cgi?id=318761
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318761&action=review

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:8
> +	   This is needed to enable the DFG.

Please add a comment about why you went with putting the code in an asm file
instead of inline assembly.  We spoke offline about this, but I think it's good
to document the reason here in the ChangeLog.

> Source/JavaScriptCore/jit/JITStubsMSVC64.asm:48
> +PTR_SIZE EQU 8
> +
> +PROBE_PROBE_FUNCTION_OFFSET EQU (0 * PTR_SIZE)
> +PROBE_ARG_OFFSET EQU (1 * PTR_SIZE)
> +PROBE_INIT_STACK_FUNCTION_OFFSET EQU (2 * PTR_SIZE)
> +PROBE_INIT_STACK_ARG_OFFSET EQU (3 * PTR_SIZE)

It's unfortunate that MSVC does not allow us to use inline asm.  Apart from all
these duplicate definitions, we lost the ability to validate the correctness of
these values via the static_asserts.  Please put a comment above this blob of
constant defines to state that these constant values should patch the x86_64
version in MacroAssemblerX86Common.cpp.

> Source/JavaScriptCore/jit/JITStubsMSVC64.asm:109
> +    ;     esp[0 * ptrSize]: rflags
> +    ;     esp[1 * ptrSize]: return address / saved rip
> +    ;     esp[2 * ptrSize]: probe handler function
> +    ;     esp[3 * ptrSize]: probe arg
> +    ;     esp[4 * ptrSize]: saved rax
> +    ;     esp[5 * ptrSize]: saved rsp

This comment is now stale (see MacroAssemblerX86Common.cpp).  Please update it.