Diff
Modified: trunk/JSTests/ChangeLog (264506 => 264507)
--- trunk/JSTests/ChangeLog 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/JSTests/ChangeLog 2020-07-17 14:29:30 UTC (rev 264507)
@@ -1,3 +1,17 @@
+2020-07-17 Xan Lopez <[email protected]>
+
+ Math.max() can yield the wrong result for max(0, -0).
+ https://bugs.webkit.org/show_bug.cgi?id=204457
+
+ Reviewed by Mark Lam.
+
+ Add a test to make sure we follow the spec in Math.{max,min} and
+ -0.0 < 0.0.
+
+ * stress/math-max-min-negative-zero.js: Added.
+ (assert):
+ (test):
+
2020-07-17 Alexey Shvayka <[email protected]>
emitIsUndefined() should not special-case [[IsHTMLDDA]] objects
Added: trunk/JSTests/stress/math-max-min-negative-zero.js (0 => 264507)
--- trunk/JSTests/stress/math-max-min-negative-zero.js (rev 0)
+++ trunk/JSTests/stress/math-max-min-negative-zero.js 2020-07-17 14:29:30 UTC (rev 264507)
@@ -0,0 +1,23 @@
+function testMax(x, y) {
+ return Math.max(x, y);
+}
+
+function testMin(x, y) {
+ return Math.min(x, y);
+}
+
+for (var i = 0; i < 1000; ++i) {
+ x = 100/testMax(0.0, -0.0);
+ if (x < 0)
+ throw "FAILED";
+ x = 100/testMax(-0.0, 0.0);
+ if (x < 0)
+ throw "FAILED";
+
+ x = 100/testMin(0.0, -0.0);
+ if (x > 0)
+ throw "FAILED";
+ x = 100/testMin(-0.0, 0.0);
+ if (x > 0)
+ throw "FAILED";
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (264506 => 264507)
--- trunk/Source/_javascript_Core/ChangeLog 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-07-17 14:29:30 UTC (rev 264507)
@@ -1,3 +1,38 @@
+2020-07-17 Xan López <[email protected]>
+
+ Math.max() can yield the wrong result for max(0, -0).
+ https://bugs.webkit.org/show_bug.cgi?id=204457
+
+ Reviewed by Mark Lam.
+
+ The implementations for Math.{max,min} in both DFG and FTL are not
+ considering the fact that according to the spec -0.0 < 0.0 (which
+ is not true for normal double arithmetic).
+ See: https://tc39.es/ecma262/#sec-math.max and https://tc39.es/ecma262/#sec-math.min
+
+ Beyond tweaking the algorithms used in DFG and FTL we must
+ implement the and/or operations on double in MIPS and ARMv7, since
+ these are used in the DFG JIT to distinguish between -0.0 and 0.0.
+
+ * assembler/ARMv7Assembler.h:
+ (JSC::ARMv7Assembler::vand):
+ (JSC::ARMv7Assembler::vorr):
+ * assembler/MacroAssemblerARMv7.h:
+ (JSC::MacroAssemblerARMv7::andDouble):
+ (JSC::MacroAssemblerARMv7::orDouble):
+ * assembler/MacroAssemblerMIPS.h:
+ (JSC::MacroAssemblerMIPS::andDouble):
+ (JSC::MacroAssemblerMIPS::orDouble):
+ * assembler/testmasm.cpp:
+ (JSC::testAndOrDouble):
+ (JSC::run):
+ * dfg/DFGAbstractInterpreterInlines.h: consider that -0.0 < 0.0 per the ECMAScript spec.
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGSpeculativeJIT.cpp: ditto.
+ (JSC::DFG::SpeculativeJIT::compileArithMinMax):
+ * ftl/FTLLowerDFGToB3.cpp: ditto.
+ (JSC::FTL::DFG::LowerDFGToB3::compileArithMinOrMax):
+
2020-07-17 Alexey Shvayka <[email protected]>
emitIsUndefined() should not special-case [[IsHTMLDDA]] objects
Modified: trunk/Source/_javascript_Core/assembler/ARMv7Assembler.h (264506 => 264507)
--- trunk/Source/_javascript_Core/assembler/ARMv7Assembler.h 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/assembler/ARMv7Assembler.h 2020-07-17 14:29:30 UTC (rev 264507)
@@ -595,6 +595,8 @@
OP_VSQRT_T1 = 0xEEB0,
OP_VCVTSD_T1 = 0xEEB0,
OP_VCVTDS_T1 = 0xEEB0,
+ OP_VAND_T1 = 0xEF00,
+ OP_VORR_T1 = 0xEF20,
OP_B_T3a = 0xF000,
OP_B_T4a = 0xF000,
OP_AND_imm_T1 = 0xF000,
@@ -653,6 +655,8 @@
} OpcodeID1;
typedef enum {
+ OP_VAND_T1b = 0x0010,
+ OP_VORR_T1b = 0x0010,
OP_VADD_T2b = 0x0A00,
OP_VDIVb = 0x0A00,
OP_FLDSb = 0x0A00,
@@ -1823,6 +1827,16 @@
}
#endif
+ void vand(FPDoubleRegisterID rd, FPDoubleRegisterID rn, FPDoubleRegisterID rm)
+ {
+ m_formatter.vfpOp(OP_VAND_T1, OP_VAND_T1b, true, rn, rd, rm);
+ }
+
+ void vorr(FPDoubleRegisterID rd, FPDoubleRegisterID rn, FPDoubleRegisterID rm)
+ {
+ m_formatter.vfpOp(OP_VORR_T1, OP_VORR_T1b, true, rn, rd, rm);
+ }
+
void vadd(FPDoubleRegisterID rd, FPDoubleRegisterID rn, FPDoubleRegisterID rm)
{
m_formatter.vfpOp(OP_VADD_T2, OP_VADD_T2b, true, rn, rd, rm);
Modified: trunk/Source/_javascript_Core/assembler/MacroAssemblerARMv7.h (264506 => 264507)
--- trunk/Source/_javascript_Core/assembler/MacroAssemblerARMv7.h 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/assembler/MacroAssemblerARMv7.h 2020-07-17 14:29:30 UTC (rev 264507)
@@ -1162,6 +1162,16 @@
m_assembler.vmul(dest, op1, op2);
}
+ void andDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
+ {
+ m_assembler.vand(op1, op2, dest);
+ }
+
+ void orDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
+ {
+ m_assembler.vorr(op1, op2, dest);
+ }
+
void sqrtDouble(FPRegisterID src, FPRegisterID dest)
{
m_assembler.vsqrt(dest, src);
Modified: trunk/Source/_javascript_Core/assembler/MacroAssemblerMIPS.h (264506 => 264507)
--- trunk/Source/_javascript_Core/assembler/MacroAssemblerMIPS.h 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/assembler/MacroAssemblerMIPS.h 2020-07-17 14:29:30 UTC (rev 264507)
@@ -3080,6 +3080,54 @@
m_assembler.addd(dest, dest, fpTempRegister);
}
+ // andDouble and orDouble are a bit convoluted to implement
+ // because we don't have FP instructions for those
+ // operations. That means we'll have to go back and forth between
+ // the FPU and the CPU, which accounts for most of the code here.
+ void andDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
+ {
+ m_assembler.mfc1(immTempRegister, op1);
+ m_assembler.mfc1(dataTempRegister, op2);
+ m_assembler.andInsn(cmpTempRegister, immTempRegister, dataTempRegister);
+ m_assembler.mtc1(cmpTempRegister, dest);
+
+#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
+ m_assembler.mfhc1(immTempRegister, op1);
+ m_assembler.mfhc1(dataTempRegister, op2);
+#else
+ m_assembler.mfc1(immTempRegister, FPRegisterID(op1+1));
+ m_assembler.mfc1(dataTempRegister, FPRegisterID(op2+1));
+#endif
+ m_assembler.andInsn(cmpTempRegister, immTempRegister, dataTempRegister);
+#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
+ m_assembler.mthc1(cmpTempRegister, dest);
+#else
+ m_assembler.mtc1(cmpTempRegister, FPRegisterID(dest+1));
+#endif
+ }
+
+ void orDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
+ {
+ m_assembler.mfc1(immTempRegister, op1);
+ m_assembler.mfc1(dataTempRegister, op2);
+ m_assembler.orInsn(cmpTempRegister, immTempRegister, dataTempRegister);
+ m_assembler.mtc1(cmpTempRegister, dest);
+
+#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
+ m_assembler.mfhc1(immTempRegister, op1);
+ m_assembler.mfhc1(dataTempRegister, op2);
+#else
+ m_assembler.mfc1(immTempRegister, FPRegisterID(op1+1));
+ m_assembler.mfc1(dataTempRegister, FPRegisterID(op2+1));
+#endif
+ m_assembler.orInsn(cmpTempRegister, immTempRegister, dataTempRegister);
+#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
+ m_assembler.mthc1(cmpTempRegister, dest);
+#else
+ m_assembler.mtc1(cmpTempRegister, FPRegisterID(dest+1));
+#endif
+ }
+
void subDouble(FPRegisterID src, FPRegisterID dest)
{
m_assembler.subd(dest, dest, src);
Modified: trunk/Source/_javascript_Core/assembler/testmasm.cpp (264506 => 264507)
--- trunk/Source/_javascript_Core/assembler/testmasm.cpp 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/assembler/testmasm.cpp 2020-07-17 14:29:30 UTC (rev 264507)
@@ -2291,6 +2291,52 @@
CHECK_EQ(memoryLocation, 0x12341234);
}
+void testAndOrDouble()
+{
+ double arg1, arg2;
+
+ auto andDouble = compile([&] (CCallHelpers& jit) {
+ emitFunctionPrologue(jit);
+ jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg1), FPRInfo::fpRegT1);
+ jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg2), FPRInfo::fpRegT2);
+
+ jit.andDouble(FPRInfo::fpRegT1, FPRInfo::fpRegT2, FPRInfo::returnValueFPR);
+
+ emitFunctionEpilogue(jit);
+ jit.ret();
+ });
+
+ auto operands = doubleOperands();
+ for (auto a : operands) {
+ for (auto b : operands) {
+ arg1 = a;
+ arg2 = b;
+ uint64_t expectedResult = bitwise_cast<uint64_t>(arg1) & bitwise_cast<uint64_t>(arg2);
+ CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(andDouble)), expectedResult);
+ }
+ }
+
+ auto orDouble = compile([&] (CCallHelpers& jit) {
+ emitFunctionPrologue(jit);
+ jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg1), FPRInfo::fpRegT1);
+ jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg2), FPRInfo::fpRegT2);
+
+ jit.orDouble(FPRInfo::fpRegT1, FPRInfo::fpRegT2, FPRInfo::returnValueFPR);
+
+ emitFunctionEpilogue(jit);
+ jit.ret();
+ });
+
+ for (auto a : operands) {
+ for (auto b : operands) {
+ arg1 = a;
+ arg2 = b;
+ uint64_t expectedResult = bitwise_cast<uint64_t>(arg1) | bitwise_cast<uint64_t>(arg2);
+ CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(orDouble)), expectedResult);
+ }
+ }
+}
+
void testByteSwap()
{
#if CPU(X86_64) || CPU(ARM64)
@@ -2652,6 +2698,8 @@
RUN(testOrImmMem());
+ RUN(testAndOrDouble());
+
if (tasks.isEmpty())
usage();
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (264506 => 264507)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2020-07-17 14:29:30 UTC (rev 264507)
@@ -1181,7 +1181,9 @@
if (left && right && left.isNumber() && right.isNumber()) {
double a = left.asNumber();
double b = right.asNumber();
- setConstant(node, jsDoubleNumber(a < b ? a : (b <= a ? b : a + b)));
+ // The spec for Math.min states that +0 is considered to be larger than -0.
+ double result = a < b || (!a && !b && std::signbit(a)) ? a : (b <= a ? b : a + b);
+ setConstant(node, jsDoubleNumber(result));
break;
}
setNonCellTypeForNode(node,
@@ -1210,7 +1212,9 @@
if (left && right && left.isNumber() && right.isNumber()) {
double a = left.asNumber();
double b = right.asNumber();
- setConstant(node, jsDoubleNumber(a > b ? a : (b >= a ? b : a + b)));
+ // The spec for Math.max states that +0 is considered to be larger than -0.
+ double result = a > b || (!a && !b && !std::signbit(a)) ? a : (b >= a ? b : a + b);
+ setConstant(node, jsDoubleNumber(result));
break;
}
setNonCellTypeForNode(node,
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (264506 => 264507)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2020-07-17 14:29:30 UTC (rev 264507)
@@ -6181,10 +6181,20 @@
MacroAssembler::JumpList done;
MacroAssembler::Jump op1Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleLessThanAndOrdered : MacroAssembler::DoubleGreaterThanAndOrdered, op1FPR, op2FPR);
+ MacroAssembler::Jump opNotEqual = m_jit.branchDouble(MacroAssembler::DoubleNotEqualAndOrdered, op1FPR, op2FPR);
- // op2 is eather the lesser one or one of then is NaN
- MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleGreaterThanOrEqualAndOrdered : MacroAssembler::DoubleLessThanOrEqualAndOrdered, op1FPR, op2FPR);
+ // The spec for Math.min and Math.max states that +0 is considered to be larger than -0.
+ if (node->op() == ArithMin)
+ m_jit.orDouble(op1FPR, op2FPR, resultFPR);
+ else
+ m_jit.andDouble(op1FPR, op2FPR, resultFPR);
+ done.append(m_jit.jump());
+
+ opNotEqual.link(&m_jit);
+ // op2 is either the lesser one or one of then is NaN
+ MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleGreaterThanAndOrdered : MacroAssembler::DoubleLessThanAndOrdered, op1FPR, op2FPR);
+
// Unordered case. We don't know which of op1, op2 is NaN. Manufacture NaN by adding
// op1 + op2 and putting it into result.
m_jit.addDouble(op1FPR, op2FPR, resultFPR);
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (264506 => 264507)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2020-07-17 14:09:02 UTC (rev 264506)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2020-07-17 14:29:30 UTC (rev 264507)
@@ -2870,12 +2870,14 @@
case DoubleRepUse: {
LValue left = lowDouble(m_node->child1());
LValue right = lowDouble(m_node->child2());
-
+
LBasicBlock notLessThan = m_out.newBlock();
+ LBasicBlock isEqual = m_out.newBlock();
+ LBasicBlock notEqual = m_out.newBlock();
LBasicBlock continuation = m_out.newBlock();
-
+
Vector<ValueFromBlock, 2> results;
-
+
results.append(m_out.anchor(left));
m_out.branch(
m_node->op() == ArithMin
@@ -2882,14 +2884,29 @@
? m_out.doubleLessThan(left, right)
: m_out.doubleGreaterThan(left, right),
unsure(continuation), unsure(notLessThan));
-
- LBasicBlock lastNext = m_out.appendTo(notLessThan, continuation);
- results.append(m_out.anchor(m_out.select(
+
+ // The spec for Math.min and Math.max states that +0 is considered to be larger than -0.
+ LBasicBlock lastNext = m_out.appendTo(notLessThan, isEqual);
+ m_out.branch(
+ m_out.doubleEqual(left, right),
+ rarely(isEqual), usually(notEqual));
+
+ lastNext = m_out.appendTo(isEqual, notEqual);
+ results.append(m_out.anchor(
m_node->op() == ArithMin
- ? m_out.doubleGreaterThanOrEqual(left, right)
- : m_out.doubleLessThanOrEqual(left, right),
- right, m_out.constDouble(PNaN))));
+ ? m_out.bitOr(left, right)
+ : m_out.bitAnd(left, right)));
m_out.jump(continuation);
+
+ lastNext = m_out.appendTo(notEqual, continuation);
+ results.append(
+ m_out.anchor(
+ m_out.select(
+ m_node->op() == ArithMin
+ ? m_out.doubleGreaterThan(left, right)
+ : m_out.doubleLessThan(left, right),
+ right, m_out.constDouble(PNaN))));
+ m_out.jump(continuation);
m_out.appendTo(continuation, lastNext);
setDouble(m_out.phi(Double, results));
