Attachment 359099 Details for Bug 193413 – Patch

[patch] Patch

bug-193413-20190114164928.patch (text/plain), 8.35 KB, created by Yusuke Suzuki on 2019-01-14 16:49:28 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-14 16:49:28 PST

Size: 8.35 KB

patch

obsolete

>Subversion Revision: 239955
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index cc5b39291b6d97b69d9188a35eb12dcc3fe635e6..b19384cf763d9b15ff2cd082035b86e2a7b11b35 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,46 @@
>+2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [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 NOBODY (OOPS!).
>+
>+        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):
>+
> 2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [JSC] Do not use asArrayModes() with Structures because it discards TypedArray information
>diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>index 2c55ac3e330630b516b3f5dc26f172436ac3d9b5..f87fcd532c12f72bedc816b9f560a1eb58bec053 100644
>--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>@@ -1934,9 +1934,17 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
>                         return false;
> 
>                     Structure* structure = m_vm.getStructure(structureIDLate);
>-
>-                    if (!isCopyOnWrite(structure->indexingMode()))
>+                    switch (node->arrayMode().type()) {
>+                    case Array::Int32:
>+                    case Array::Contiguous:
>+                    case Array::Double:
>+                        if (structure->indexingMode() != (toIndexingShape(node->arrayMode().type()) | CopyOnWrite | IsArray))
>+                            return false;
>+                        break;
>+                    default:
>                         return false;
>+                    }
>+                    ASSERT(isCopyOnWrite(structure->indexingMode()));
> 
>                     JSImmutableButterfly* immutableButterfly = JSImmutableButterfly::fromButterfly(butterfly);
>                     if (index < immutableButterfly->length()) {
>diff --git a/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp b/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
>index ae2f15fd68403fcfb3ba9130886af983c1f944da..d2f83ac120d26f802b49cb17754a9f21ec50ef53 100644
>--- a/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
>@@ -186,7 +186,7 @@ void AbstractValue::fixTypeForRepresentation(Graph& graph, NodeFlags representat
> {
>     if (representation == NodeResultDouble) {
>         if (m_value) {
>-            ASSERT(m_value.isNumber());
>+            DFG_ASSERT(graph, node, m_value.isNumber());
>             if (m_value.isInt32())
>                 m_value = jsDoubleNumber(m_value.asNumber());
>         }
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index b1f15c122d3f62d5e93e83a0263d50986a3f996c..309d508a91192173e70c4cb82de97b1a271e8b31 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [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 NOBODY (OOPS!).
>+
>+        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):
>+
> 2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [JSC] Do not use asArrayModes() with Structures because it discards TypedArray information
>diff --git a/JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js b/JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..be81502a468f3c7715ee5633db32147a0ddba0d9
>--- /dev/null
>+++ b/JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js
>@@ -0,0 +1,14 @@
>+//@ runDefault("--jitPolicyScale=0")
>+function compareArray(a, b) {
>+    if (b.length !== a.length) {
>+        return;
>+    }
>+    for (var i = 0; i < a.length; i++) {
>+        b[0];
>+    }
>+}
>+compareArray([], [0]);
>+compareArray([0, 'b'].copyWithin(), ['a', 0]);
>+compareArray([0], [1.1]);
>+runString('');
>+for (var i = 0; i < 1e6; ++i);

Flags:

keith_miller: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193413: 359098 | 359099