Bug 217079 – REGRESSION(r259582): Build fails on aarch64 Linux with WebKit 2.30.1 on LLIntOffsetsExtractor.cpp.o

• Component: JavaScriptCore
• Status: REOPENED

[Guillaume G]

Build failed on aarch64 Linux with WebKit 2.30.1 with GCC 10.2.

[  214s] FAILED: Source/JavaScriptCore/CMakeFiles/LLIntOffsetsExtractor.dir/llint/LLIntOffsetsExtractor.cpp.o 
[  214s] /usr/bin/ccache /var/lib/build/ccache/bin/c++ -DBUILDING_GTK__=1 -DBUILDING_WITH_CMAKE=1 -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/usr/bin/xdg-dbus-proxy\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DJSC_COMPILATION -DJSC_GLIB_API_ENABLED -DSTATICALLY_LINKED_WITH_WTF -DSVN_REVISION=\"tarball\" -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -IDerivedSources/ForwardingHeaders -I. -I../Source/JavaScriptCore -I../Source/JavaScriptCore/API -I../Source/JavaScriptCore/assembler -I../Source/JavaScriptCore/b3 -I../Source/JavaScriptCore/b3/air -I../Source/JavaScriptCore/bindings -I../Source/JavaScriptCore/builtins -I../Source/JavaScriptCore/bytecode -I../Source/JavaScriptCore/bytecompiler -I../Source/JavaScriptCore/dfg -I../Source/JavaScriptCore/disassembler -I../Source/JavaScriptCore/disassembler/ARM64 -I../Source/JavaScriptCore/disassembler/udis86 -I../Source/JavaScriptCore/domjit -I../Source/JavaScriptCore/ftl -I../Source/JavaScriptCore/heap -I../Source/JavaScriptCore/debugger -I../Source/JavaScriptCore/inspector -I../Source/JavaScriptCore/inspector/agents -I../Source/JavaScriptCore/inspector/augmentable -I../Source/JavaScriptCore/inspector/remote -I../Source/JavaScriptCore/interpreter -I../Source/JavaScriptCore/jit -I../Source/JavaScriptCore/llint -I../Source/JavaScriptCore/parser -I../Source/JavaScriptCore/profiler -I../Source/JavaScriptCore/runtime -I../Source/JavaScriptCore/tools -I../Source/JavaScriptCore/wasm -I../Source/JavaScriptCore/wasm/js -I../Source/JavaScriptCore/yarr -IDerivedSources/JavaScriptCore -IDerivedSources/JavaScriptCore/inspector -IDerivedSources/JavaScriptCore/runtime -IDerivedSources/JavaScriptCore/yarr -IDerivedSources/ForwardingHeaders/JavaScriptCore/glib -IDerivedSources/JavaScriptCore/javascriptcoregtk/jsc -I../Source/JavaScriptCore/API/glib -IDerivedSources/JavaScriptCore/javascriptcoregtk -I../Source/JavaScriptCore/inspector/remote/glib -isystem /usr/include/glib-2.0 -isystem /usr/lib64/glib-2.0/include -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -g1 -Wl,--no-keep-memory -DNDEBUG -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIE -std=c++17 -MD -MT Source/JavaScriptCore/CMakeFiles/LLIntOffsetsExtractor.dir/llint/LLIntOffsetsExtractor.cpp.o -MF Source/JavaScriptCore/CMakeFiles/LLIntOffsetsExtractor.dir/llint/LLIntOffsetsExtractor.cpp.o.d -o Source/JavaScriptCore/CMakeFiles/LLIntOffsetsExtractor.dir/llint/LLIntOffsetsExtractor.cpp.o -c ../Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp
[  214s] In file included from ../Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:30,
[  214s]                  from ../Source/JavaScriptCore/assembler/MacroAssembler.h:46,
[  214s]                  from ../Source/JavaScriptCore/jit/GPRInfo.h:28,
[  214s]                  from ../Source/JavaScriptCore/bytecode/ArithProfile.h:28,
[  214s]                  from ../Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:28:
[  214s] ../Source/JavaScriptCore/assembler/ARM64Assembler.h: In static member function 'static void JSC::ARM64Assembler::replaceWithJump(void*, void*)':
[  214s] ../Source/JavaScriptCore/assembler/ARM64Assembler.h:2576:51: error: 'class JSC::ExecutableAllocator' has no member named 'getJumpIslandTo'
[  214s]  2576 |             to = ExecutableAllocator::singleton().getJumpIslandTo(where, to);
[  214s]       |                                                   ^~~~~~~~~~~~~~~
[  214s] ../Source/JavaScriptCore/assembler/ARM64Assembler.h: In static member function 'static void* JSC::ARM64Assembler::prepareForAtomicRelinkJumpConcurrently(void*, void*)':
[  214s] ../Source/JavaScriptCore/assembler/ARM64Assembler.h:2781:49: error: 'class JSC::ExecutableAllocator' has no member named 'getJumpIslandToConcurrently'
[  214s]  2781 |         return ExecutableAllocator::singleton().getJumpIslandToConcurrently(from, to);
[  214s]       |                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
[  214s] ../Source/JavaScriptCore/assembler/ARM64Assembler.h: In static member function 'static void JSC::ARM64Assembler::linkJumpOrCall(int*, const int*, void*)':
[  214s] ../Source/JavaScriptCore/assembler/ARM64Assembler.h:3024:51: error: 'class JSC::ExecutableAllocator' has no member named 'getJumpIslandTo'
[  214s]  3024 |             to = ExecutableAllocator::singleton().getJumpIslandTo(bitwise_cast<void*>(fromInstruction), to);
[  214s]       |                                                   ^~~~~~~~~~~~~~~

