Bug 173922

Summary: Fix the AArch64 build after r218869
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED DUPLICATE    
Severity: Blocker CC: clopez, fpizlo, jfbastien, mark.lam, saam
Priority: P1    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=173929
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
patch mark.lam: review+, jfbastien: commit-queue-

Description Csaba Osztrogonác 2017-06-28 07:02:20 PDT
https://trac.webkit.org/changeset/218869 (bug173700) broke the AArch64 build:
https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/1126

{standard input}: Assembler messages:
{standard input}:28: Error: unexpected register in the immediate operand at operand 3 -- `subs xzr,x3,sp'
{standard input}:120: Error: unexpected register in the immediate operand at operand 3 -- `subs xzr,x3,sp'

note: I just noticed this build failure, no time and plan to fix it myself.
Comment 1 JF Bastien 2017-06-28 08:47:37 PDT
I'll take a look.
Comment 2 JF Bastien 2017-06-28 09:10:04 PDT
Yeah on ARM64 that second register can't be SP, because register 31 means ZR here.
Comment 3 JF Bastien 2017-06-28 09:21:43 PDT
I think we want this:

diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
index fd35381d..0fb0254 100644
--- a/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
@@ -136,7 +136,7 @@ macro doVMEntry(makeCall)
     addp CallFrameHeaderSlots, t4, t4
     lshiftp 3, t4
     subp sp, t4, t3
-    bpa t3, sp, .throwStackOverflow
+    bibeq sp, t3, .throwStackOverflow
 
     # Ensure that we have enough additional stack capacity for the incoming args,
     # and the frame for the JS code we're executing. We need to do this check


It still makes the assembler sad though. I think it expects "wsp" and is sad when it just sees "sp"? Looking...
Comment 4 JF Bastien 2017-06-28 09:27:18 PDT
Ugh this is weird. arm64.rb:arm64Operand can't return "wsp" for "sp" all the time because other instructions get sad. But manually editing DerivedSources/JavaScriptCore/LLIntAssembly.h to "wsp" in those two spots works. So I can hack up arm64.rb:lowerARM64's handing for these branch instructions...
Comment 5 JF Bastien 2017-06-28 09:35:18 PDT
Oh derp, the problem is we're using w registers instead of x. This should be 64-bit, not 32, and the "sp" is implicitly 64 so no need for "xsp". Why is are we doing that though? Looking more...
Comment 6 JF Bastien 2017-06-28 09:39:08 PDT
OK bqbeq is the right thing, since it's quad. I now know a bit more llint.
Comment 7 JF Bastien 2017-06-28 09:42:06 PDT
Created attachment 314032 [details]
patch
Comment 8 JF Bastien 2017-06-28 09:46:03 PDT
Another option is to teach all the branches to flip their operands and the condition if the last operand is SP. I'm not sure that's worth doing in this patch.
Comment 9 Mark Lam 2017-06-28 09:46:44 PDT
Comment on attachment 314032 [details]
patch

this works as a workaround but bpa should also work because p stands for ointer and pointer on 64bit should be quad
Comment 10 JF Bastien 2017-06-28 09:47:43 PDT
The patch got reverted, I'll re-apply it with this fix.
Comment 11 JF Bastien 2017-06-28 09:51:19 PDT
Will fix in 173700.

*** This bug has been marked as a duplicate of bug 173700 ***