Strict Equality on objects should only check that one of the two sides is an object.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jun 2015 19:13:54 +0000 (19:13 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jun 2015 19:13:54 +0000 (19:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145992

Source/JavaScriptCore:

This patch adds a new optimization for checking strict equality on objects.
If we speculate that a strict equality comparison has an object on one side
we only need to type check that side. Equality is then determined by a pointer
comparison between the two values (although in the 32-bit case we must also check
that the other side is a cell). Once LICM hoists type checks out of a loop we
can be cleverer about how we choose the operand we type check if both are
speculated to be objects.

For testing I added the addressOf function, which returns the address
of a Cell to the runtime.

Patch by Keith Miller <keith_miller@apple.com> on 2015-06-24
Reviewed by Mark Lam.

* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileStrictEq):
* dfg/DFGSpeculativeJIT.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compileObjectStrictEquality):
(JSC::DFG::SpeculativeJIT::compilePeepHoleObjectStrictEquality):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compileObjectStrictEquality):
(JSC::DFG::SpeculativeJIT::compilePeepHoleObjectStrictEquality):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileCompareStrictEq):
* jsc.cpp:
(GlobalObject::finishCreation):
(functionAddressOf):
* tests/stress/equality-type-checking.js: Added.
(Foo):
(checkStrictEq):
(checkStrictEqOther):

LayoutTests:

Patch by Keith Miller <keith_miller@apple.com> on 2015-06-24
Reviewed by Mark Lam.

Adds a test that checks if strict equality checks with objects properly exit out of DFG code when
dealing with document.all, which is an object that masquerades as undefined.

* js/dom/document-all-strict-eq-expected.txt: Added.
* js/dom/document-all-strict-eq.html: Added.
* js/dom/script-tests/document-all-strict-eq.js: Added.
(f):

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/js/dom/document-all-strict-eq-expected.txt [new file with mode: 0644]
LayoutTests/js/dom/document-all-strict-eq.html [new file with mode: 0644]
LayoutTests/js/dom/script-tests/document-all-strict-eq.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/tests/stress/equality-type-checking.js [new file with mode: 0644]

index 715108c..fcfa711 100644 (file)
@@ -1,3 +1,18 @@
+2015-06-24  Keith Miller  <keith_miller@apple.com>
+
+        Strict Equality on objects should only check that one of the two sides is an object.
+        https://bugs.webkit.org/show_bug.cgi?id=145992
+
+        Reviewed by Mark Lam.
+
+        Adds a test that checks if strict equality checks with objects properly exit out of DFG code when
+        dealing with document.all, which is an object that masquerades as undefined.
+
+        * js/dom/document-all-strict-eq-expected.txt: Added.
+        * js/dom/document-all-strict-eq.html: Added.
+        * js/dom/script-tests/document-all-strict-eq.js: Added.
+        (f):
+
 2015-06-24  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [Win] Test gardening for fast/text/font-weight{,-zh}.html
