Bug 122762 – [iOS] Upstream JavaScriptCore support for ARM64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Daniel Bates]

Upstream ARM64 infrastructure.

[Daniel Bates]

Created attachment 214162 [details]
WTF changes

[Daniel Bates]

Created attachment 214163 [details]
Build system changes

[Daniel Bates]

Created attachment 214164 [details]
JIT changes

[Daniel Bates]

Created attachment 214165 [details]
Assembler and disassembler changes

[WebKit Commit Bot]

Attachment 214162 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Atomics.h', u'Source/WTF/wtf/FastMalloc.cpp', u'Source/WTF/wtf/Platform.h', u'Source/WTF/wtf/dtoa.cpp', u'Source/WTF/wtf/dtoa/utils.h', u'Source/WTF/wtf/text/ASCIIFastPath.h', u'Source/WTF/wtf/text/StringImpl.h']" exit_code: 1
Source/WTF/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Attachment 214164 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/CCallHelpers.h', u'Source/JavaScriptCore/jit/ExecutableAllocator.h', u'Source/JavaScriptCore/jit/FPRInfo.h', u'Source/JavaScriptCore/jit/GPRInfo.h', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/jit/JITStubsARM64.h', u'Source/JavaScriptCore/jit/JSInterfaceJIT.h', u'Source/JavaScriptCore/jit/Repatch.cpp', u'Source/JavaScriptCore/jit/ScratchRegisterAllocator.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JITOperations.cpp:971:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperations.cpp:978:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITOperations.cpp:979:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITOperations.cpp:980:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:424:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/jit/GPRInfo.h:547:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/jit/JITStubsARM64.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITStubsARM64.h:63:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/jit/JITStubsARM64.h:130:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITStubsARM64.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITStubsARM64.h:133:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/jit/JITStubsARM64.h:141:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITStubsARM64.h:147:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITStubsARM64.h:148:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/jit/JITOperationWrappers.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 214166 [details]
Yarr and DFG changes

[WebKit Commit Bot]

Attachment 214165 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/assembler/ARM64Assembler.h', u'Source/JavaScriptCore/assembler/AbstractMacroAssembler.h', u'Source/JavaScriptCore/assembler/LinkBuffer.cpp', u'Source/JavaScriptCore/assembler/LinkBuffer.h', u'Source/JavaScriptCore/assembler/MacroAssembler.h', u'Source/JavaScriptCore/assembler/MacroAssemblerARM64.h', u'Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h', u'Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp', u'Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h', u'Source/JavaScriptCore/disassembler/ARM64Disassembler.cpp']" exit_code: 1
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:314:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:404:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:529:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:748:  Missing spaces around |  [whitespace/operators] [3]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:862:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h:202:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1350:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1352:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1353:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1354:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1357:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1361:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1366:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1371:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1376:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1711:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2281:  Missing spaces around /  [whitespace/operators] [3]
Source/JavaScriptCore/assembler/ARM64Assembler.h:26:  #ifndef header guard has wrong style, please use: ARM64Assembler_h  [build/header_guard] [5]
Source/JavaScriptCore/assembler/ARM64Assembler.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:350:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:497:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:2508:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/assembler/ARM64Assembler.h:2886:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:2887:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:2888:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:2889:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/JavaScriptCore/assembler/ARM64Assembler.h:3090:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/JavaScriptCore/assembler/MacroAssembler.h:39:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/disassembler/ARM64Disassembler.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 33 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 214167 [details]
Offlineasm changes

[Daniel Bates]

Created attachment 214168 [details]
LLInt changes

[WebKit Commit Bot]

Attachment 214166 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/heap/MachineStackMarker.cpp', u'Source/JavaScriptCore/heap/Region.h', u'Source/JavaScriptCore/yarr/YarrJIT.cpp', u'Source/JavaScriptCore/yarr/YarrJIT.h']" exit_code: 1
Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/JavaScriptCore/heap/MachineStackMarker.cpp:342:  user_count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 214167 [details]
Offlineasm changes

Attachment 214167 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3950884

[Oliver Hunt]

Comment on attachment 214162 [details]
WTF changes

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

>> Source/WTF/ChangeLog:1
>> [phone]  Daniel Bates  <[e-mail]>
> 
> ChangeLog entry has no bug number  [changelog/bugnumber] [5]

inlude the bug#

[Oliver Hunt]

Comment on attachment 214163 [details]
Build system changes

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:62
> +GCC_WARN_64_TO_32_BIT_CONVERSION_arm64 = NO;

That looks like something we'll need to fix -- filing a bug would be nice :)

[Oliver Hunt]

Comment on attachment 214164 [details]
JIT changes

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

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:424
>> +    // Calling convention:      f(edi, esi, edx, ecx, ...);
> 
> Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]

this comment is clearly wrong, bug to fix it  please

[Filip Pizlo]

Comment on attachment 214167 [details]
Offlineasm changes

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

> Source/JavaScriptCore/offlineasm/risc_arm64.rb:31
> +# Copyright (C) 2012 Apple Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +# 1. Redistributions of source code must retain the above copyright
> +#    notice, this list of conditions and the following disclaimer.
> +# 2. Redistributions in binary form must reproduce the above copyright
> +#    notice, this list of conditions and the following disclaimer in the
> +#    documentation and/or other materials provided with the distribution.
> +#
> +# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
> +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> +# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
> +# BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> +# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> +# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> +# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> +# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> +# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> +# THE POSSIBILITY OF SUCH DAMAGE.
> +
> +require 'ast'
> +require 'opt'
> +require 'risc'
> +
> +# This file contains utilities that should be in risc.rb if it wasn't for the
> +# fact that risc.rb is shared with the ARMv7 backend, and may be part of a
> +# code dump before ARM64 goes public.
> +#

