[DFG][FTL] operationHasIndexedProperty does not consider negative int32_t

https://bugs.webkit.org/show_bug.cgi?id=180190

Reviewed by Mark Lam.

JSTests:

* stress/operation-in-may-have-negative-int32-array-storage.js: Added.
(shouldBe):
(test1):
* stress/operation-in-may-have-negative-int32-contiguous-array.js: Added.
(shouldBe):
(test1):
* stress/operation-in-may-have-negative-int32-double-array.js: Added.
(shouldBe):
(test1):
* stress/operation-in-may-have-negative-int32-generic-array.js: Added.
(shouldBe):
(test1):
* stress/operation-in-may-have-negative-int32-int32-array.js: Added.
(shouldBe):
(test1):
* stress/operation-in-may-have-negative-int32.js: Added.
(shouldBe):
(test2):
* stress/operation-in-negative-int32-cast.js: Added.
(shouldBe):
(test1):

Source/JavaScriptCore:

If DFG HasIndexedProperty node observes negative index, it goes to a slow
path by calling operationHasIndexedProperty. The problem is that
operationHasIndexedProperty does not account negative index. Negative index
was used as uint32 array index.

In this patch we add a path for negative index in operationHasIndexedProperty.
And rename it to operationHasIndexedPropertyByInt to make intension clear.
We also move operationHasIndexedPropertyByInt from JITOperations to DFGOperations
since it is only used in DFG and FTL.

While fixing this bug, we found that our op_in does not record OutOfBound feedback.
This causes repeated OSR exit and significantly regresses the performance. We opened
a bug to track this issue[1].

[1]: https://bugs.webkit.org/show_bug.cgi?id=180192

* dfg/DFGOperations.cpp:
* dfg/DFGOperations.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileHasIndexedProperty):
* jit/JITOperations.cpp:
* jit/JITOperations.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225342 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 46fe130..e558624 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,32 @@
+2017-11-30  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [DFG][FTL] operationHasIndexedProperty does not consider negative int32_t
+        https://bugs.webkit.org/show_bug.cgi?id=180190
+
+        Reviewed by Mark Lam.
+
+        * stress/operation-in-may-have-negative-int32-array-storage.js: Added.
+        (shouldBe):
+        (test1):
+        * stress/operation-in-may-have-negative-int32-contiguous-array.js: Added.
+        (shouldBe):
+        (test1):
+        * stress/operation-in-may-have-negative-int32-double-array.js: Added.
+        (shouldBe):
+        (test1):
+        * stress/operation-in-may-have-negative-int32-generic-array.js: Added.
+        (shouldBe):
+        (test1):
+        * stress/operation-in-may-have-negative-int32-int32-array.js: Added.
+        (shouldBe):
+        (test1):
+        * stress/operation-in-may-have-negative-int32.js: Added.
+        (shouldBe):
+        (test2):
+        * stress/operation-in-negative-int32-cast.js: Added.
+        (shouldBe):
+        (test1):
+
 2017-11-28  JF Bastien  <jfbastien@apple.com>
 
         Strict and sloppy functions shouldn't share structure

diff --git a/JSTests/stress/operation-in-may-have-negative-int32-array-storage.js b/JSTests/stress/operation-in-may-have-negative-int32-array-storage.js
new file mode 100644 (file)
index 0000000..986b922
--- /dev/null
+++ b/JSTests/stress/operation-in-may-have-negative-int32-array-storage.js
@@ -0,0 +1,19 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var k = -1;
+var o1 = [20];
+o1[k] = 42;
+ensureArrayStorage(o1);
+
+function test1(o)
+{
+    return k in o;
+}
+noInline(test1);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1), true);

diff --git a/JSTests/stress/operation-in-may-have-negative-int32-contiguous-array.js b/JSTests/stress/operation-in-may-have-negative-int32-contiguous-array.js
new file mode 100644 (file)
index 0000000..e574ba7
--- /dev/null
+++ b/JSTests/stress/operation-in-may-have-negative-int32-contiguous-array.js
@@ -0,0 +1,18 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var k = -1;
+var o1 = ["Cocoa"];
+o1[k] = 42;
+
+function test1(o)
+{
+    return k in o;
+}
+noInline(test1);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1), true);

