[webkit-changes] [282857] trunk

Tue, 21 Sep 2021 18:14:15 -0700

Title: [282857] trunk
Revision
282857
Author
[email protected]
Date
2021-09-21 18:14:06 -0700 (Tue, 21 Sep 2021)

Log Message

[JSC] CompareStrictEq is omitting String check incorrectly
https://bugs.webkit.org/show_bug.cgi?id=230582
rdar://83237121

Reviewed by Mark Lam.

JSTests:

* stress/compare-strict-eq-string-check.js: Added.
(foo):
(bar):

Source/_javascript_Core:

1. Add left and right prefixes to neitherDoubleNorHeapBigIntChild and notDoubleChild edges since
   registers are named with left and right. Without this prefix, it is hard to follow in the code.
2. Remove leftGPR and rightGPR and use leftRegs.payloadGPR() and rightRegs.payloadGPR() to avoid
   having different variables pointing to the same registers.
3. DFG needsTypeCheck is done with wrong type filters. As a result, necessary checks are omitted.
   This patch fixes that. FTL does not have the same problem.

* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compileNeitherDoubleNorHeapBigIntToNotDoubleStrictEquality):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (282856 => 282857)


--- trunk/JSTests/ChangeLog	2021-09-22 00:34:47 UTC (rev 282856)
+++ trunk/JSTests/ChangeLog	2021-09-22 01:14:06 UTC (rev 282857)
@@ -1,3 +1,15 @@
+2021-09-21  Yusuke Suzuki  <[email protected]>
+
+        [JSC] CompareStrictEq is omitting String check incorrectly
+        https://bugs.webkit.org/show_bug.cgi?id=230582
+        rdar://83237121
+
+        Reviewed by Mark Lam.
+
+        * stress/compare-strict-eq-string-check.js: Added.
+        (foo):
+        (bar):
+
 2021-09-21  Justin Michaud  <[email protected]>
 
         Differential testing: live statement don't execute

Added: trunk/JSTests/stress/compare-strict-eq-string-check.js (0 => 282857)


--- trunk/JSTests/stress/compare-strict-eq-string-check.js	                        (rev 0)
+++ trunk/JSTests/stress/compare-strict-eq-string-check.js	2021-09-22 01:14:06 UTC (rev 282857)
@@ -0,0 +1,9 @@
+function foo() {}
+function bar(a0, a1) {
+  a0 instanceof foo;
+  a0 === a1;
+}
+for (let i=0; i<100000; i++) {
+  bar({});
+  bar(1n, '');
+}

Modified: trunk/Source/_javascript_Core/ChangeLog (282856 => 282857)


--- trunk/Source/_javascript_Core/ChangeLog	2021-09-22 00:34:47 UTC (rev 282856)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-09-22 01:14:06 UTC (rev 282857)
@@ -1,3 +1,23 @@
+2021-09-21  Yusuke Suzuki  <[email protected]>
+
+        [JSC] CompareStrictEq is omitting String check incorrectly
+        https://bugs.webkit.org/show_bug.cgi?id=230582
+        rdar://83237121
+
+        Reviewed by Mark Lam.
+
+        1. Add left and right prefixes to neitherDoubleNorHeapBigIntChild and notDoubleChild edges since
+           registers are named with left and right. Without this prefix, it is hard to follow in the code.
+        2. Remove leftGPR and rightGPR and use leftRegs.payloadGPR() and rightRegs.payloadGPR() to avoid
+           having different variables pointing to the same registers.
+        3. DFG needsTypeCheck is done with wrong type filters. As a result, necessary checks are omitted.
+           This patch fixes that. FTL does not have the same problem.
+
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compileNeitherDoubleNorHeapBigIntToNotDoubleStrictEquality):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):
+
 2021-09-21  Mark Lam  <[email protected]>
 
         Replace a few ASSERTs with static_asserts in the ARM64 MacroAssemblers.

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (282856 => 282857)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2021-09-22 00:34:47 UTC (rev 282856)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2021-09-22 01:14:06 UTC (rev 282857)
@@ -336,13 +336,13 @@
     }
 }
 