diff --git a/LayoutTests/js/dom/document-all-strict-eq-expected.txt b/LayoutTests/js/dom/document-all-strict-eq-expected.txt
new file mode 100644 (file)
index 0000000..c1d5b03
--- /dev/null
@@ -0,0 +1,12 @@
+Test to make sure that document.all works properly with strict eq in the DFG
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+document.all was undefined
+PASS f(testObj, document.all) is false
+PASS f(document.all, document.all) is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/dom/document-all-strict-eq.html b/LayoutTests/js/dom/document-all-strict-eq.html
new file mode 100644 (file)
index 0000000..84fc442
--- /dev/null
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="script-tests/document-all-strict-eq.js"></script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/js/dom/script-tests/document-all-strict-eq.js b/LayoutTests/js/dom/script-tests/document-all-strict-eq.js
new file mode 100644 (file)
index 0000000..2274d8c
--- /dev/null
@@ -0,0 +1,29 @@
+description("Test to make sure that document.all works properly with strict eq in the DFG");
+
+var f = function(a, b) {
+    return a === b
+
+}
+noInline(f);
+
+var testObj = {};
+
+// Warm up f to dfg the code before the masqueradesAsUndefined watchpoint fires
+for (var i = 0; i < 1000; i++) {
+    if (!f(testObj,testObj))
+        testFailed("f(testObj,testObj) should have been true but got false");}
+
+
+
+if (undefined == document.all) {
+    debug("document.all was undefined");
+    shouldBe("f(testObj, document.all)", "false");
+
+    // Get JIT to recompile the jettisoned code after watchpoint fired
+    for (var i = 0; i < 1000; i++) {
+        if (!f(testObj,testObj))
+            testFailed("f(testObj,testObj) should have been true but got false");
+    }
+
+    shouldBe("f(document.all, document.all)", "true");
+}
index 2e9cf3b..23064bf 100644 (file)
@@ -1,3 +1,44 @@
+2015-06-24  Keith Miller  <keith_miller@apple.com>
+
+        Strict Equality on objects should only check that one of the two sides is an object.
+        https://bugs.webkit.org/show_bug.cgi?id=145992
+
+        This patch adds a new optimization for checking strict equality on objects.
+        If we speculate that a strict equality comparison has an object on one side
+        we only need to type check that side. Equality is then determined by a pointer
+        comparison between the two values (although in the 32-bit case we must also check
+        that the other side is a cell). Once LICM hoists type checks out of a loop we
+        can be cleverer about how we choose the operand we type check if both are
+        speculated to be objects.
+
+        For testing I added the addressOf function, which returns the address
+        of a Cell to the runtime.
+
+        Reviewed by Mark Lam.
+
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileStrictEq):
+        * dfg/DFGSpeculativeJIT.h:
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compileObjectStrictEquality):
+        (JSC::DFG::SpeculativeJIT::compilePeepHoleObjectStrictEquality):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compileObjectStrictEquality):
+        (JSC::DFG::SpeculativeJIT::compilePeepHoleObjectStrictEquality):
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileCompareStrictEq):
+        * jsc.cpp:
+        (GlobalObject::finishCreation):
+        (functionAddressOf):
+        * tests/stress/equality-type-checking.js: Added.
+        (Foo):
+        (checkStrictEq):
+        (checkStrictEqOther):
+
 2015-06-24  Mark Lam  <mark.lam@apple.com>
 
         Fixed assertion in JSStringJoiner::join() (regression from r185899).
index 872275d..1affa1a 100644 (file)
@@ -471,7 +471,21 @@ private:
                 fixEdge<StringUse>(node->child2());
                 break;
             }
