Bug 148376 – New tests introduced in r188545 fail on 32 bit ARM

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

12 different tests fail on ARM Linux (only 32 bit) which
introduced in https://trac.webkit.org/changeset/188545

Maybe they fail on iOS devices too, but there isn't any public iOS tester buildbot.

- EFL ARM Thumb2: https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release/builds/14915
- EFL ARM Traditional: https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Traditional%20Release/builds/14755
- GTK ARM (???): https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/8297

I won't have time to investigate this bug in the near future,
feel free to pick it up if somebody is interested in it.

[GSkachkov]

 Csaba Osztrogonác I'll try to reproduce on iOS.

[Csaba Osztrogonác]

(In reply to comment #1)
>  Csaba Osztrogonác I'll try to reproduce on iOS.

Have you managed to reproduce this bug on iOS too or is it a Linux only issue?

[GSkachkov]

(In reply to comment #2)
> (In reply to comment #1)
> >  Csaba Osztrogonác I'll try to reproduce on iOS.
> 
> Have you managed to reproduce this bug on iOS too or is it a Linux only
> issue?

I've not managed to reproduce on my default ios simulator but it is 64 bit, I still can't run test for ios 32 bit. Is it possibe to build and run test for Linux ARM on MacOS for instance with using VM with Ubuntu?

[Csaba Osztrogonác]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > >  Csaba Osztrogonác I'll try to reproduce on iOS.
> > 
> > Have you managed to reproduce this bug on iOS too or is it a Linux only
> > issue?
> 
> I've not managed to reproduce on my default ios simulator but it is 64 bit,
> I still can't run test for ios 32 bit. Is it possibe to build and run test
> for Linux ARM on MacOS for instance with using VM with Ubuntu?

I don't think so if it is possible to run 
tests in VM, only on real ARM hardware.

I'll create a debug build and try to provide a detailed gdb 
backtrace about these crashes sometime in the next week.

[Csaba Osztrogonác]

Here is a crash log in debug mode:

stress/arrowfunction-bound.js.default: ASSERTION FAILED: !from || from->JSCell::inherits(std::remove_pointer<To>::type::info())
stress/arrowfunction-bound.js.default: ../../Source/JavaScriptCore/runtime/JSCell.h(250) : To JSC::jsCast(From*) [with To = JSC::JSObject*; From = JSC::JSCell]
stress/arrowfunction-bound.js.default: 1   0xb65960e4 WTFCrashWithSecurityImplication
stress/arrowfunction-bound.js.default: 2   0x41134 JSC::JSObject* JSC::jsCast<JSC::JSObject*, JSC::JSCell>(JSC::JSCell*)
stress/arrowfunction-bound.js.default: 3   0x3bade JSC::asObject(JSC::JSCell*)
stress/arrowfunction-bound.js.default: 4   0x3de14 JSC::JSValue::getPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
stress/arrowfunction-bound.js.default: 5   0xb62ffbb2
stress/arrowfunction-bound.js.default: Segmentation fault
stress/arrowfunction-bound.js.default: ERROR: Unexpected exit code: 139
FAIL: stress/arrowfunction-bound.js.default

[Csaba Osztrogonác]

Unfortunately I can't provide GDB backtrace, because GDB segfaults :(

[Csaba Osztrogonác]

It should be a bug somewhere in the JIT engine, because 
this test passes with --useJIT=false command line option.

[GSkachkov]

(In reply to comment #7)
> It should be a bug somewhere in the JIT engine, because 
> this test passes with --useJIT=false command line option.

I've run safari on ios-simulator with 32 bit and but didn't manage reproduce 
issue when I was runnig test manually.
I'll take a look closer to my fixes that related to the JIT this weekend, possible I'll provide list of suspicious changes. That sad I can't reproduce issue on my env. 

Is there any way to use env(building bot) where you found  this issue by me for checking my local changes there, for instance for 1 week period?

[GSkachkov]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > >  Csaba Osztrogonác I'll try to reproduce on iOS.
> > > 
> > > Have you managed to reproduce this bug on iOS too or is it a Linux only
> > > issue?
> > 
> > I've not managed to reproduce on my default ios simulator but it is 64 bit,
> > I still can't run test for ios 32 bit. Is it possibe to build and run test
> > for Linux ARM on MacOS for instance with using VM with Ubuntu?
> 
> I don't think so if it is possible to run 
> tests in VM, only on real ARM hardware.
I've up & run Debian for ARM(A9) with using qEMU. Now I'm building Webkit GKT+ on Ubuntu VM. I hope that soon I'll manage to reproduce the issue.

> I'll create a debug build and try to provide a detailed gdb 
> backtrace about these crashes sometime in the next week.

[GSkachkov]

(In reply to comment #9)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (In reply to comment #1)
> > > > >  Csaba Osztrogonác I'll try to reproduce on iOS.
> > > > 
> > > > Have you managed to reproduce this bug on iOS too or is it a Linux only
> > > > issue?
> > > 
> > > I've not managed to reproduce on my default ios simulator but it is 64 bit,
> > > I still can't run test for ios 32 bit. Is it possibe to build and run test
> > > for Linux ARM on MacOS for instance with using VM with Ubuntu?
> > 
> > I don't think so if it is possible to run 
> > tests in VM, only on real ARM hardware.
> I've up & run Debian for ARM(A9) with using qEMU. Now I'm building Webkit
> GKT+ on Ubuntu VM. I hope that soon I'll manage to reproduce the issue.
> 
> > I'll create a debug build and try to provide a detailed gdb 
> > backtrace about these crashes sometime in the next week.

Ohh, you was right, qEMU unbearable slow, so I've bought used Samsung Chromebook with ARM. Now I'm in process of building jsc, hope soon will run the test and manage to reproduce the issue.

[GSkachkov]

Created attachment 261535 [details]
Patch

[GSkachkov]

I've managed to fix tests on my GTK ARM.

[GSkachkov]

Created attachment 261538 [details]
Patch

[Saam Barati]

Comment on attachment 261538 [details]
Patch

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

r=me with some nits.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4641
> +#if !USE(JSVALUE32_64)

nit: make this "USE(JSVALUE64)"

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4652
> +#if !USE(JSVALUE32_64)

nit: make this "USE(JSVALUE64)"

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4671
> +#if !USE(JSVALUE32_64)

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4705
> +#if !USE(JSVALUE32_64)

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4709
> +        m_jit.storePtr(thisValueTagGPR, MacroAssembler::Address(resultGPR, JSArrowFunction::offsetOfThisValue() + TagOffset));

nit: I would also make this store32.

[GSkachkov]

Created attachment 261574 [details]
Patch

Fix review comments

[Saam Barati]

Comment on attachment 261574 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4660
> +        DFG_TYPE_CHECK(thisValue.jsValueRegs(), node->child2(), SpecCell, m_jit.branchIfNotCell(thisValue.jsValueRegs()));

When would this ever not be a cell?

[GSkachkov]

Comment on attachment 261574 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4660
>> +        DFG_TYPE_CHECK(thisValue.jsValueRegs(), node->child2(), SpecCell, m_jit.branchIfNotCell(thisValue.jsValueRegs()));
> 
> When would this ever not be a cell?

At start child2 type is SpeculatedType SpecHeapTop  = 0x3bbfffff -> after this line it converts to SpeculatedType SpecCell = 0x000fffff
Without this line this code raise ASSERT error in following module https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h#L120
and it says that it expect Cell but there was Top. 
In JSVALUE64 branch this type check is done by following statement SpeculateCellOperand thisValue(this, node->child2()); (4653)

[Saam Barati]

(In reply to comment #17)
> Comment on attachment 261574 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261574&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4660
> >> +        DFG_TYPE_CHECK(thisValue.jsValueRegs(), node->child2(), SpecCell, m_jit.branchIfNotCell(thisValue.jsValueRegs()));
> > 
> > When would this ever not be a cell?
> 
> At start child2 type is SpeculatedType SpecHeapTop  = 0x3bbfffff -> after
> this line it converts to SpeculatedType SpecCell = 0x000fffff
> Without this line this code raise ASSERT error in following module
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/
> DFGAbstractInterpreterInlines.h#L120
> and it says that it expect Cell but there was Top. 
> In JSVALUE64 branch this type check is done by following statement
> SpeculateCellOperand thisValue(this, node->child2()); (4653)

Oh, right. The "this" value could be the empty value.

[WebKit Commit Bot]

Comment on attachment 261574 [details]
Patch

Clearing flags on attachment: 261574

Committed r190063: <http://trac.webkit.org/changeset/190063>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[GSkachkov]

Created attachment 261678 [details]
arm_tests_speccell.log

Log of tests with failed assert

[Csaba Osztrogonác]

(In reply to comment #19)
> Comment on attachment 261574 [details]
> Patch
> 
> Clearing flags on attachment: 261574
> 
> Committed r190063: <http://trac.webkit.org/changeset/190063>

How and why could CQ set the svn:executable property of soource files?

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
    Property svn:executable set to *

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
    Property svn:executable set to *

trunk/Source/JavaScriptCore/jit/JIT.h
    Property svn:executable set to *

trunk/Source/JavaScriptCore/jit/JITInlines.h
    Property svn:executable set to *

trunk/Source/JavaScriptCore/jit/JITOpcodes.cpp
    Property svn:executable set to *

[Csaba Osztrogonác]

Fixed in http://trac.webkit.org/changeset/196419

[GSkachkov]

It is weird, only one difference with another patches, I modified files on Laptop with ChromeOS to check on ARM.

[Alexey Proskuryakov]

> How and why could CQ set the svn:executable property of soource files?

The patch had that in:

+old mode 100644
+new mode 100755