[Guillaume G]

It was also failing with 2.30.0.

[Mike Gorse]

We are currently passing -DENABLE_JIT=OFF on aarch64.

[Michael Catanzaro]

Looks like this broke in r259582 "Implement 1GB of executable memory on arm64"

[Mike Gorse]

I tried patching wtf/Platform.h to only define USE_JUMP_ISLANDS if JIT is enabled, but now I get this failure, related to R262670 I think:
../Source/JavaScriptCore/assembler/LinkBuffer.cpp: In member function 'void JSC::LinkBuffer::copyCompactAndLinkCode(JSC::MacroAssembler&, JSC::JITCompilationEffort)':
../Source/JavaScriptCore/assembler/LinkBuffer.cpp:380:13: error: there are no arguments to 'dumpJITMemory' that depend on a template parameter, so a declaration of 'dumpJITMemory' must be available [-fpermissive]

[Mike Gorse]

Created attachment 410073 [details]
Patch.

[Michael Catanzaro]

Comment on attachment 410073 [details]
Patch.

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

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:377
> +    ASSERT(codeOutData == outData);

Careful, the assert is different in the two codepaths!

[Mike Gorse]

Created attachment 410127 [details]
Patch.

Good catch. Fixed the asserts.

[Michael Catanzaro]

Comment on attachment 410127 [details]
Patch.

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

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:389
> +    performJITMemcpy(codeOutData, outData, m_size);
> +#endif // ENABLE_JIT

I had to investigate why it's OK to call performJITMemcpy when JIT is disabled. Turns out it just does a memcpy. OK.

[EWS]

Committed r267795: <https://trac.webkit.org/changeset/267795>

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

[Radar WebKit Bug Importer]

<rdar://problem/69796640>

[Mark Lam]

Comment on attachment 410127 [details]
Patch.

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

Please roll out this patch and fix the issues below.

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:377
> +#if ENABLE(JIT)

This change doesn't make sense.  LinkBuffer::copyCompactAndLinkCode() should only be built in if the assembler is enabled.  Not only that, this change is actually wrong.  If useFastJITPermissions() is true, we should not be doing the performJITMemcpy() here at all.  You've actually introduced a bug.  Fortunately, this bug will not manifest if ENABLE(JIT) is true.  It still makes the code lie about what is correct and expected.