-            if (node->child1()->shouldSpeculateObject() && node->child2()->shouldSpeculateObject()) {
+            WatchpointSet* masqueradesAsUndefinedWatchpoint = m_graph.globalObjectFor(node->origin.semantic)->masqueradesAsUndefinedWatchpoint();
+            if (masqueradesAsUndefinedWatchpoint->isStillValid()) {
+                
+                if (node->child1()->shouldSpeculateObject()) {
+                    m_graph.watchpoints().addLazily(masqueradesAsUndefinedWatchpoint);
+                    fixEdge<ObjectUse>(node->child1());
+                    break;
+                }
+                if (node->child2()->shouldSpeculateObject()) {
+                    m_graph.watchpoints().addLazily(masqueradesAsUndefinedWatchpoint);
+                    fixEdge<ObjectUse>(node->child2());
+                    break;
+                }
+                
+            } else if (node->child1()->shouldSpeculateObject() && node->child2()->shouldSpeculateObject()) {
                 fixEdge<ObjectUse>(node->child1());
                 fixEdge<ObjectUse>(node->child2());
                 break;
index de54e73..98db3e0 100644 (file)
@@ -3915,6 +3915,36 @@ bool SpeculativeJIT::compileStrictEq(Node* node)
         return false;
     }
 
+    if (node->isBinaryUseKind(ObjectUse, UntypedUse)) {
+        unsigned branchIndexInBlock = detectPeepHoleBranch();
+        if (branchIndexInBlock != UINT_MAX) {
+            Node* branchNode = m_block->at(branchIndexInBlock);
+            compilePeepHoleObjectStrictEquality(node->child1(), node->child2(), branchNode);
+            use(node->child1());
+            use(node->child2());
+            m_indexInBlock = branchIndexInBlock;
+            m_currentNode = branchNode;
+            return true;
+        }
+        compileObjectStrictEquality(node->child1(), node->child2());
+        return false;
+    }
+    
+    if (node->isBinaryUseKind(UntypedUse, ObjectUse)) {
+        unsigned branchIndexInBlock = detectPeepHoleBranch();
+        if (branchIndexInBlock != UINT_MAX) {
+            Node* branchNode = m_block->at(branchIndexInBlock);
+            compilePeepHoleObjectStrictEquality(node->child2(), node->child1(), branchNode);
+            use(node->child1());
+            use(node->child2());
+            m_indexInBlock = branchIndexInBlock;
+            m_currentNode = branchNode;
+            return true;
+        }
+        compileObjectStrictEquality(node->child2(), node->child1());
+        return false;
+    }
+
     if (node->isBinaryUseKind(ObjectUse)) {
         unsigned branchIndexInBlock = detectPeepHoleBranch();
         if (branchIndexInBlock != UINT_MAX) {
index 1ff980c..5d219da 100644 (file)
@@ -2088,8 +2088,10 @@ public:
     void compilePeepHoleBooleanBranch(Node*, Node* branchNode, JITCompiler::RelationalCondition);
     void compilePeepHoleDoubleBranch(Node*, Node* branchNode, JITCompiler::DoubleCondition);
     void compilePeepHoleObjectEquality(Node*, Node* branchNode);
+    void compilePeepHoleObjectStrictEquality(Edge objectChild, Edge otherChild, Node* branchNode);
     void compilePeepHoleObjectToObjectOrOtherEquality(Edge leftChild, Edge rightChild, Node* branchNode);
     void compileObjectEquality(Node*);
+    void compileObjectStrictEquality(Edge objectChild, Edge otherChild);
     void compileObjectToObjectOrOtherEquality(Edge leftChild, Edge rightChild);
     void compileObjectOrOtherLogicalNot(Edge value);
     void compileLogicalNot(Node*);
index 28c5caa..c876151 100644 (file)
@@ -1184,6 +1184,57 @@ void SpeculativeJIT::compileObjectEquality(Node* node)
     booleanResult(resultPayloadGPR, node);
 }
 
+void SpeculativeJIT::compileObjectStrictEquality(Edge objectChild, Edge otherChild)
+{
+    SpeculateCellOperand op1(this, objectChild);
+    JSValueOperand op2(this, otherChild);
+
+    GPRReg op1GPR = op1.gpr();
+    GPRReg op2GPR = op2.payloadGPR();
+
+    DFG_TYPE_CHECK(JSValueSource::unboxedCell(op1GPR), objectChild, SpecObject, m_jit.branchIfNotObject(op1GPR));
+
+    GPRTemporary resultPayload(this, Reuse, op1);
+    GPRReg resultPayloadGPR = resultPayload.gpr();
+    
+    MacroAssembler::Jump op2CellJump = m_jit.branchIfCell(op2.jsValueRegs());
+    
+    m_jit.move(TrustedImm32(0), resultPayloadGPR);
+    MacroAssembler::Jump op2NotCellJump = m_jit.jump();
+    
+    // At this point we know that we can perform a straight-forward equality comparison on pointer
+    // values because we are doing strict equality.
+    op2CellJump.link(&m_jit);
+    m_jit.compare32(MacroAssembler::Equal, op1GPR, op2GPR, resultPayloadGPR);
+    
+    op2NotCellJump.link(&m_jit);
+    booleanResult(resultPayloadGPR, m_currentNode);
+}
+    
+void SpeculativeJIT::compilePeepHoleObjectStrictEquality(Edge objectChild, Edge otherChild, Node* branchNode)
+{
+    BasicBlock* taken = branchNode->branchData()->taken.block;
+    BasicBlock* notTaken = branchNode->branchData()->notTaken.block;
+
+    SpeculateCellOperand op1(this, objectChild);
+    JSValueOperand op2(this, otherChild);
+    
+    GPRReg op1GPR = op1.gpr();
+    GPRReg op2GPR = op2.payloadGPR();
+
+    DFG_TYPE_CHECK(JSValueSource::unboxedCell(op1GPR), objectChild, SpecObject, m_jit.branchIfNotObject(op1GPR));
+
+    branch32(MacroAssembler::NotEqual, op2.tagGPR(), TrustedImm32(JSValue::CellTag), notTaken);
+    
+    if (taken == nextBlock()) {
+        branch32(MacroAssembler::NotEqual, op1GPR, op2GPR, notTaken);
+        jump(taken);
+    } else {
+        branch32(MacroAssembler::Equal, op1GPR, op2GPR, taken);
+        jump(notTaken);
+    }
+}
+
 void SpeculativeJIT::compileObjectToObjectOrOtherEquality(Edge leftChild, Edge rightChild)
 {
     SpeculateCellOperand op1(this, leftChild);
index 27b77df..a67b2f3 100644 (file)
@@ -1296,6 +1296,47 @@ void SpeculativeJIT::compileObjectEquality(Node* node)
     jsValueResult(resultGPR, m_currentNode, DataFormatJSBoolean);
 }
 
+void SpeculativeJIT::compileObjectStrictEquality(Edge objectChild, Edge otherChild)
+{
+    SpeculateCellOperand op1(this, objectChild);
+    JSValueOperand op2(this, otherChild);
+    GPRTemporary result(this);
+
+    GPRReg op1GPR = op1.gpr();
+    GPRReg op2GPR = op2.gpr();
+    GPRReg resultGPR = result.gpr();
+
+    DFG_TYPE_CHECK(JSValueSource::unboxedCell(op1GPR), objectChild, SpecObject, m_jit.branchIfNotObject(op1GPR));
+
+    // At this point we know that we can perform a straight-forward equality comparison on pointer
+    // values because we are doing strict equality.
+    m_jit.compare64(MacroAssembler::Equal, op1GPR, op2GPR, resultGPR);
+    m_jit.or32(TrustedImm32(ValueFalse), resultGPR);
+    jsValueResult(resultGPR, m_currentNode, DataFormatJSBoolean);
+}
+    
+void SpeculativeJIT::compilePeepHoleObjectStrictEquality(Edge objectChild, Edge otherChild, Node* branchNode)
+{
+    BasicBlock* taken = branchNode->branchData()->taken.block;
+    BasicBlock* notTaken = branchNode->branchData()->notTaken.block;
+    
+    SpeculateCellOperand op1(this, objectChild);
+    JSValueOperand op2(this, otherChild);
+    
+    GPRReg op1GPR = op1.gpr();
+    GPRReg op2GPR = op2.gpr();
+    
+    DFG_TYPE_CHECK(JSValueSource::unboxedCell(op1GPR), objectChild, SpecObject, m_jit.branchIfNotObject(op1GPR));
+
+    if (taken == nextBlock()) {
+        branchPtr(MacroAssembler::NotEqual, op1GPR, op2GPR, notTaken);
+        jump(taken);
+    } else {
+        branchPtr(MacroAssembler::Equal, op1GPR, op2GPR, taken);
+        jump(notTaken);
+    }
+}
+
 void SpeculativeJIT::compileObjectToObjectOrOtherEquality(Edge leftChild, Edge rightChild)
 {
     SpeculateCellOperand op1(this, leftChild);
index 3124e02..1fe7987 100644 (file)
@@ -329,6 +329,10 @@ inline CapabilityLevel canCompile(Node* node)
             break;
         if (node->isBinaryUseKind(StringIdentUse))
             break;
+        if (node->isBinaryUseKind(ObjectUse, UntypedUse))
+            break;
+        if (node->isBinaryUseKind(UntypedUse, ObjectUse))
+            break;
         if (node->isBinaryUseKind(ObjectUse))
             break;
         if (node->isBinaryUseKind(BooleanUse))
index 2572f7a..d5ce0d7 100644 (file)
@@ -4157,7 +4157,23 @@ private:
                 m_out.equal(lowStringIdent(m_node->child1()), lowStringIdent(m_node->child2())));
             return;
         }
+
+        if (m_node->isBinaryUseKind(ObjectUse, UntypedUse)) {
+            setBoolean(
+                m_out.equal(
+                    lowNonNullObject(m_node->child1()),
+                    lowJSValue(m_node->child2())));
+            return;
+        }
         
+        if (m_node->isBinaryUseKind(UntypedUse, ObjectUse)) {
+            setBoolean(
+                m_out.equal(
+                    lowNonNullObject(m_node->child2()),
+                    lowJSValue(m_node->child1())));
+            return;
+        }
+
         if (m_node->isBinaryUseKind(ObjectUse)) {
             setBoolean(
                 m_out.equal(
index 9127251..02c6731 100644 (file)
@@ -458,6 +458,7 @@ static EncodedJSValue JSC_HOST_CALL functionFullGC(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionEdenGC(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionHeapSize(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDeleteAllCompiledCode(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionAddressOf(ExecState*);
 #ifndef NDEBUG
 static EncodedJSValue JSC_HOST_CALL functionReleaseExecutableMemory(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDumpCallFrame(ExecState*);
@@ -599,6 +600,7 @@ protected:
         addFunction(vm, "edenGC", functionEdenGC, 0);
         addFunction(vm, "gcHeapSize", functionHeapSize, 0);
         addFunction(vm, "deleteAllCompiledCode", functionDeleteAllCompiledCode, 0);
+        addFunction(vm, "addressOf", functionAddressOf, 1);
 #ifndef NDEBUG
         addFunction(vm, "dumpCallFrame", functionDumpCallFrame, 0);
         addFunction(vm, "releaseExecutableMemory", functionReleaseExecutableMemory, 0);
@@ -877,6 +879,21 @@ EncodedJSValue JSC_HOST_CALL functionDeleteAllCompiledCode(ExecState* exec)
     return JSValue::encode(jsUndefined());
 }
 
+// This function is not generally very helpful in 64-bit code as the tag and payload
+// share a register. But in 32-bit JITed code the tag may not be checked if an
+// optimization removes type checking requirements, such as in ===.
+EncodedJSValue JSC_HOST_CALL functionAddressOf(ExecState* exec)
+{
+    JSValue value = exec->argument(0);
+    if (!value.isCell())
+        return JSValue::encode(jsUndefined());
+    // Need to cast to uint64_t so bitwise_cast will play along.
+    uint64_t asNumber = reinterpret_cast<uint64_t>(value.asCell());
+    EncodedJSValue returnValue = JSValue::encode(jsNumber(bitwise_cast<double>(asNumber)));
+    return returnValue;
+}
+
+
 #ifndef NDEBUG
 EncodedJSValue JSC_HOST_CALL functionReleaseExecutableMemory(ExecState* exec)
 {
diff --git a/Source/JavaScriptCore/tests/stress/equality-type-checking.js b/Source/JavaScriptCore/tests/stress/equality-type-checking.js
new file mode 100644 (file)
index 0000000..5c1bf75
--- /dev/null
@@ -0,0 +1,34 @@
+/***
+ * There was a bug on 32-bit builds where === with objects would not check the tag
+ * when determining equality via pointer comparison.
+ */
+
+"use strict";
+
+function Foo() {}
+
+function checkStrictEq(a, b) {
+    return a === b;
+}
+noInline(checkStrictEq);
+
+function checkStrictEqOther(a, b) {
+    return a === b;
+}
+noInline(checkStrictEqOther);
+
+var foo = new Foo();
+var address = addressOf(foo);
+
+if (address === undefined)
+    throw "Error: address should not be undefined";
+
+if (foo === address || address === foo)
+    throw "Error: an address should not be equal to it's object";
+
+for (var i = 0; i < 10000000; i++) {
+    if (checkStrictEq(foo, address))
+        throw "Error: an address should not be equal to it's object";
+    if (checkStrictEqOther(address,foo))
+        throw "Error: an address should not be equal to it's object";
+}