Bug 175724 - Implement 64-bit MacroAssembler::probe support for Windows.
Summary: Implement 64-bit MacroAssembler::probe support for Windows.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-18 10:22 PDT by Per Arne Vollan
Modified: 2017-08-22 12:26 PDT (History)
12 users (show)

See Also:


Attachments
Patch (13.88 KB, patch)
2017-08-22 09:44 PDT, Per Arne Vollan
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-08-18 10:22:05 PDT
This is needed in order to enable the DFG.
Comment 1 Per Arne Vollan 2017-08-22 09:44:19 PDT
Created attachment 318761 [details]
Patch
Comment 2 Mark Lam 2017-08-22 10:08:07 PDT
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.
Comment 3 Per Arne Vollan 2017-08-22 10:11:36 PDT
(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.
Comment 4 Per Arne Vollan 2017-08-22 12:25:41 PDT
Committed r221032: <https://trac.webkit.org/changeset/221032/webkit>
Comment 5 Radar WebKit Bug Importer 2017-08-22 12:26:47 PDT
<rdar://problem/34018595>