I think the real fix for this should be to ensure that all the support for the ASSEMBLER is not built in if none of the JITs are enabled.  For now, can you roll out this patch and move this ENABLE(JIT) this to just around the if statement for the dumpJITMemory?  Also add a FIXME comment here that this is a temporary build fix until we can figure out why all this JIT and ASSEMBLER infrastructure is built in on Linux when JIT is disabled.

You should also check why ENABLE(ASSEMBLER) is enabled for you.  For example, do you have ENABLE(YARR_JIT) set to true, or do you have ENABLE(C_LOOP) set to false for your configuration?  Maybe there's no bug there, but you should make sure.

> Source/WTF/wtf/PlatformEnable.h:888
> +#if CPU(ARM64) && CPU(ADDRESS64) && ENABLE(JIT)

This change also doesn't make sense.  USE(JUMP_ISLANDS) should at most be dependent on ENABLE(ASSEMBLER), but it is only ever used in assembler code.  Hence, this change should not be necessary at all.

[Mark Lam]

Comment on attachment 410127 [details]
Patch.

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

>> Source/WTF/wtf/PlatformEnable.h:888
>> +#if CPU(ARM64) && CPU(ADDRESS64) && ENABLE(JIT)
> 
> This change also doesn't make sense.  USE(JUMP_ISLANDS) should at most be dependent on ENABLE(ASSEMBLER), but it is only ever used in assembler code.  Hence, this change should not be necessary at all.

If there's no configuration issue on your part, adding this ENABLE(JIT) as a workaround is executable for now.  Please add a FIXME to say that this is a temporary workaround (just like in my previous comment).

[WebKit Commit Bot]

Re-opened since this is blocked by bug 217135

[Michael Catanzaro]

Mike, I've been talking with Mark about this. This build failure only occurs when cloop and JIT are both enabled. This is not a configuration that we *intentionally* use on Linux (although Apple uses it). Problem is I changed WebKitFeatures.cmake a while back to do the right thing on all architectures, so we no longer have to pass -DENABLE_JIT=OFF -DUSE_SYSTEM_MALLOC=ON. Except it really only does the right thing on all architectures *except* aarch64, where it's impossible to make the right choice without knowing system page size (which could be different on the builder than on the target system). So enterprise distros that support 64k pages still need to set these flags manually, but only for aarch64. Now here is my mistake: I used to have WebKitFeatures.cmake set up such that -DENABLE_JIT=OFF would set -DENABLE_C_LOOP=ON. And I assumed that was still the case, but it no longer is, because that code was no longer needed anymore now that it does the right thing for "all" architectures (oops). So you're ending up with cloop disabled by mistake, and that is why LinkBuffer.cpp is being compiled. I have the identical mistake in RHEL's build system too.

So there are two problems: (a) you should build with -DENABLE_C_LOOP=ON. (b) this  bug really should be fixed as Mark suggests, but it should only affect Apple ports because only Apple ports should be building this file with JIT disabled.

P.S. There is no amazing reason to always enable cloop when JIT is disabled. Upstream WebKit has three options: cloop, asm llint, and JIT. We just simplify it down to two for Linux so there is one fewer case to think about. Hence, it's not wrong or horrible to ship with both cloop and JIT disabled, just not what I had intended.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> Mike, I've been talking with Mark about this. This build failure only occurs
> when cloop and JIT are both enabled.

I meant: both disabled

[Michael Catanzaro]

