Bug 203290 – REGRESSION(r251468): Build, test failures in 32-bit JSC after BytecodeIndex refactoring

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Pablo Saavedra]

Error:

```
FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-11.cpp.o 
/home/buildbot/buildroot/buildroot/output.rpi3/host/usr/ccache/arm-buildroot-linux-gnueabihf-g++ --sysroot=/home/buildbot/buildroot/buildroot/output.rpi3/host/usr/arm-buildroot-linux-gnueabihf/sysroot  -DBUILDING_JSCONLY__ -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DSTATICALLY_LINKED_WITH_WTF -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 -I../../Source/ThirdParty/capstone/Source/include -IDerivedSources -I../../Source/ThirdParty -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-psabi -Wno-noexcept-type -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -O3 -fno-strict-aliasing -fno-exceptions -fno-rtti  -DNDEBUG -fPIC   -ffp-contract=off -std=c++17 -MD -MT Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-11.cpp.o -MF Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-11.cpp.o.d -o Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-11.cpp.o -c DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-11.cpp
In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-11.cpp:5:
../../Source/JavaScriptCore/dfg/DFGOSRExit.cpp: In function ‘void JSC::DFG::reifyInlinedCallFrames(JSC::DFG::Context&, JSC::CodeBlock*, const JSC::DFG::OSRExitBase&)’:
../../Source/JavaScriptCore/dfg/DFGOSRExit.cpp:828:58: error: no matching function for call to ‘JSC::CallSiteIndex::CallSiteIndex(const JSC::Instruction*&)’
         uint32_t locationBits = CallSiteIndex(instruction).bits();
```


it is related with JIT code because WPE still builds with `-DENABLE_JIT=OFF`

the fail was introduced in `BytecodeIndex should be a proper C++ class` https://trac.webkit.org/changeset/251468

[Pablo Saavedra]

Created attachment 381671 [details]
patch

[Pablo Saavedra]

Created attachment 381672 [details]
patch

[Pablo Saavedra]

This https://trac.webkit.org/changeset/251468/webkit#file83 is still needed. I just uploaded a patch adapting the code to the changes done in r251468.

[Mark Lam]

Does this patch pass JSC tests on 32-bit targets?  I think the i386 EWS bit is broken.  So, we can’t rely on it to tell us.

[Pablo Saavedra]

