Bug 299018 – REGRESSION(298234.133@webkitglib/2.50): [GTK] [2.50.0] Fails to build in i386: use of undeclared label 'op_instanceof_return_location'

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alberto Garcia]

Here is the full log, I think this is due to this commit: https://github.com/WebKit/WebKit/commit/f3f7e7880c36ac7d0735efc084c618382194382e

/usr/bin/clang++ -DBUILDING_GTK__=1 -DBUILDING_WEBKIT=1 -DBUILDING_WITH_CMAKE=1 -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/usr/bin/xdg-dbus-proxy\" -DGETTEXT_PACKAGE=\"WebKitGTK-4.1\" -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DPAS_BMALLOC=1 -D_GLIBCXX_ASSERTIONS=1 -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCore/Headers -I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include -I/tmp/webkit2gtk-2.50.0/build-soup3 -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/API -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/assembler -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/b3 -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/b3/air -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/bindings -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/builtins -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/bytecode -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/bytecompiler -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/dfg -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/disassembler -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/disassembler/ARM64 -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/disassembler/zydis -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/domjit -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/ftl -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/fuzzilli -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/heap -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/debugger -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/inspector -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/inspector/agents -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/inspector/augmentable -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/inspector/remote -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/interpreter -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/jit -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/llint -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/parser -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/profiler -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/runtime -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/tools -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/wasm -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/wasm/js -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/yarr -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCore/DerivedSources -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCore/DerivedSources/inspector -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCore/DerivedSources/runtime -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCore/DerivedSources/yarr -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/API/glib -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCoreGLib/DerivedSources -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCoreGLib/DerivedSources/jsc -I/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCoreGLib/Headers -I/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/inspector/remote/glib -I/tmp/webkit2gtk-2.50.0/build-soup3/WTF/Headers -I/usr/include/sysprof-6 -fdiagnostics-color=always -fcolor-diagnostics -Wextra -Wall -Werror=undefined-internal -Werror=undefined-inline -pipe -msse2 -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-parentheses-equality -Qunused-arguments -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare -fasynchronous-unwind-tables -fdebug-types-section -g1 -O2 -ffile-prefix-map=/tmp/webkit2gtk-2.50.0=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -DNDEBUG -DG_DISABLE_CAST_CHECKS -fno-strict-aliasing -fno-exceptions -fno-rtti -fcoroutines -ffunction-sections -fdata-sections -std=c++23 -fPIC -fvisibility=hidden -MD -MT Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o -MF Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o.d -o Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o -c /tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp
/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:286:9: error: use of undeclared label 'op_instanceof_return_location'
  286 |         FOR_EACH_BYTECODE_HELPER_ID(OPCODE_ENTRY)
      |         ^
/tmp/webkit2gtk-2.50.0/build-soup3/JavaScriptCore/DerivedSources/Bytecodes.h:989:11: note: expanded from macro 'FOR_EACH_BYTECODE_HELPER_ID'
  989 |     macro(op_instanceof_return_location, 0) \
      |           ^
/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:286:37: error: use of undeclared label 'op_instanceof_return_location_wide16'
  286 |         FOR_EACH_BYTECODE_HELPER_ID(OPCODE_ENTRY)
      |                                     ^
/tmp/webkit2gtk-2.50.0/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:286:37: error: use of undeclared label 'op_instanceof_return_location_wide32'
3 errors generated.

[Michael Catanzaro]

A patch from Yijia in bug #296042, which I have not tested yet:

```
diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
index 96aeb5dfba8e..3d495e673eca 100644
--- a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
@@ -3088,6 +3088,11 @@ _wasm_ipint_call_return_location_wide16:
 _wasm_ipint_call_return_location_wide32:
     crash()
 
+_op_instanceof_return_location:
+_op_instanceof_return_location_wide16:
+_op_instanceof_return_location_wide32:
+    crash()
+
 end # WEBASSEMBLY
 
 include? LowLevelInterpreterAdditions

[Alberto Garcia]

I confirm that it fixes the i386 build, thanks!

[Mike Gorse]

For me, this patch fixes the i586 build but makes the build fail on ppc64le:

/home/abuild/rpmbuild/BUILD/webkit2gtk3-2.50.0-build/webkitgtk-2.50.0/build/JavaScriptCore/DerivedSources/LLIntAssembly.h:41233:24: error: duplicate label ‘op_instanceof_return_location’
41233 | OFFLINE_ASM_GLUE_LABEL(op_instanceof_return_location)
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/abuild/rpmbuild/BUILD/webkit2gtk3-2.50.0-build/webkitgtk-2.50.0/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:124:39: note: in definition of macro ‘OFFLINE_ASM_GLUE_LABEL’
  124 | #define OFFLINE_ASM_GLUE_LABEL(label) label: TRACE_LABEL("OFFLINE_ASM_GLUE_LABEL", label); USE_LABEL(label);

There are similar errors for the other two opcodes that are being added.

[Michael Catanzaro]

I'm afraid this is too confusing for me. I'll need help from JavaScriptCore developers.

[Yijia Huang]

Hi Mike, can you try this:
```
diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
index af53fe269664..237fd6aab9c8 100644
--- a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
@@ -2958,6 +2958,13 @@ _wasm_ipint_call_return_location_wide16:
 _wasm_ipint_call_return_location_wide32:
     crash()
 