diff --git a/JSTests/stress/operation-in-may-have-negative-int32-double-array.js b/JSTests/stress/operation-in-may-have-negative-int32-double-array.js
new file mode 100644 (file)
index 0000000..1301418
--- /dev/null
+++ b/JSTests/stress/operation-in-may-have-negative-int32-double-array.js
@@ -0,0 +1,18 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var k = -1;
+var o1 = [42.5];
+o1[k] = 300.2;
+
+function test1(o)
+{
+    return k in o;
+}
+noInline(test1);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1), true);

diff --git a/JSTests/stress/operation-in-may-have-negative-int32-generic-array.js b/JSTests/stress/operation-in-may-have-negative-int32-generic-array.js
new file mode 100644 (file)
index 0000000..75fb263
--- /dev/null
+++ b/JSTests/stress/operation-in-may-have-negative-int32-generic-array.js
@@ -0,0 +1,18 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var k = -1;
+var o1 = [];
+o1[k] = 42;
+
+function test1(o)
+{
+    return k in o;
+}
+noInline(test1);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1), true);

diff --git a/JSTests/stress/operation-in-may-have-negative-int32-int32-array.js b/JSTests/stress/operation-in-may-have-negative-int32-int32-array.js
new file mode 100644 (file)
index 0000000..52d6da9
--- /dev/null
+++ b/JSTests/stress/operation-in-may-have-negative-int32-int32-array.js
@@ -0,0 +1,18 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var k = -1;
+var o1 = [20];
+o1[k] = 42;
+
+function test1(o)
+{
+    return k in o;
+}
+noInline(test1);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1), true);

diff --git a/JSTests/stress/operation-in-may-have-negative-int32.js b/JSTests/stress/operation-in-may-have-negative-int32.js
new file mode 100644 (file)
index 0000000..f54b2ee
--- /dev/null
+++ b/JSTests/stress/operation-in-may-have-negative-int32.js
@@ -0,0 +1,32 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var k = -1;
+var o1 = {};
+o1[k] = true;
+var o2 = {};
+
+function test1(o)
+{
+    return k in o;
+}
+noInline(test1);
+
+function test2(o)
+{
+    return k in o;
+}
+noInline(test2);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1), true);
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o2), false);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test2(o2), false);
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test2(o1), true);

diff --git a/JSTests/stress/operation-in-negative-int32-cast.js b/JSTests/stress/operation-in-negative-int32-cast.js
new file mode 100644 (file)
index 0000000..685245c
--- /dev/null
+++ b/JSTests/stress/operation-in-negative-int32-cast.js
@@ -0,0 +1,20 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+var INT32_MIN = -2147483648;
+var INT32_MIN_IN_UINT32 = 0x80000000;
+var o1 = [];
+o1[INT32_MIN_IN_UINT32] = true;
+ensureArrayStorage(o1);
+
+function test1(o, key)
+{
+    return key in o;
+}
+noInline(test1);
+
+for (var i = 0; i < 1e6; ++i)
+    shouldBe(test1(o1, INT32_MIN), false);

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 96e0ce0..9e977f2 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,37 @@
+2017-11-30  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [DFG][FTL] operationHasIndexedProperty does not consider negative int32_t
+        https://bugs.webkit.org/show_bug.cgi?id=180190
+
+        Reviewed by Mark Lam.
+
+        If DFG HasIndexedProperty node observes negative index, it goes to a slow
+        path by calling operationHasIndexedProperty. The problem is that
+        operationHasIndexedProperty does not account negative index. Negative index
+        was used as uint32 array index.
+
+        In this patch we add a path for negative index in operationHasIndexedProperty.
+        And rename it to operationHasIndexedPropertyByInt to make intension clear.
+        We also move operationHasIndexedPropertyByInt from JITOperations to DFGOperations
+        since it is only used in DFG and FTL.
+
+        While fixing this bug, we found that our op_in does not record OutOfBound feedback.
+        This causes repeated OSR exit and significantly regresses the performance. We opened
+        a bug to track this issue[1].
+
+        [1]: https://bugs.webkit.org/show_bug.cgi?id=180192
+
+        * dfg/DFGOperations.cpp:
+        * dfg/DFGOperations.h:
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileHasIndexedProperty):
+        * jit/JITOperations.cpp:
+        * jit/JITOperations.h:
+
 2017-11-30  Michael Saboff  <msaboff@apple.com>
 
         Allow JSC command line tool to accept UTF8

diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
index e114769..77bca04 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
@@ -634,7 +634,7 @@ ALWAYS_INLINE EncodedJSValue getByValCellInt(ExecState* exec, JSCell* base, int3
     NativeCallFrameTracer tracer(vm, exec);
     
     if (index < 0) {
-        // Go the slowest way possible becase negative indices don't use indexed storage.
+        // Go the slowest way possible because negative indices don't use indexed storage.
         return JSValue::encode(JSValue(base).get(exec, Identifier::from(exec, index)));
     }
 
@@ -1808,6 +1808,18 @@ char* JIT_OPERATION operationEnsureArrayStorage(ExecState* exec, JSCell* cell)
     return reinterpret_cast<char*>(asObject(cell)->ensureArrayStorage(vm));
 }
 
+EncodedJSValue JIT_OPERATION operationHasIndexedPropertyByInt(ExecState* exec, JSCell* baseCell, int32_t subscript, int32_t internalMethodType)
+{
+    VM& vm = exec->vm();
+    NativeCallFrameTracer tracer(&vm, exec);
+    JSObject* object = baseCell->toObject(exec, exec->lexicalGlobalObject());
+    if (UNLIKELY(subscript < 0)) {
+        // Go the slowest way possible because negative indices don't use indexed storage.
+        return JSValue::encode(jsBoolean(object->hasPropertyGeneric(exec, Identifier::from(exec, subscript), static_cast<PropertySlot::InternalMethodType>(internalMethodType))));
+    }
+    return JSValue::encode(jsBoolean(object->hasPropertyGeneric(exec, subscript, static_cast<PropertySlot::InternalMethodType>(internalMethodType))));
+}
+
 StringImpl* JIT_OPERATION operationResolveRope(ExecState* exec, JSString* string)
 {
     VM& vm = exec->vm();

diff --git a/Source/JavaScriptCore/dfg/DFGOperations.h b/Source/JavaScriptCore/dfg/DFGOperations.h
index abf06b6..aad0bef 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGOperations.h
+++ b/Source/JavaScriptCore/dfg/DFGOperations.h
@@ -80,6 +80,7 @@ EncodedJSValue JIT_OPERATION operationGetByIdWithThis(ExecState*, EncodedJSValue
 EncodedJSValue JIT_OPERATION operationGetByValWithThis(ExecState*, EncodedJSValue, EncodedJSValue, EncodedJSValue) WTF_INTERNAL;
 EncodedJSValue JIT_OPERATION operationGetPrototypeOf(ExecState*, EncodedJSValue) WTF_INTERNAL;
 EncodedJSValue JIT_OPERATION operationGetPrototypeOfObject(ExecState*, JSObject*) WTF_INTERNAL;
+EncodedJSValue JIT_OPERATION operationHasIndexedPropertyByInt(ExecState*, JSCell*, int32_t, int32_t);
 char* JIT_OPERATION operationNewArray(ExecState*, Structure*, void*, size_t) WTF_INTERNAL;
 char* JIT_OPERATION operationNewArrayBuffer(ExecState*, Structure*, size_t, size_t) WTF_INTERNAL;
 char* JIT_OPERATION operationNewEmptyArray(ExecState*, Structure*) WTF_INTERNAL;

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
index bfda638..6c82f17 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
@@ -5271,7 +5271,7 @@ void SpeculativeJIT::compile(Node* node)
         moveTrueTo(resultPayloadGPR);
         MacroAssembler::Jump done = m_jit.jump();
 
-        addSlowPathGenerator(slowPathCall(slowCases, this, operationHasIndexedProperty, JSValueRegs(resultTagGPR, resultPayloadGPR), baseGPR, indexGPR, static_cast<int32_t>(node->internalMethodType())));
+        addSlowPathGenerator(slowPathCall(slowCases, this, operationHasIndexedPropertyByInt, JSValueRegs(resultTagGPR, resultPayloadGPR), baseGPR, indexGPR, static_cast<int32_t>(node->internalMethodType())));
         
         done.link(&m_jit);
         booleanResult(resultPayloadGPR, node);

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
index 0fde482..b338644 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
@@ -5733,7 +5733,7 @@ void SpeculativeJIT::compile(Node* node)
         }
         }
 