I would merge this entire file with risk.rb.

> Source/JavaScriptCore/offlineasm/risc_arm64.rb:32
> +# FIXME: when ARM64 goes public, we should merge this file into risc.rb

:-/

[Oliver Hunt]

Comment on attachment 214165 [details]
Assembler and disassembler changes

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

>> Source/JavaScriptCore/assembler/ARM64Assembler.h:497
>> +        S
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

Thi should really have a name beyond "S"

>> Source/JavaScriptCore/assembler/ARM64Assembler.h:2886
>> +        return expected &&
> 
> Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]

Can you file a bug on style errors like this, they're gratuitous and i suspect a byproduct of an absence of a style checker being run on this code :-/

[Build Bot]

Comment on attachment 214163 [details]
Build system changes

Attachment 214163 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3945812

[Build Bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3776931

[Filip Pizlo]

Comment on attachment 214165 [details]
Assembler and disassembler changes

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

>>> Source/JavaScriptCore/assembler/ARM64Assembler.h:497
>>> +        S
>> 
>> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> 
> Thi should really have a name beyond "S"

No.  The purpose of this is to be able to say things like add<64, S>, which more closely reads like actual ARM64 assembly.

"S" is well-known to be a flag in ARM assembly.  It goes by no other name than "S".

It would greatly hurt the clarity of this code if we made this more than just S.

[Build Bot]

Comment on attachment 214166 [details]
Yarr and DFG changes

Attachment 214166 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3949823

[Build Bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3403996

[Build Bot]

Comment on attachment 214163 [details]
Build system changes

Attachment 214163 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3945814

[Build Bot]

Comment on attachment 214166 [details]
Yarr and DFG changes

Attachment 214166 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3885863

[Build Bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3948752

[EFL EWS Bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3954851

[kov's GTK+ EWS bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/3945821

[EFL EWS Bot]

Comment on attachment 214166 [details]
Yarr and DFG changes

Attachment 214166 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3885883

[EFL EWS Bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3946896

[EFL EWS Bot]

Comment on attachment 214166 [details]
Yarr and DFG changes

Attachment 214166 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3766936

[Daniel Bates]

Created attachment 214209 [details]
Offlineasm changes

Merged the contents of file offlineasm/risc_arm64.rb into file offlineasm/risc.rb per Filip Pizlo's remark.

[EFL EWS Bot]

Comment on attachment 214209 [details]
Offlineasm changes

Attachment 214209 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3799027

[Daniel Bates]

(In reply to comment #14)
> > Source/JavaScriptCore/Configurations/Base.xcconfig:62
> > +GCC_WARN_64_TO_32_BIT_CONVERSION_arm64 = NO;
> 
> That looks like something we'll need to fix -- filing a bug would be nice :)

Filed bug #122791.

[EFL EWS Bot]

Comment on attachment 214209 [details]
Offlineasm changes

Attachment 214209 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4036011

[Daniel Bates]

Created attachment 214210 [details]
Offlineasm changes

Remove inclusion of module risc_arm64 from file Source/JavaScriptCore/offlineasm/arm64.rb since we merged module risc_arm64 into module risc.

[EFL EWS Bot]

Comment on attachment 214210 [details]
Offlineasm changes

Attachment 214210 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4039006

[EFL EWS Bot]

Comment on attachment 214210 [details]
Offlineasm changes

Attachment 214210 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4025006

[Daniel Bates]

(In reply to comment #15)
> >> Source/JavaScriptCore/jit/ThunkGenerators.cpp:424
> >> +    // Calling convention:      f(edi, esi, edx, ecx, ...);
> > 
> [...]
> this comment is clearly wrong, bug to fix it  please

Filed bug #122796. Can you save me some time; do you know what the correct calling convention is for ARM64? Otherwise, I'll look into fixing this comment.

[Daniel Bates]

(In reply to comment #13)
> >> Source/WTF/ChangeLog:1
> >> [phone]  Daniel Bates  <[e-mail]>
> > 
> > ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> 
> inlude the bug#

I will fix up the change log entries when I concatenate all the reviewed patches to make a single commit.

[Build Bot]

Comment on attachment 214210 [details]
Offlineasm changes

Attachment 214210 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3461046

[Build Bot]

Comment on attachment 214210 [details]
Offlineasm changes

Attachment 214210 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4025004

[kov's GTK+ EWS bot]

Comment on attachment 214164 [details]
JIT changes

Attachment 214164 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4041011

[kov's GTK+ EWS bot]

Comment on attachment 214166 [details]
Yarr and DFG changes

Attachment 214166 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4060051

[Zoltan Herczeg]

Wow, 64 bit ARM support in JSC! This is great!

[Filip Pizlo]

Comment on attachment 214210 [details]
Offlineasm changes

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

> Source/JavaScriptCore/offlineasm/x86.rb:49
> -class SpecialRegister < NoChildren
> +class SpecialRegister

I think you want to revert this.  It looks like a bug fix that landed on trunk but that was reverted by accident in our ARM64 work.  Also, it has nothing to do with arm64 - this is in the x86 file.

[Daniel Bates]

(In reply to comment #45)
> > Source/JavaScriptCore/offlineasm/x86.rb:49
> > -class SpecialRegister < NoChildren
> > +class SpecialRegister
> 
> I think you want to revert this.  It looks like a bug fix that landed on trunk but that was reverted by accident in our ARM64 work.  Also, it has nothing to do with arm64 - this is in the x86 file.

Will revert change before landing.

[Daniel Bates]

Committed r157474: <http://trac.webkit.org/changeset/157474>