+if C_LOOP
+_op_instanceof_return_location:
+_op_instanceof_return_location_wide16:
+_op_instanceof_return_location_wide32:
+    crash()
+end
+
 end # WEBASSEMBLY
 
 include? LowLevelInterpreterAdditions
```

[Mike Gorse]

Hi Yijia,

I tried your patch from comment 5, and, unfortunately, I get the same error on ppc64le that I was getting with your original patch. I'm not passing any options for enabling or disabling cloop or jit on ppc64le, and it looks as though cloop is being used there.

[Yijia Huang]

Hi Mike and Michael, from the those crash logs and your reports, it seems that both i586/i386 (32bit) and ppc64le (64bit) use `C_LOOP`. So, I propose this fix.

```
if C_LOOP and not JSVALUE64
_op_instanceof_return_location:
_op_instanceof_return_location_wide16:
_op_instanceof_return_location_wide32:
    crash()
end
```

[Michael Catanzaro]

OK, I'll test that and create a pull request if it works. Thanks!

[Michael Catanzaro]

OK, a pull request doesn't work yet because the change from bug #296042 has not landed in main yet. Ideally we would remember that the patch needs to be edited before it lands during security "merge back."

[Mike Gorse]

Thanks, Yijia. This works for me on both i586 and ppc64le.

[Alberto Garcia]

The armhf build fails in Debian:

FAILED: Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o 
error: Undefined temporary symbol .Lop_instanceof_return_location
error: Undefined temporary symbol .Lop_instanceof_return_location_wide16
error: Undefined temporary symbol .Lop_instanceof_return_location_wide32

[Yijia Huang]

Hi Alberto, can you try this

```
...

if (C_LOOP and not JSVALUE64) or ARMv7
_op_instanceof_return_location:
_op_instanceof_return_location_wide16:
_op_instanceof_return_location_wide32:
    crash()
end

end # WEBASSEMBLY
```

in LowLevelInterpreter.asm?

[Alberto Garcia]

Thanks, Yijia!

Same error, unfortunately.

[Michael Catanzaro]

*** Bug 299171 has been marked as a duplicate of this bug. ***

[Yijia Huang]

Hi Alberto, I am confused.The armhf build configuration in Debian requires these return location labels but we don't know the exact preprocessor defines used for armhf builds. The labels are needed for OSR exit support in instanceof operations.

Current solution covers:
- 32-bit C_LOOP builds (i586, i386)
- Excludes ppc64le which generates labels from BytecodeList.rb

For armhf builds that still fail, the Debian maintainer should determine the correct armhf-specific preprocessor condition and add these labels under that condition:
```
    _op_instanceof_return_location:
    _op_instanceof_return_location_wide16:
    _op_instanceof_return_location_wide32:
        crash()
```

Another proposal from my side is:
```
if (C_LOOP and not PPC64LE)
_op_instanceof_return_location:
_op_instanceof_return_location_wide16:
_op_instanceof_return_location_wide32:
    crash()
end
```

[Alberto Garcia]

There must be something else going on with the armhf build because the compilation fails even if add that block unconditionally (i.e with no "if" / "end"):

[1/6554] Building CXX object Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
FAILED: Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
error: Undefined temporary symbol .Lop_instanceof_return_location
error: Undefined temporary symbol .Lop_instanceof_return_location_wide16
error: Undefined temporary symbol .Lop_instanceof_return_location_wide32
3 errors generated.

[Justin Michaud]

Instead of the provided fix, may I suggest:

```
diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
index 6d4036a55f96..f49782964262 100644
--- a/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
@@ -3354,6 +3354,9 @@ op(loop_osr_entry_gate, macro ()
     crash() # Should never reach here.
 end)
 
+op(op_instanceof_return_location, macro ()
+    crash() # Should never reach here.
+end)
 
 llintOpWithMetadata(op_check_private_brand, OpCheckPrivateBrand, macro (size, get, dispatch, metadata, return)
     metadata(t5, t2)
```

[Justin Michaud]

```
    _op_instanceof_return_location:
    _op_instanceof_return_location_wide16:
    _op_instanceof_return_location_wide32:
        crash()
```

For cloop, this needs to be outside the IF WEBASSEMBLY block

[Michael Catanzaro]

Hi Berto, I will wait for you to test the suggested changes and confirm they are good, since Fedora doesn't build for 32-bit ARM anymore.

[Alberto Garcia]

Hi, I confirm that this change alone (i.e no changes to LowLevelInterpreter.asm needed) fixes i386, armhf and ppc64el builds in Debian:

```
--- webkitgtk.orig/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
+++ webkitgtk/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
@@ -3354,6 +3354,9 @@ op(loop_osr_entry_gate, macro ()
     crash() # Should never reach here.
 end)
 
+op(op_instanceof_return_location, macro ()
+    crash() # Should never reach here.
+end)
 
 llintOpWithMetadata(op_check_private_brand, OpCheckPrivateBrand, macro (size, get, dispatch, metadata, return)
     metadata(t5, t2)
```

Thanks Justin and Yijia!!

[Mike Gorse]

I've tested Justin's patch from comment 17 on openSUSE, and it builds correctly on our targets as well.

[Michael Catanzaro]

Landed https://github.com/WebKit/WebKit/commit/fcaa289f6015595a638eb96b9f728eaaa7b13ab8