[JSC] AI should check the given constant's array type when folding GetByVal into...
authoryusukesuzuki@slowstart.org <yusukesuzuki@slowstart.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Jan 2019 01:26:43 +0000 (01:26 +0000)
committeryusukesuzuki@slowstart.org <yusukesuzuki@slowstart.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Jan 2019 01:26:43 +0000 (01:26 +0000)
commit634db9d77e9ef8e8e23176a380d427e1a1732525
treee7d32ed5aeafa8cbcdcdd5be7d56947c3dcd0aa0
parentc9d211996a3803a3544c44d5be4adc84f5926e3e
[JSC] AI should check the given constant's array type when folding GetByVal into constant
https://bugs.webkit.org/show_bug.cgi?id=193413
<rdar://problem/46092389>

Reviewed by Keith Miller.

JSTests:

This test is super flaky. It causes crash in r238109, but it does not crash with `--useConcurrentJIT=false`.
It does not cause any crashes on the latest revision too. Basically, it highly depends on the timing, and
without this patch, the root cause is not fixed yet. If GetLocal is turned into JSConstant in AI,
but GetByVal does not have appropriate ArrayModes, JSC crashes.

* stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js: Added.
(compareArray):

Source/JavaScriptCore:

If GetByVal's DFG::ArrayMode's type is Array::Double, we expect that the result of GetByVal is Double, since we already performed CheckStructure or CheckArray
to ensure this array type. But this assumption on the given value becomes wrong in AI, since CheckStructure may not perform filtering. And the proven AbstractValue
in GetByVal would not be expected one.

We have the graph before performing constant folding.

53:<!0:->     GetLocal(Check:Untyped:@77, JS|MustGen|UseAsOther, Array, arg2(C<Array>/FlushedCell), R:Stack(7), bc#37, ExitValid)  predicting Array
54:< 1:->     JSConstant(JS|PureNum|UseAsOther|UseAsInt|ReallyWantsInt, BoolInt32, Int32: 0, bc#37, ExitValid)
93:<!0:->     CheckStructure(Cell:@53, MustGen, [%C7:Array], R:JSCell_structureID, Exits, bc#37, ExitValid)
94:< 1:->     GetButterfly(Check:Cell:@53, Storage|PureInt, R:JSObject_butterfly, Exits, bc#37, ExitValid)
55:<!0:->     GetByVal(Check:KnownCell:@53, Check:Int32:@54, Check:Untyped:@94, Double|MustGen|VarArgs|PureInt, AnyIntAsDouble|NonIntAsdouble, Double+OriginalCopyOnWriteArray+SaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedDoubleProperties, Exits, bc#37, ExitValid)  predicting StringIdent|NonIntAsdouble

And 53 is converted to JSConstant in the constant folding. It leads to constant folding attempt in GetByVal.

53:< 1:->     JSConstant(JS|UseAsOther, Array, Weak:Object: 0x117fb4370 with butterfly 0x8000e4050 (Structure %BV:Array), StructureID: 104, bc#37, ExitValid)
54:< 1:->     JSConstant(JS|PureNum|UseAsOther|UseAsInt|ReallyWantsInt, BoolInt32, Int32: 0, bc#37, ExitValid)
93:<!0:->     CheckStructure(Cell:@53, MustGen, [%C7:Array], R:JSCell_structureID, Exits, bc#37, ExitValid)
94:< 1:->     GetButterfly(Check:Cell:@53, Storage|PureInt, R:JSObject_butterfly, Exits, bc#37, ExitValid)
55:<!0:->     GetByVal(Check:KnownCell:@53, Check:Int32:@54, Check:Untyped:@94, Double|MustGen|VarArgs|PureInt, AnyIntAsDouble|NonIntAsdouble, Double+OriginalCopyOnWriteArray+SaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedDoubleProperties, Exits, bc#37, ExitValid)  predicting StringIdent|NonIntAsdouble

GetByVal gets constant Array from @53, and attempt to perform constant folding by leverating CoW state: if the given array's butterfly is CoW and we performed CoW array check for this GetByVal, the array would not be changed as long as the check works.
However, CheckStructure for @53 does not filter anything at AI. So, if @53 is CopyOnWrite | Contiguous array (not CopyOnWrite | Double array!), GetByVal will get a JSValue. But it does not meet the requirement of GetByVal since it has Double Array mode, and says it returns Double.
Here, CheckStructure is valid because structure of the constant object would be changed. What we should do is additional CoW & ArrayShape check in GetByVal when folding since this node leverages CoW's interesting feature,
"If CoW array check (CheckStructure etc.) is emitted by GetByVal's DFG::ArrayMode, the content is not changed from the creation!".

This patch adds ArrayShape check in addition to CoW status check in GetByVal.

Unfortunately, this crash is very flaky. In the above case, if @53 stays GetLocal after the constant folding phase, this issue does not occur. We can see this crash in r238109, but it is really hard to reproduce it in the current ToT.
I verified this fix works in r238109 with the attached test.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::fixTypeForRepresentation):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239964 268f45cc-cd09-0410-ab3c-d52691b4dbfc
JSTests/ChangeLog
JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGAbstractValue.cpp