Bug 197192 – [GTK][WPE][JSCOnly] compile error when -DWTF_CPU_ARM64_CORTEXA53=ON set for arm64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Kai]

It removes function JSC::AssemblerBuffer::data() for ARM64 in commit https://trac.webkit.org/changeset/236589/webkit.
But it is required by Cortex A53 from https://trac.webkit.org/changeset/175514/webkit.

When compile webkitgtk in Yocto Project (https://www.yoctoproject.org) for qemuarm64 which a qemu arm64 bsp. It sets '-DWTF_CPU_ARM64_CORTEXA53=ON' and causes webkitgtk 2.24.0 fails with error:

| In file included from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:30,                                                                   
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk
-2.24.0/Source/JavaScriptCore/assembler/MacroAssembler.h:46,                                                                        
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScriptCore/jit/GPRInfo.h:28,                                                                                     
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScriptCore/bytecode/ArithProfile.h:28,                                                                           
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:28:                                                                   
| /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScr
iptCore/assembler/ARM64Assembler.h:465:48: warning: 'JSC::ARM64Assembler::LinkRecord::<unnamed union>::RealTypes::m_compareRegister'
is too small to hold all values of 'JSC::ARM64Assembler::RegisterID' {aka 'enum JSC::ARM64Registers::RegisterID'}                   
|                  RegisterID m_compareRegister : 6;                                                                                
|                                                 ^                                                                                 
| In file included from DerivedSources/ForwardingHeaders/wtf/Platform.h:31,                                                         
|                  from DerivedSources/ForwardingHeaders/wtf/ExportMacros.h:32,                                                     
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScriptCore/runtime/JSExportMacros.h:32,                                                                          
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScriptCore/config.h:32,                                                                                          
|                  from /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk
-2.24.0/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:26:                                                                   
| /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScr
iptCore/assembler/ARM64Assembler.h: In member function 'void JSC::ARM64Assembler::nopCortexA53Fix835769()':                         
| /home/kkang/buildarea/wrlx-201902/LIN1019-1104/tmp-glibc/work/aarch64-wrs-linux/webkitgtk/2.24.0-r0/webkitgtk-2.24.0/Source/JavaScr
iptCore/assembler/ARM64Assembler.h:3769:100: error: 'class JSC::AssemblerBuffer' has no member named 'data'                         
|                  if (UNLIKELY((*reinterpret_cast_ptr<int32_t*>(reinterpret_cast_ptr<char*>(m_buffer.data()) + m_buffer.codeSize() -
 sizeof(int32_t)) & 0x0a000000) == 0x08000000))                                                                                     
|                                                                                                     ^~~~                          
| DerivedSources/ForwardingHeaders/wtf/Compiler.h:343:41: note: in definition of macro 'UNLIKELY'                                   
|  #define UNLIKELY(x) __builtin_expect(!!(x), 0)

[Alberto Garcia]

The Debian arm64 buildd is also having this problem, I confirm that the build works if WTF_CPU_ARM64_CORTEXA53 is OFF.

This is the full build log (note that it's a 1.4 MiB text file):

https://buildd.debian.org/status/fetch.php?pkg=webkit2gtk&arch=arm64&ver=2.26.1-2&stamp=1570464655&raw=0

[Carlos Alberto Lopez Perez]

'-DWTF_CPU_ARM64_CORTEXA53=ON' Enables a workaround for a CPU Bug (erratum 835769) present in early versions of ARM64 Cortex A53 (in more detail, according to what I find in Internet, to revisions <= r0p4 of the Cortex A53 CPU).

This workaround was added 5 years ago in r175514 but it seems it stopped building after r236589 removed removes function JSC::AssemblerBuffer::data() for ARM64.

There are two major problems with this workaround:

1) The way it gets enabled its completely broken for cross-building. The CMake logic tries to parse /proc/cpuinfo to see if the build machine is a Cortex A53 (it just assumes build_machine==target_machine). It also doesn't do any extra effort to match the revision of the CPU to see if its newer than r0p4. It simply enables it for any build_machine==cortex_a53.

2) Testing that the workaround continues to be necessary and that it continues to work is tricky because it requires the affected hardware available.

[Carlos Alberto Lopez Perez]

Our current Linux JSCOnly ARM64 test bot happens to be an x86_64 machine that cross-builds and then runs the tests remotely.

Because it cross-builds its not enabling the workaround by default

The test machine where it currently runs is a Raspberry Pi 3, which happens to be an ARM Cortex A53 with a revision 4 of the core (r0p4)

	# head /proc/cpuinfo
	processor	: 0
	BogoMIPS	: 38.40
	Features	: fp asimd evtstrm crc32
	CPU implementer	: 0x41
	CPU architecture: 8
	CPU variant	: 0x0
	CPU part	: 0xd03
	CPU revision	: 4

	[....]


So, in theory, it would be affected by this erratum 835769. In practice I'm not 100% sure if its affected. But its the closed hardware I have that might have the bug.


And according to the current test results https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release things seems to be fine there, right?
So I guess that either the workaround is not necessary anymore, or that the RPi3 is not affected.

But, even if we assume that the RPi3 is not affected, the workaround its not building anymore, and we can't test it works because we don't have hardware.

And previously it was not getting enabled by default (only when the build machine was a Cortex A53 ... and native building on ARM is not the usual way of building WebKit).


So I think we should just remove this code for the workaround unless there is some bug report about this issue or a way to test it.

[Alberto Garcia]

What happens if you have this enabled but your CPU is not Cortex A53?

Does it break?

[Carlos Alberto Lopez Perez]

(In reply to Alberto Garcia from comment #4)
> What happens if you have this enabled but your CPU is not Cortex A53?
> 
> Does it break?

No, but it can affect performance.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #3)
> So I think we should just remove this code for the workaround unless there
> is some bug report about this issue or a way to test it.

Agreed. It clearly doesn't work and breaks the build on this hardware. Let's just remove it for now. The people who are trying to make things work on this hardware can come up with a new workaround if required.

[Michael Catanzaro]

Warning: this might require a clean build to pass EWS. I saw Ruby-level crashes until I deleted my build directory.

[Michael Catanzaro]

Created attachment 396274 [details]
Patch

[Michael Catanzaro]

Ping reviewers

[Yusuke Suzuki]

Comment on attachment 396274 [details]
Patch

r=me

[EWS]

Committed r260680: <https://trac.webkit.org/changeset/260680>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396274 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62347977>