(In reply to Mark Lam from comment #11) 
> I think the real fix for this should be to ensure that all the support for
> the ASSEMBLER is not built in if none of the JITs are enabled.

I notice there are only a few ENABLE(JIT) guards under Source/JavaScriptCore/assembler:

$ git grep 'ENABLE(JIT)'
MacroAssemblerCodeRef.h:#if CPU(ARM_THUMB2) && ENABLE(JIT)
testmasm.cpp:#if ENABLE(JIT)
testmasm.cpp:#else // not ENABLE(JIT)
testmasm.cpp:#endif // ENABLE(JIT)

I also notice we have, in PlatformEnable.h:

/* If either the JIT or the RegExp JIT is enabled, then the Assembler must be
   enabled as well: */
#if ENABLE(JIT) || ENABLE(YARR_JIT) || !ENABLE(C_LOOP)
#if defined(ENABLE_ASSEMBLER) && !ENABLE_ASSEMBLER
#error "Cannot enable the JIT or RegExp JIT without enabling the Assembler"
#else
#undef ENABLE_ASSEMBLER
#define ENABLE_ASSEMBLER 1
#endif
#endif

So we could just remove || !ENABLE(C_LOOP) from the first line, and remove the test from MacroAssemblerCodeRef.h. (testmasm.cpp is built regardless of whether JIT is enabled.)

[Mark Lam]

(In reply to Michael Catanzaro from comment #16)
> (In reply to Mark Lam from comment #11) 
> > I think the real fix for this should be to ensure that all the support for
> > the ASSEMBLER is not built in if none of the JITs are enabled.
> 
> I notice there are only a few ENABLE(JIT) guards under
> Source/JavaScriptCore/assembler:
> 
> $ git grep 'ENABLE(JIT)'
> MacroAssemblerCodeRef.h:#if CPU(ARM_THUMB2) && ENABLE(JIT)
> testmasm.cpp:#if ENABLE(JIT)
> testmasm.cpp:#else // not ENABLE(JIT)
> testmasm.cpp:#endif // ENABLE(JIT)
> 
> I also notice we have, in PlatformEnable.h:
> 
> /* If either the JIT or the RegExp JIT is enabled, then the Assembler must be
>    enabled as well: */
> #if ENABLE(JIT) || ENABLE(YARR_JIT) || !ENABLE(C_LOOP)
> #if defined(ENABLE_ASSEMBLER) && !ENABLE_ASSEMBLER
> #error "Cannot enable the JIT or RegExp JIT without enabling the Assembler"
> #else
> #undef ENABLE_ASSEMBLER
> #define ENABLE_ASSEMBLER 1
> #endif
> #endif
> 
> So we could just remove || !ENABLE(C_LOOP) from the first line, and remove
> the test from MacroAssemblerCodeRef.h. (testmasm.cpp is built regardless of
> whether JIT is enabled.)

Yes, the assembler should not need to be on if we don't enable any JITs.  I think this is an existing bug when building for the asm llint.  It's something I'll look into eventually, but I don't think we should need to fix that right now to resolve this bug's build failure.  But if you would like to try to fix it, that's fine too.

[Michael Catanzaro]

Ah, actually there is a flaw in my plan. If we have ENABLE_JIT=ON but ENABLE_YARR_JIT=OFF, then we need ENABLE_ASSEMBLER=ON. I don't know why that should be supported -- it smells like too many build configurations -- but it does mean that ENABLE_ASSEMBLER=ON cannot currently assume ENABLE_JIT=ON (unless we change that).

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #18)
> If we have ENABLE_JIT=ON but
> ENABLE_YARR_JIT=OFF, then we need ENABLE_ASSEMBLER=ON.

I guess I'm really backwards today. I meant: "if we have ENABLE_JIT=OFF but ENABLE_YARR_JIT=ON, then we need ENABLE_ASSEMBLER=ON"

[Mike Gorse]

Thanks. If I pass -DENABLE_C_LOOP=ON to cmake, then I get a successful build (I also needed to pass -DENABLE_SAMPLING_PROFILER=OFF).
I'll make that change to openSUSE / SLE.

[Michael Catanzaro]

(In reply to Mike Gorse from comment #20)
> Thanks. If I pass -DENABLE_C_LOOP=ON to cmake, then I get a successful build
> (I also needed to pass -DENABLE_SAMPLING_PROFILER=OFF).
> I'll make that change to openSUSE / SLE.

And I'll make that change in RHEL.

We really need to come up with something better for aarch64. :/