-void SpeculativeJIT::compileNeitherDoubleNorHeapBigIntToNotDoubleStrictEquality(Node* node, Edge neitherDoubleNorHeapBigIntChild, Edge notDoubleChild)
+void SpeculativeJIT::compileNeitherDoubleNorHeapBigIntToNotDoubleStrictEquality(Node* node, Edge leftNeitherDoubleNorHeapBigIntChild, Edge rightNotDoubleChild)
 {
-    ASSERT(neitherDoubleNorHeapBigIntChild.useKind() == NeitherDoubleNorHeapBigIntUse);
-    ASSERT(notDoubleChild.useKind() == NotDoubleUse);
+    ASSERT(leftNeitherDoubleNorHeapBigIntChild.useKind() == NeitherDoubleNorHeapBigIntUse);
+    ASSERT(rightNotDoubleChild.useKind() == NotDoubleUse);
 
-    JSValueOperand left(this, neitherDoubleNorHeapBigIntChild, ManualOperandSpeculation);
-    JSValueOperand right(this, notDoubleChild, ManualOperandSpeculation);
+    JSValueOperand left(this, leftNeitherDoubleNorHeapBigIntChild, ManualOperandSpeculation);
+    JSValueOperand right(this, rightNotDoubleChild, ManualOperandSpeculation);
 
     GPRTemporary temp(this);
     GPRTemporary leftTemp(this);
@@ -352,8 +352,6 @@
     JSValueRegs leftRegs = left.jsValueRegs();
     JSValueRegs rightRegs = right.jsValueRegs();
 
-    GPRReg leftGPR = leftRegs.payloadGPR();
-    GPRReg rightGPR = rightRegs.payloadGPR();
     GPRReg tempGPR = temp.gpr();
     GPRReg leftTempGPR = leftTemp.gpr();
     GPRReg rightTempGPR = rightTemp.gpr();
@@ -362,7 +360,7 @@
 
     // We try to avoid repeated and redundant checks here, which leads to the following pseudo-code:
     /*
-     if (left == true) {
+     if (left == right) {
        speculateNeitherDoubleNorHeapBigInt(left);
        return true;
      }
@@ -385,44 +383,40 @@
     JITCompiler::JumpList trueCase;
     JITCompiler::JumpList falseCase;
 
-    JITCompiler::Jump notEqual = m_jit.branch64(JITCompiler::NotEqual, leftGPR, rightGPR);
+    JITCompiler::Jump notEqual = m_jit.branch64(JITCompiler::NotEqual, leftRegs.payloadGPR(), rightRegs.payloadGPR());
     // We cannot use speculateNeitherDoubleNorHeapBigInt here, because it updates the interpreter state, and we can skip over it.
     // So we would always skip the speculateNotDouble that follows.
-    if (needsTypeCheck(neitherDoubleNorHeapBigIntChild, ~SpecFullDouble)) {
-        if (needsTypeCheck(neitherDoubleNorHeapBigIntChild, ~SpecInt32Only))
+    if (needsTypeCheck(leftNeitherDoubleNorHeapBigIntChild, ~SpecFullDouble)) {
+        if (needsTypeCheck(leftNeitherDoubleNorHeapBigIntChild, ~SpecInt32Only))
             trueCase.append(m_jit.branchIfInt32(leftRegs));
-        speculationCheck(BadType, leftRegs, neitherDoubleNorHeapBigIntChild.node(), m_jit.branchIfNumber(leftRegs, tempGPR));
+        speculationCheck(BadType, leftRegs, leftNeitherDoubleNorHeapBigIntChild.node(), m_jit.branchIfNumber(leftRegs, tempGPR));
     }
-    if (needsTypeCheck(neitherDoubleNorHeapBigIntChild, ~SpecHeapBigInt)) {
-        if (needsTypeCheck(neitherDoubleNorHeapBigIntChild, SpecCell))
+    if (needsTypeCheck(leftNeitherDoubleNorHeapBigIntChild, ~SpecHeapBigInt)) {
+        if (needsTypeCheck(leftNeitherDoubleNorHeapBigIntChild, SpecCell))
             trueCase.append(m_jit.branchIfNotCell(leftRegs));
-        speculationCheck(BadType, leftRegs, neitherDoubleNorHeapBigIntChild.node(), m_jit.branchIfHeapBigInt(leftGPR));
+        speculationCheck(BadType, leftRegs, leftNeitherDoubleNorHeapBigIntChild.node(), m_jit.branchIfHeapBigInt(leftRegs.payloadGPR()));
     }
     trueCase.append(m_jit.jump());
     notEqual.link(&m_jit);
 
-    speculateNotDouble(notDoubleChild, rightRegs, tempGPR);
-    speculateNotDouble(neitherDoubleNorHeapBigIntChild, leftRegs, tempGPR);
+    speculateNotDouble(rightNotDoubleChild, rightRegs, tempGPR);
+    speculateNotDouble(leftNeitherDoubleNorHeapBigIntChild, leftRegs, tempGPR);
 
-    bool leftMayBeNotCell = needsTypeCheck(neitherDoubleNorHeapBigIntChild, SpecCellCheck);
-    if (leftMayBeNotCell)
+    if (needsTypeCheck(leftNeitherDoubleNorHeapBigIntChild, SpecCellCheck))
         falseCase.append(m_jit.branchIfNotCell(leftRegs));
 
-    DFG_TYPE_CHECK(leftRegs, neitherDoubleNorHeapBigIntChild, ~SpecHeapBigInt, m_jit.branchIfHeapBigInt(leftGPR));
+    DFG_TYPE_CHECK(leftRegs, leftNeitherDoubleNorHeapBigIntChild, ~SpecHeapBigInt, m_jit.branchIfHeapBigInt(leftRegs.payloadGPR()));
 
-    bool leftMayBeNotStringKnowingCell = needsTypeCheck(neitherDoubleNorHeapBigIntChild, (~SpecString) & SpecCellCheck);
-    if (leftMayBeNotStringKnowingCell)
-        falseCase.append(m_jit.branchIfNotString(leftGPR));
+    if (needsTypeCheck(leftNeitherDoubleNorHeapBigIntChild, SpecString))
+        falseCase.append(m_jit.branchIfNotString(leftRegs.payloadGPR()));
 
-    bool rightMayBeNotCell = needsTypeCheck(notDoubleChild, SpecCellCheck);
-    if (rightMayBeNotCell)
+    if (needsTypeCheck(rightNotDoubleChild, SpecCellCheck))
         falseCase.append(m_jit.branchIfNotCell(rightRegs));
 
-    bool rightMayBeNotStringKnowingCell = needsTypeCheck(notDoubleChild, (~SpecString) & SpecCellCheck);
-    if (rightMayBeNotStringKnowingCell)
-        falseCase.append(m_jit.branchIfNotString(rightGPR));
+    if (needsTypeCheck(rightNotDoubleChild, SpecString))
+        falseCase.append(m_jit.branchIfNotString(rightRegs.payloadGPR()));
 
-    compileStringEquality(node, leftGPR, rightGPR, tempGPR, leftTempGPR, rightTempGPR, leftTemp2GPR, rightTemp2GPR, trueCase, falseCase);
+    compileStringEquality(node, leftRegs.payloadGPR(), rightRegs.payloadGPR(), tempGPR, leftTempGPR, rightTempGPR, leftTemp2GPR, rightTemp2GPR, trueCase, falseCase);
 }
 
 void SpeculativeJIT::nonSpeculativePeepholeStrictEq(Node* node, Node* branchNode, bool invert)

Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (282856 => 282857)


--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp	2021-09-22 00:34:47 UTC (rev 282856)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp	2021-09-22 01:14:06 UTC (rev 282857)
@@ -9878,15 +9878,15 @@
         setBoolean(m_out.phi(Int32, fastTrue, fastFalse, slowResult));
     }
 
-    void compileNeitherDoubleNorHeapBigIntToNotDoubleStrictEquality(Edge neitherDoubleNorHeapBigIntEdge, Edge notDoubleEdge)
+    void compileNeitherDoubleNorHeapBigIntToNotDoubleStrictEquality(Edge leftNeitherDoubleNorHeapBigIntEdge, Edge rightNotDoubleEdge)
     {
-        ASSERT(neitherDoubleNorHeapBigIntEdge.useKind() == NeitherDoubleNorHeapBigIntUse);
-        ASSERT(notDoubleEdge.useKind() == NotDoubleUse);
+        ASSERT(leftNeitherDoubleNorHeapBigIntEdge.useKind() == NeitherDoubleNorHeapBigIntUse);
+        ASSERT(rightNotDoubleEdge.useKind() == NotDoubleUse);
 
-        LValue leftValue = lowJSValue(neitherDoubleNorHeapBigIntEdge, ManualOperandSpeculation);
-        LValue rightValue = lowJSValue(notDoubleEdge, ManualOperandSpeculation);
-        SpeculatedType leftValueType = provenType(neitherDoubleNorHeapBigIntEdge);
-        SpeculatedType rightValueType = provenType(notDoubleEdge);
+        LValue leftValue = lowJSValue(leftNeitherDoubleNorHeapBigIntEdge, ManualOperandSpeculation);
+        LValue rightValue = lowJSValue(rightNotDoubleEdge, ManualOperandSpeculation);
+        SpeculatedType leftValueType = provenType(leftNeitherDoubleNorHeapBigIntEdge);
+        SpeculatedType rightValueType = provenType(rightNotDoubleEdge);
 
         LBasicBlock triviallyEqualCase = m_out.newBlock();
         LBasicBlock leftIsNotInt32EqualCase = m_out.newBlock();
@@ -9928,11 +9928,11 @@
         m_out.branch(isInt32(leftValue, leftValueType), unsure(returnTrueBlock), unsure(leftIsNotInt32EqualCase));
 
         m_out.appendTo(leftIsNotInt32EqualCase, leftIsCellEqualCase);
-        typeCheckWithoutUpdatingInterpreter(jsValueValue(leftValue), neitherDoubleNorHeapBigIntEdge, ~SpecFullDouble, isNumber(leftValue));
+        typeCheckWithoutUpdatingInterpreter(jsValueValue(leftValue), leftNeitherDoubleNorHeapBigIntEdge, ~SpecFullDouble, isNumber(leftValue));
         m_out.branch(isCell(leftValue, leftValueType & ~SpecFullNumber), unsure(leftIsCellEqualCase), unsure(returnTrueBlock));
 
         m_out.appendTo(leftIsCellEqualCase, returnTrueBlock);
-        typeCheckWithoutUpdatingInterpreter(jsValueValue(leftValue), neitherDoubleNorHeapBigIntEdge, ~SpecHeapBigInt, isHeapBigInt(leftValue));
+        typeCheckWithoutUpdatingInterpreter(jsValueValue(leftValue), leftNeitherDoubleNorHeapBigIntEdge, ~SpecHeapBigInt, isHeapBigInt(leftValue));
         m_out.jump(returnTrueBlock);
 
         m_out.appendTo(returnTrueBlock, notTriviallyEqualCase);
@@ -9940,13 +9940,13 @@
         m_out.jump(continuation);
 
         m_out.appendTo(notTriviallyEqualCase, leftIsCell);
-        speculateNotDouble(neitherDoubleNorHeapBigIntEdge);
-        speculateNotDouble(notDoubleEdge);
+        speculateNotDouble(leftNeitherDoubleNorHeapBigIntEdge);
+        speculateNotDouble(rightNotDoubleEdge);
         ValueFromBlock fastFalse = m_out.anchor(m_out.booleanFalse);
         m_out.branch(isNotCell(leftValue, leftValueType & ~SpecFullDouble), unsure(continuation), unsure(leftIsCell));
 
         m_out.appendTo(leftIsCell, leftIsString);
-        FTL_TYPE_CHECK(jsValueValue(leftValue), neitherDoubleNorHeapBigIntEdge, ~SpecHeapBigInt, isHeapBigInt(leftValue));
+        FTL_TYPE_CHECK(jsValueValue(leftValue), leftNeitherDoubleNorHeapBigIntEdge, ~SpecHeapBigInt, isHeapBigInt(leftValue));
         m_out.branch(isNotString(leftValue, leftValueType & SpecCell & ~SpecHeapBigInt), unsure(continuation), unsure(leftIsString));
 
         m_out.appendTo(leftIsString, rightIsCell);
@@ -9956,7 +9956,7 @@
         m_out.branch(isNotString(rightValue, rightValueType & SpecCell & ~SpecFullDouble), unsure(continuation), unsure(rightIsString));
 
         m_out.appendTo(rightIsString, continuation);
-        ValueFromBlock slowResult = m_out.anchor(stringsEqual(leftValue, rightValue, neitherDoubleNorHeapBigIntEdge, notDoubleEdge));
+        ValueFromBlock slowResult = m_out.anchor(stringsEqual(leftValue, rightValue, leftNeitherDoubleNorHeapBigIntEdge, rightNotDoubleEdge));
         m_out.jump(continuation);
 
         m_out.appendTo(continuation, lastNext);

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to