Bug 203705 - Disable Wasm interpreter on WinCairo
Summary: Disable Wasm interpreter on WinCairo
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks: 203716
  Show dependency treegraph
 
Reported: 2019-10-31 17:13 PDT by Tadeu Zagallo
Modified: 2019-10-31 20:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2019-10-31 17:16 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (1.33 KB, patch)
2019-10-31 19:39 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-10-31 17:13:32 PDT
...
Comment 1 Tadeu Zagallo 2019-10-31 17:16:50 PDT
Created attachment 382524 [details]
Patch
Comment 2 Yusuke Suzuki 2019-10-31 17:22:22 PDT
Do you know why ENABLE(WEBASSEMBLY) is ON in Windows?
Comment 3 Tadeu Zagallo 2019-10-31 17:26:26 PDT
(In reply to Yusuke Suzuki from comment #2)
> Do you know why ENABLE(WEBASSEMBLY) is ON in Windows?

No idea, I asked in the original bug, and that's the only reason I didn't disable it in the original commit, since I didn't know whether it'd be of interest to actually make it work or whether I should just disable it. I figured I should just have the patch ready so we can get it back building just in case.
Comment 4 Yusuke Suzuki 2019-10-31 19:33:07 PDT
Comment on attachment 382524 [details]
Patch

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

r=me with FIXME request.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2018
> +if WEBASSEMBLY and not X86_64_WIN

Can you add a FIXME and URL to fix this issue? I think this is essentially unnecessary, and it should be fixed.
I don’t consider that WEBASSEMBLY is true on Windows, so it would eventually can cause the problem again: like putting a code that is guarded by WEBASSEMBLY, but we don’t consider Windows at all.
So I think this should be fixed soon, but I don’t want to Wasm interpreter patch roll-out. So, I’m OK for this for very short-term workaround.
Comment 5 Tadeu Zagallo 2019-10-31 19:34:41 PDT
Comment on attachment 382524 [details]
Patch

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

Thanks for the review!

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2018
>> +if WEBASSEMBLY and not X86_64_WIN
> 
> Can you add a FIXME and URL to fix this issue? I think this is essentially unnecessary, and it should be fixed.
> I don’t consider that WEBASSEMBLY is true on Windows, so it would eventually can cause the problem again: like putting a code that is guarded by WEBASSEMBLY, but we don’t consider Windows at all.
> So I think this should be fixed soon, but I don’t want to Wasm interpreter patch roll-out. So, I’m OK for this for very short-term workaround.

Thanks, that makes sense. I'll add it.
Comment 6 Tadeu Zagallo 2019-10-31 19:39:33 PDT
Created attachment 382540 [details]
Patch for landing
Comment 7 Brent Fulgham 2019-10-31 19:57:17 PDT
Note: This breaks 64-bit Windows in general, not just WinCairo.
Comment 8 Brent Fulgham 2019-10-31 20:19:28 PDT
Comment on attachment 382540 [details]
Patch for landing

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

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2019
> +// https://bugs.webkit.org/show_bug.cgi?id=203716

Shouldn't these be '#' comments?
Comment 9 Tadeu Zagallo 2019-10-31 20:20:49 PDT
Comment on attachment 382540 [details]
Patch for landing

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2019
>> +// https://bugs.webkit.org/show_bug.cgi?id=203716
> 
> Shouldn't these be '#' comments?

We usually use `#`, so that was unintentional, but both work.
Comment 10 WebKit Commit Bot 2019-10-31 20:23:20 PDT
Comment on attachment 382540 [details]
Patch for landing

Clearing flags on attachment: 382540

Committed r251904: <https://trac.webkit.org/changeset/251904>
Comment 11 WebKit Commit Bot 2019-10-31 20:23:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-10-31 20:24:16 PDT
<rdar://problem/56805484>
Comment 13 Brent Fulgham 2019-10-31 20:25:05 PDT
Sadly, I don't believe this fixes the 64-bit Window build. I still see errors:

:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(26614): error A2008: syntax error : xmm1 [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\
ource\JavaScriptCore\JavaScriptCore.vcxproj]
:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(26721): error A2008: syntax error : xmm1 [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\
ource\JavaScriptCore\JavaScriptCore.vcxproj]

.... and many more.

26614 in LowLevelIntepreterWin.asm is the 'ucomi' command below:

  _offlineasm_compareJumpOp__llintOpWithJump__llintOpWithM_____makeReturn__fn__fn__fn__429_impl__op2NotInt:
    cvtsi2sd xmm0, eax                                       ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1861
    test rdx, r14                                            ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1862
    jz _offlineasm_compareJumpOp__llintOpWithJump__llintOpWithM___n__fn__makeReturn__fn__fn__fn__429_impl__slow
    add rdx, r14                                             ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1863
    movd xmm1, rdx                                           ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1864
    ucomi xmm1, xmm0                                         ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1712
    jbe _offlineasm_compareJumpOp__llintOpWithJump__llintOpWithM____makeReturn__fn__fn__fn__429_impl__jumpTarget
    add r10, 4                                               ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:308
    movzx eax, byte ptr [0 + r13 + r10 * 1]                  ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:28
    lea rdx, g_opcodeMap                                     ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:29
    jmp qword ptr [0 + rdx + rax * 8]                        ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:30
Comment 14 Brent Fulgham 2019-10-31 20:25:21 PDT
Reopening: This does not fix the build issue (as shown above).
Comment 15 Brent Fulgham 2019-10-31 20:27:48 PDT
More errors:

C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(5973): error A2008: syntax error : , [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(5990): error A2008: syntax error : , [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(6098): error A2008: syntax error : , [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(6115): error A2008: syntax error : , [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(6225): error A2008: syntax error : , [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(6242): error A2008: syntax error : , [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(6341): error A2152: coprocessor register cannot be first operand [C:\Projects\WebKit\OpenSource\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:/Projects/WebKit/OpenSource/WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(6358): error A2152: coprocessor register cannot be first operand [C:\Projects\WebKit\OpenSou

Line 5973 is the 'mul' command:

  _offlineasm_binaryOpCustomStore__llintOpWithMetadata__ll___n__fn__makeReturn__fn__fn__fn__op1NotIntReady:
    movsx r8, byte ptr [1 + r13 + r10 * 1]                   ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:50
    add rax, r14                                             ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1025
    movd xmm0, rax                                           ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1026
    mul xmm0, xmm1                                           ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1090
    movd rax, xmm0                                           ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1028
    sub rax, r14                                             ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1029
    mov qword ptr [0 + rbp + r8 * 8], rax                    ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1030
    add r10, 6                                               ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:308
    movzx eax, byte ptr [0 + r13 + r10 * 1]                  ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:28
    lea rdx, g_opcodeMap                                     ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:29
    jmp qword ptr [0 + rdx + rax * 8]                        ; C:/Projects/WebKit/OpenSource/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:30
Comment 16 Brent Fulgham 2019-10-31 20:28:03 PDT
I really don't think things are in a good state right now. Can't we roll this out somehow?