Bug 175724 – Implement 64-bit MacroAssembler::probe support for Windows.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Per Arne Vollan]

This is needed in order to enable the DFG.

[Per Arne Vollan]

Created attachment 318761 [details]
Patch

[Mark Lam]

Comment on attachment 318761 [details]
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.

[Per Arne Vollan]

(In reply to Mark Lam from comment #2)
> Comment on attachment 318761 [details]
> 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.

Thanks for reviewing, Mark! I will update the patch before landing.

[Per Arne Vollan]

Committed r221032: <https://trac.webkit.org/changeset/221032/webkit>

[Radar WebKit Bug Importer]

<rdar://problem/34018595>