(In reply to Mark Lam from comment #4)
> Does this patch pass JSC tests on 32-bit targets?  I think the i386 EWS bit
> is broken.  So, we can’t rely on it to tell us.

I just tested in ARMv7 boards right now.

[Pablo Saavedra]

(In reply to Pablo Saavedra from comment #5)
> (In reply to Mark Lam from comment #4)
> > Does this patch pass JSC tests on 32-bit targets?  I think the i386 EWS bit
> > is broken.  So, we can’t rely on it to tell us.
> 
> I just tested in ARMv7 boards right now.


Sorry, I mean, I tested build step ... not the JSC tests.

[Pablo Saavedra]

(In reply to Pablo Saavedra from comment #6)
> (In reply to Pablo Saavedra from comment #5)
> > (In reply to Mark Lam from comment #4)
> > > Does this patch pass JSC tests on 32-bit targets?  I think the i386 EWS bit
> > > is broken.  So, we can’t rely on it to tell us.
> > 
> > I just tested in ARMv7 boards right now.
> 
> 
> Sorry, I mean, I tested build step ... not the JSC tests.

It is being a bit complicated to ensure anything because the r251425 (https://bugs.webkit.org/show_bug.cgi?id=202392#c41) was introduced so many failures the tests JSC tests

[Zan Dobersek]

Created attachment 381729 [details]
WIP fixes

This one contains both compile-time fixes as well as a few runtime fixes that temporarily cover work in #203285 or bring back 32-bit specifics from r251468.

This still leaves a good handful of tests failing, seemingly due to something in the DFG layer.

[Zan Dobersek]

Created attachment 381734 [details]
WIP fixes

Another fix in CodeOrigin::isHashTableDeletedValue().

[Yusuke Suzuki]

Comment on attachment 381672 [details]
patch

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

> Source/JavaScriptCore/interpreter/CallFrame.h:55
> +#if USE(JSVALUE32_64)
> +        explicit CallSiteIndex(const Instruction* instruction)
> +            : m_bytecodeIndex(bitwise_cast<uint32_t>(instruction))
> +        { }
> +#endif

I would like to get the refactoring that removes 32 / 64bit difference here: Use 64bit's logic and remove USE(JSVALUE32_64) around CallSiteIndex.
Can you do that?
Soon, we will introduce another change and that change could make this 32bit logic not working.

[Pablo Saavedra]

Created attachment 381779 [details]
patch

[Zan Dobersek]

(In reply to Yusuke Suzuki from comment #10)
> Comment on attachment 381672 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381672&action=review
> 
> > Source/JavaScriptCore/interpreter/CallFrame.h:55
> > +#if USE(JSVALUE32_64)
> > +        explicit CallSiteIndex(const Instruction* instruction)
> > +            : m_bytecodeIndex(bitwise_cast<uint32_t>(instruction))
> > +        { }
> > +#endif
> 
> I would like to get the refactoring that removes 32 / 64bit difference here:
> Use 64bit's logic and remove USE(JSVALUE32_64) around CallSiteIndex.
> Can you do that?
> Soon, we will introduce another change and that change could make this 32bit
> logic not working.

We can do manual casting to BytecodeIndex where required.

[Zan Dobersek]

Created attachment 381789 [details]
Pending fixes

This is a list of logic fixes that fix the remaining test failures (all but one). I'll polish this into a patch in a bit.

[Zan Dobersek]

Created attachment 381796 [details]
Patch

[Zan Dobersek]

Comment on attachment 381796 [details]
Patch

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

> Source/JavaScriptCore/bytecode/BytecodeIndex.h:42
> +    BytecodeIndex(WTF::HashTableDeletedValueType)
> +        : m_offset(deletedValue().asBits())
> +    {
> +    }

This missing constructor was the main problem of the remaining issues. Without it, the BytecodeIndex(WTF::HashTableDeletedValue) construction defaulted to the uin32_t constructor. With WTF::HashTableDeletedValue being effectively 0, this ended up constructing zero-valued BytecodeIndex objects that on their own are completely valid, non-empty and non-deleted.

[Zan Dobersek]

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

> Source/JavaScriptCore/bytecode/BytecodeIndex.h:42
> +    BytecodeIndex(WTF::HashTableDeletedValueType)
> +        : m_offset(deletedValue().asBits())
> +    {
> +    }

This missing constructor was the main problem of the remaining issues. Without it, the BytecodeIndex(WTF::HashTableDeletedValue) construction defaulted to the uin32_t constructor. With WTF::HashTableDeletedValue being effectively 0, this ended up constructing zero-valued BytecodeIndex objects that on their own are completely valid, non-empty and non-deleted.

[Zan Dobersek]

On 32-bit ARMv7, the patch puts into working order all the tests except one ChakraCore test. That failure is due to the BytecodeIndex rework, and apparently originates from the DFG layer, but it's still unclear why it occurs.

[Keith Miller]

Comment on attachment 381796 [details]
Patch

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

r=me weird that this worked on 64 bit... since I would have guessed this would break a lot of things...

>> Source/JavaScriptCore/bytecode/BytecodeIndex.h:42
>> +    }
> 
> This missing constructor was the main problem of the remaining issues. Without it, the BytecodeIndex(WTF::HashTableDeletedValue) construction defaulted to the uin32_t constructor. With WTF::HashTableDeletedValue being effectively 0, this ended up constructing zero-valued BytecodeIndex objects that on their own are completely valid, non-empty and non-deleted.

Ugh, we should make WTF::HashTableDeletedValueType be a enum class...

[Zan Dobersek]

Comment on attachment 381796 [details]
Patch

Clearing flags on attachment: 381796

Committed r251583: <https://trac.webkit.org/changeset/251583>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56608497>