Attachment 359705 Details for Bug 193652 – Patch

[patch] Patch

bug-193652-20190121173057.patch (text/plain), 4.80 KB, created by Caio Lima on 2019-01-21 12:30:59 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Caio Lima

Created: 2019-01-21 12:30:59 PST

Size: 4.80 KB

patch

obsolete

>Subversion Revision: 240235
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index dbc7d46ad189e0e9407da5935177173aa925c0b8..2592c67c14428287a9a0b2d669d18abdad612063 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-01-21  Caio Lima  <ticaiolima@gmail.com>
>+
>+        DoesGC rule is wrong for nodes with BigIntUse
>+        https://bugs.webkit.org/show_bug.cgi?id=193652
>+
>+        Reviewed by Saam Barati.
>+
>+        Former rule was that ValueOp does not GC. However this is wrong, since
>+        these operations can trigger GC and mess up memory management. In the end, this
>+        will generate wrong code because we will have wrong GC epoch value during 
>+        Store Barrier Insertion phase.
>+        We changed this to consider BigIntUse for such nodes and properly return true when
>+        they are BigIntUse.
>+
>+        * dfg/DFGDoesGC.cpp:
>+        (JSC::DFG::doesGC):
>+
> 2019-01-20  Saam Barati  <sbarati@apple.com>
> 
>         DFG: When inlining DataView set* intrinsics we need to set undefined as our result
>diff --git a/Source/JavaScriptCore/dfg/DFGDoesGC.cpp b/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
>index 4f79af6c4cc5c1d9a9581e712441ad703994cb2d..33a20e539a6fd2c26ef114277ac7c69f17c87866 100644
>--- a/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
>@@ -97,14 +97,7 @@ bool doesGC(Graph& graph, Node* node)
>     case ArithTrunc:
>     case ArithFRound:
>     case ArithUnary:
>-    case ValueBitAnd:
>-    case ValueBitOr:
>-    case ValueBitXor:
>-    case ValueAdd:
>-    case ValueSub:
>-    case ValueMul:
>     case ValueNegate:
>-    case ValueDiv:
>     case TryGetById:
>     case GetById:
>     case GetByIdFlush:
>@@ -383,6 +376,13 @@ bool doesGC(Graph& graph, Node* node)
>     case ParseInt: // We might resolve a rope even though we don't clobber anything.
>     case SetAdd:
>     case MapSet:
>+    case ValueBitAnd:
>+    case ValueBitOr:
>+    case ValueBitXor:
>+    case ValueAdd:
>+    case ValueSub:
>+    case ValueMul:
>+    case ValueDiv:
>         return true;
> 
>     case GetIndexedPropertyStorage:
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 9dcfb8a3ba6b7f49a5e06a59083524d42e253358..286670b0088d0e106f61bf3e9fd61ff2f20f3bfe 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-01-21  Caio Lima  <ticaiolima@gmail.com>
>+
>+        DoesGC rule is wrong for nodes with BigIntUse
>+        https://bugs.webkit.org/show_bug.cgi?id=193652
>+
>+        Reviewed by Saam Barati.
>+
>+        * stress/big-int-value-op-update-gc-rules.js: Added.
>+        (assert):
>+        (doesGCAdd):
>+        (doesGCSub):
>+        (doesGCDiv):
>+        (doesGCMul):
>+        (doesGCBitAnd):
>+        (doesGCBitOr):
>+        (doesGCBitXor):
>+
> 2019-01-20  Saam Barati  <sbarati@apple.com>
> 
>         DFG: When inlining DataView set* intrinsics we need to set undefined as our result
>diff --git a/JSTests/stress/big-int-value-op-update-gc-rules.js b/JSTests/stress/big-int-value-op-update-gc-rules.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..56e526f7e1103eafa01e82ce9e724d23cf142e77
>--- /dev/null
>+++ b/JSTests/stress/big-int-value-op-update-gc-rules.js
>@@ -0,0 +1,105 @@
>+//@ runBigIntEnabled
>+
>+function assert(a, e) {
>+    if (a !== e)
>+        throw new Error("Expected: " + e + " but got: " + a);
>+}
>+
>+function doesGCAdd(a) {
>+    let o = {};
>+    let c = a + 1n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCAdd);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCAdd(3n);
>+    assert(o.b, 4n);
>+}
>+
>+function doesGCSub(a) {
>+    let o = {};
>+    let c = a - 1n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCSub);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCSub(3n);
>+    assert(o.b, 2n);
>+}
>+
>+function doesGCDiv(a) {
>+    let o = {};
>+    let c = a / 2n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCDiv);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCDiv(4n);
>+    assert(o.b, 2n);
>+}
>+
>+function doesGCMul(a) {
>+    let o = {};
>+    let c = a * 2n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCMul);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCMul(4n);
>+    assert(o.b, 8n);
>+}
>+
>+function doesGCBitAnd(a) {
>+    let o = {};
>+    let c = a & 0b11n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCBitAnd);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCBitAnd(0b1010n);
>+    assert(o.b, 0b10n);
>+}
>+
>+function doesGCBitOr(a) {
>+    let o = {};
>+    let c = a | 0b11n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCBitOr);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCBitOr(0b10n);
>+    assert(o.b, 0b11n);
>+}
>+
>+function doesGCBitXor(a) {
>+    let o = {};
>+    let c = a ^ 0b11n;
>+    o.b = c;
>+
>+    return o;
>+}
>+noInline(doesGCBitXor);
>+
>+for (var i = 0; i < 10000; i++) {
>+    let o = doesGCBitXor(0b10n);
>+    assert(o.b, 0b1n);
>+}
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 193652: 359701 | 359704 | 359705