Bug 259525 - REGRESSION(266186@main): Broke cloop build on all architectures
Summary: REGRESSION(266186@main): Broke cloop build on all architectures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-07-26 11:03 PDT by Michael Catanzaro
Modified: 2023-07-28 07:51 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2023-07-26 11:03:54 PDT
It seems 266186@main broke the cloop build on at least x86_64, ppc64le, and s390x. The generated LLIntAssembly.h is trying to use a bunch of declarations that don't exist:

In file included from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:433:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h: In static member function ‘static JSC::JSValue JSC::LLInt::CLoop::execute(JSC::OpcodeID, void*, JSC::VM*, JSC::ProtoCallFrame*, bool)’:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40233:36: error: ‘ipint_unreachable_validate’ was not declared in this scope
40233 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_unreachable_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40233:1: error: ‘OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL’ was not declared in this scope; did you mean ‘OFFLINE_ASM_GLOBAL_LABEL’?
40233 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_unreachable_validate)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      | OFFLINE_ASM_GLOBAL_LABEL
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40238:36: error: ‘ipint_nop_validate’ was not declared in this scope
40238 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_nop_validate)
      |                                    ^~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40243:36: error: ‘ipint_block_validate’ was not declared in this scope
40243 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_block_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40248:36: error: ‘ipint_loop_validate’ was not declared in this scope
40248 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_loop_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40253:36: error: ‘ipint_if_validate’ was not declared in this scope
40253 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_if_validate)
      |                                    ^~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40258:36: error: ‘ipint_else_validate’ was not declared in this scope
40258 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_else_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40268:36: error: ‘ipint_end_validate’ was not declared in this scope
40268 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_end_validate)
      |                                    ^~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40273:36: error: ‘ipint_br_validate’ was not declared in this scope
40273 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_br_validate)
      |                                    ^~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40278:36: error: ‘ipint_br_if_validate’ was not declared in this scope
40278 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_br_if_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40283:36: error: ‘ipint_br_table_validate’ was not declared in this scope
40283 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_br_table_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40288:36: error: ‘ipint_return_validate’ was not declared in this scope
40288 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_return_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40293:36: error: ‘ipint_call_validate’ was not declared in this scope
40293 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_call_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40298:36: error: ‘ipint_call_indirect_validate’ was not declared in this scope
40298 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_call_indirect_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40311:36: error: ‘ipint_drop_validate’ was not declared in this scope
40311 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_drop_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40316:36: error: ‘ipint_select_validate’ was not declared in this scope
40316 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_select_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40321:36: error: ‘ipint_select_t_validate’ was not declared in this scope
40321 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_select_t_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40329:36: error: ‘ipint_local_get_validate’ was not declared in this scope
40329 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_local_get_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40334:36: error: ‘ipint_local_set_validate’ was not declared in this scope
40334 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_local_set_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40339:36: error: ‘ipint_local_tee_validate’ was not declared in this scope
40339 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_local_tee_validate)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated due to -fmax-errors=20.

I see that InPlaceInterpreter.h attempts to define these symbols only on aarch64 and x86_64, but the above build failure is actually on x86_64. (ppc64le and s390x fail in the same way.)

I need to investigate more to figure out what's going wrong, but my guess is there are at least two different problems here:

 * Broken on x86_64 due to missing #include? It's failing when building LowLevelInterpreter.cpp which notably does not #include InPlaceInterpreter.h
 * Broken on other architecturers due to improper conditional compilation? Looks like the code attempts to use the declarations on more architectures than they are declared on.
Comment 1 Michael Catanzaro 2023-07-26 11:10:33 PDT
$ git diff
diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp b/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp
index f6da744880f3..a5873b0b58ca 100644
--- a/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp
+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp
@@ -34,6 +34,7 @@
 #include "CLoopStackInlines.h"
 #include "CodeBlock.h"
 #include "CommonSlowPaths.h"
+#include "InPlaceInterpreter.h"
 #include "Interpreter.h"
 #include "LLIntCLoop.h"
 #include "LLIntData.h"