-        addSlowPathGenerator(slowPathCall(slowCases, this, operationHasIndexedProperty, resultGPR, baseGPR, indexGPR, static_cast<int32_t>(node->internalMethodType())));
+        addSlowPathGenerator(slowPathCall(slowCases, this, operationHasIndexedPropertyByInt, resultGPR, baseGPR, indexGPR, static_cast<int32_t>(node->internalMethodType())));
         
         jsValueResult(resultGPR, node, DataFormatJSBoolean);
         break;

diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
index 11c6701..0b6f095 100644 (file)
--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
@@ -9428,7 +9428,7 @@ private:
             m_out.appendTo(slowCase, continuation);
             ValueFromBlock slowResult = m_out.anchor(m_out.equal(
                 m_out.constInt64(JSValue::encode(jsBoolean(true))), 
-                vmCall(Int64, m_out.operation(operationHasIndexedProperty), m_callFrame, base, index, internalMethodType)));
+                vmCall(Int64, m_out.operation(operationHasIndexedPropertyByInt), m_callFrame, base, index, internalMethodType)));
             m_out.jump(continuation);
 
             m_out.appendTo(continuation, lastNext);
@@ -9464,7 +9464,7 @@ private:
             m_out.appendTo(slowCase, continuation);
             ValueFromBlock slowResult = m_out.anchor(m_out.equal(
                 m_out.constInt64(JSValue::encode(jsBoolean(true))), 
-                vmCall(Int64, m_out.operation(operationHasIndexedProperty), m_callFrame, base, index, internalMethodType)));
+                vmCall(Int64, m_out.operation(operationHasIndexedPropertyByInt), m_callFrame, base, index, internalMethodType)));
             m_out.jump(continuation);
             
             m_out.appendTo(continuation, lastNext);

diff --git a/Source/JavaScriptCore/jit/JITOperations.cpp b/Source/JavaScriptCore/jit/JITOperations.cpp
index acd23e8..b6ac876 100644 (file)
--- a/Source/JavaScriptCore/jit/JITOperations.cpp
+++ b/Source/JavaScriptCore/jit/JITOperations.cpp
@@ -2398,14 +2398,6 @@ EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState* exec, Encode
     return JSValue::encode(jsBoolean(base->hasPropertyGeneric(exec, asString(propertyName)->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));
 }
 
-EncodedJSValue JIT_OPERATION operationHasIndexedProperty(ExecState* exec, JSCell* baseCell, int32_t subscript, int32_t internalMethodType)
-{
-    VM& vm = exec->vm();
-    NativeCallFrameTracer tracer(&vm, exec);
-    JSObject* object = baseCell->toObject(exec, exec->lexicalGlobalObject());
-    return JSValue::encode(jsBoolean(object->hasPropertyGeneric(exec, subscript, static_cast<PropertySlot::InternalMethodType>(internalMethodType))));
-}
-    
 JSCell* JIT_OPERATION operationGetPropertyEnumerator(ExecState* exec, JSCell* cell)
 {
     VM& vm = exec->vm();

diff --git a/Source/JavaScriptCore/jit/JITOperations.h b/Source/JavaScriptCore/jit/JITOperations.h
index 6a5941c..f7c2a4e 100644 (file)
--- a/Source/JavaScriptCore/jit/JITOperations.h
+++ b/Source/JavaScriptCore/jit/JITOperations.h
@@ -461,7 +461,6 @@ int32_t JIT_OPERATION operationCheckIfExceptionIsUncatchableAndNotifyProfiler(Ex
 int32_t JIT_OPERATION operationInstanceOfCustom(ExecState*, EncodedJSValue encodedValue, JSObject* constructor, EncodedJSValue encodedHasInstance) WTF_INTERNAL;
 
 EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState*, EncodedJSValue, JSCell*);
-EncodedJSValue JIT_OPERATION operationHasIndexedProperty(ExecState*, JSCell*, int32_t, int32_t);
 JSCell* JIT_OPERATION operationGetPropertyEnumerator(ExecState*, JSCell*);
 EncodedJSValue JIT_OPERATION operationNextEnumeratorPname(ExecState*, JSCell*, int32_t);
 JSCell* JIT_OPERATION operationToIndexString(ExecState*, int32_t);