Bug 173922 - Fix the AArch64 build after r218869
Summary: Fix the AArch64 build after r218869
Status: RESOLVED DUPLICATE of bug 173700
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Blocker
Assignee: JF Bastien
Depends on:
Blocks: 108645
  Show dependency treegraph
Reported: 2017-06-28 07:02 PDT by Csaba Osztrogonác
Modified: 2017-06-28 10:23 PDT (History)
5 users (show)

See Also:

patch (1.81 KB, patch)
2017-06-28 09:42 PDT, JF Bastien
mark.lam: review+
jfbastien: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2017-06-28 07:02:20 PDT
https://trac.webkit.org/changeset/218869 (bug173700) broke the AArch64 build:

{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]
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]

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 ***