This fixes the above failures on x86_64, but I'm sure it will still be broken on other architectures, so more is needed.

However, there is another build failure that occurs next:

In file included from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:434:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h: In static member function ‘static JSC::JSValue JSC::LLInt::CLoop::execute(JSC::OpcodeID, void*, JSC::VM*, JSC::ProtoCallFrame*, bool)’:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:40233:1: error: ‘OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL’ was not declared in this scope; did you mean ‘OFFLINE_ASM_GLOBAL_LABEL’?
40233 | OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(ipint_unreachable_validate)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      | OFFLINE_ASM_GLOBAL_LABEL

This macro is new in 266186@main and is only defined for ARM thumb2 CPUs, but it's used unconditionally. :/
Comment 2 Michael Catanzaro 2023-07-26 11:59:51 PDT
I was able to get past that with this hack:

diff --git a/Source/JavaScriptCore/offlineasm/asm.rb b/Source/JavaScriptCore/offlineasm/asm.rb
index 34afcedc8449..4a38c695592f 100644
--- a/Source/JavaScriptCore/offlineasm/asm.rb
+++ b/Source/JavaScriptCore/offlineasm/asm.rb
@@ -239,11 +239,7 @@ class Assembler
         @internalComment = $enableLabelCountComments ? "Global Label #{@numGlobalLabels}" : nil
         if isGlobal
             if !$emitWinAsm
-                if isAligned
-                    @outp.puts(formatDump("OFFLINE_ASM_GLOBAL_LABEL(#{labelName})", lastComment))
-                else
-                    @outp.puts(formatDump("OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL(#{labelName})", lastComment))
-                end
+                @outp.puts(formatDump("OFFLINE_ASM_GLOBAL_LABEL(#{labelName})", lastComment))
             else
                 putsProc(labelName, lastComment)
             end

which is of course not an acceptable solution, but I don't know the right way to fix it. I'm skeptical that this was only tested on ARM thumb2, but that's definitely the only place where OFFLINE_ASM_UNALIGNED_GLOBAL_LABEL is defined.

Anyway, next problem is it doesn't link:

/usr/bin/ld: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-6e4525b9-1.cpp.o: in function `JSC::IPInt::initialize()':
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/InPlaceInterpreter.cpp:53: undefined reference to `ipint_unreachable_validate'
/usr/bin/ld: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/InPlaceInterpreter.cpp:53: undefined reference to `ipint_unreachable_validate'
/usr/bin/ld: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/InPlaceInterpreter.cpp:53: undefined reference to `ipint_unreachable_validate'
/usr/bin/ld: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/InPlaceInterpreter.cpp:53: undefined reference to `ipint_nop_validate'
/usr/bin/ld: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/InPlaceInterpreter.cpp:53: undefined reference to `ipint_unreachable_validate'

There are a few hundred more undefined references; I just copied the top few. I don't know where there are supposed to be defined.

Normally at this point I would do a revert to get the build working again, since this doesn't look straightforward to fix, but there are two more follow-up commits that depend on the original change, and it looks like Daniel is actively working on this, so I'll wait a couple days to give Daniel a chance to look first.
Comment 3 daniel_liu4 2023-07-26 14:41:29 PDT
Hi Michael, thanks for bringing this up! I didn't realize that this completely broke the cloop build, so my apologies for that. I'm currently testing on an ARM device, but compiling with `Tools/Scripts/build-jsc --cloop --debug WK_VALIDATE_DEPENDENCIES=YES ARCHS=x86_64`, I've been able to make some changes that should hopefully fix the build issues. I'll wrap it into my current in-place interpreter PR and see if that hopefully fixes it.
Comment 4 daniel_liu4 2023-07-27 18:33:53 PDT
Hi Michael! It looks like the cloop build works (based on build.webkit.org) as of [8e237a17b93c448615544a93160b779c8220f68d](https://github.com/WebKit/WebKit/commit/8e237a17b93c448615544a93160b779c8220f68d)! If you're still running into the same issue, please let me know!
Comment 5 Michael Catanzaro 2023-07-28 07:51:02 PDT
Confirmed fixed on all three architectures. Thank you!