Skip to content

Commit 5a98f15

Browse files
author
[email protected]
committed
Math.max() can yield the wrong result for max(0, -0).
https://bugs.webkit.org/show_bug.cgi?id=204457 JSTests: Patch by Xan Lopez <[email protected]> on 2020-07-17 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): Source/JavaScriptCore: Patch by Xan López <[email protected]> on 2020-07-17 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): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@264507 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent e2b0c59 commit 5a98f15

10 files changed

+236
-13
lines changed

‎JSTests/ChangeLog

+14
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2020-07-17 Xan Lopez <[email protected]>
2+
3+
Math.max() can yield the wrong result for max(0, -0).
4+
https://bugs.webkit.org/show_bug.cgi?id=204457
5+
6+
Reviewed by Mark Lam.
7+
8+
Add a test to make sure we follow the spec in Math.{max,min} and
9+
-0.0 < 0.0.
10+
11+
* stress/math-max-min-negative-zero.js: Added.
12+
(assert):
13+
(test):
14+
115
2020-07-17 Alexey Shvayka <[email protected]>
216

317
emitIsUndefined() should not special-case [[IsHTMLDDA]] objects

‎JSTests/stress/math-max-min-negative-zero.js

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
function testMax(x, y) {
2+
return Math.max(x, y);
3+
}
4+
5+
function testMin(x, y) {
6+
return Math.min(x, y);
7+
}
8+
9+
for (var i = 0; i < 1000; ++i) {
10+
x = 100/testMax(0.0, -0.0);
11+
if (x < 0)
12+
throw "FAILED";
13+
x = 100/testMax(-0.0, 0.0);
14+
if (x < 0)
15+
throw "FAILED";
16+
17+
x = 100/testMin(0.0, -0.0);
18+
if (x > 0)
19+
throw "FAILED";
20+
x = 100/testMin(-0.0, 0.0);
21+
if (x > 0)
22+
throw "FAILED";
23+
}

‎Source/JavaScriptCore/ChangeLog

+35
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,38 @@
1+
2020-07-17 Xan López <[email protected]>
2+
3+
Math.max() can yield the wrong result for max(0, -0).
4+
https://bugs.webkit.org/show_bug.cgi?id=204457
5+
6+
Reviewed by Mark Lam.
7+
8+
The implementations for Math.{max,min} in both DFG and FTL are not
9+
considering the fact that according to the spec -0.0 < 0.0 (which
10+
is not true for normal double arithmetic).
11+
See: https://tc39.es/ecma262/#sec-math.max and https://tc39.es/ecma262/#sec-math.min
12+
13+
Beyond tweaking the algorithms used in DFG and FTL we must
14+
implement the and/or operations on double in MIPS and ARMv7, since
15+
these are used in the DFG JIT to distinguish between -0.0 and 0.0.
16+
17+
* assembler/ARMv7Assembler.h:
18+
(JSC::ARMv7Assembler::vand):
19+
(JSC::ARMv7Assembler::vorr):
20+
* assembler/MacroAssemblerARMv7.h:
21+
(JSC::MacroAssemblerARMv7::andDouble):
22+
(JSC::MacroAssemblerARMv7::orDouble):
23+
* assembler/MacroAssemblerMIPS.h:
24+
(JSC::MacroAssemblerMIPS::andDouble):
25+
(JSC::MacroAssemblerMIPS::orDouble):
26+
* assembler/testmasm.cpp:
27+
(JSC::testAndOrDouble):
28+
(JSC::run):
29+
* dfg/DFGAbstractInterpreterInlines.h: consider that -0.0 < 0.0 per the ECMAScript spec.
30+
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
31+
* dfg/DFGSpeculativeJIT.cpp: ditto.
32+
(JSC::DFG::SpeculativeJIT::compileArithMinMax):
33+
* ftl/FTLLowerDFGToB3.cpp: ditto.
34+
(JSC::FTL::DFG::LowerDFGToB3::compileArithMinOrMax):
35+
136
2020-07-17 Alexey Shvayka <[email protected]>
237

338
emitIsUndefined() should not special-case [[IsHTMLDDA]] objects

‎Source/JavaScriptCore/assembler/ARMv7Assembler.h

+14
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,8 @@ class ARMv7Assembler {
595595
OP_VSQRT_T1 = 0xEEB0,
596596
OP_VCVTSD_T1 = 0xEEB0,
597597
OP_VCVTDS_T1 = 0xEEB0,
598+
OP_VAND_T1 = 0xEF00,
599+
OP_VORR_T1 = 0xEF20,
598600
OP_B_T3a = 0xF000,
599601
OP_B_T4a = 0xF000,
600602
OP_AND_imm_T1 = 0xF000,
@@ -653,6 +655,8 @@ class ARMv7Assembler {
653655
} OpcodeID1;
654656

655657
typedef enum {
658+
OP_VAND_T1b = 0x0010,
659+
OP_VORR_T1b = 0x0010,
656660
OP_VADD_T2b = 0x0A00,
657661
OP_VDIVb = 0x0A00,
658662
OP_FLDSb = 0x0A00,
@@ -1823,6 +1827,16 @@ class ARMv7Assembler {
18231827
}
18241828
#endif
18251829

1830+
void vand(FPDoubleRegisterID rd, FPDoubleRegisterID rn, FPDoubleRegisterID rm)
1831+
{
1832+
m_formatter.vfpOp(OP_VAND_T1, OP_VAND_T1b, true, rn, rd, rm);
1833+
}
1834+
1835+
void vorr(FPDoubleRegisterID rd, FPDoubleRegisterID rn, FPDoubleRegisterID rm)
1836+
{
1837+
m_formatter.vfpOp(OP_VORR_T1, OP_VORR_T1b, true, rn, rd, rm);
1838+
}
1839+
18261840
void vadd(FPDoubleRegisterID rd, FPDoubleRegisterID rn, FPDoubleRegisterID rm)
18271841
{
18281842
m_formatter.vfpOp(OP_VADD_T2, OP_VADD_T2b, true, rn, rd, rm);

‎Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

+10
Original file line numberDiff line numberDiff line change
@@ -1162,6 +1162,16 @@ class MacroAssemblerARMv7 : public AbstractMacroAssembler<Assembler> {
11621162
m_assembler.vmul(dest, op1, op2);
11631163
}
11641164

1165+
void andDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
1166+
{
1167+
m_assembler.vand(op1, op2, dest);
1168+
}
1169+
1170+
void orDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
1171+
{
1172+
m_assembler.vorr(op1, op2, dest);
1173+
}
1174+
11651175
void sqrtDouble(FPRegisterID src, FPRegisterID dest)
11661176
{
11671177
m_assembler.vsqrt(dest, src);

‎Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h

+48
Original file line numberDiff line numberDiff line change
@@ -3080,6 +3080,54 @@ class MacroAssemblerMIPS : public AbstractMacroAssembler<Assembler> {
30803080
m_assembler.addd(dest, dest, fpTempRegister);
30813081
}
30823082

3083+
// andDouble and orDouble are a bit convoluted to implement
3084+
// because we don't have FP instructions for those
3085+
// operations. That means we'll have to go back and forth between
3086+
// the FPU and the CPU, which accounts for most of the code here.
3087+
void andDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
3088+
{
3089+
m_assembler.mfc1(immTempRegister, op1);
3090+
m_assembler.mfc1(dataTempRegister, op2);
3091+
m_assembler.andInsn(cmpTempRegister, immTempRegister, dataTempRegister);
3092+
m_assembler.mtc1(cmpTempRegister, dest);
3093+
3094+
#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
3095+
m_assembler.mfhc1(immTempRegister, op1);
3096+
m_assembler.mfhc1(dataTempRegister, op2);
3097+
#else
3098+
m_assembler.mfc1(immTempRegister, FPRegisterID(op1+1));
3099+
m_assembler.mfc1(dataTempRegister, FPRegisterID(op2+1));
3100+
#endif
3101+
m_assembler.andInsn(cmpTempRegister, immTempRegister, dataTempRegister);
3102+
#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
3103+
m_assembler.mthc1(cmpTempRegister, dest);
3104+
#else
3105+
m_assembler.mtc1(cmpTempRegister, FPRegisterID(dest+1));
3106+
#endif
3107+
}
3108+
3109+
void orDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)
3110+
{
3111+
m_assembler.mfc1(immTempRegister, op1);
3112+
m_assembler.mfc1(dataTempRegister, op2);
3113+
m_assembler.orInsn(cmpTempRegister, immTempRegister, dataTempRegister);
3114+
m_assembler.mtc1(cmpTempRegister, dest);
3115+
3116+
#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
3117+
m_assembler.mfhc1(immTempRegister, op1);
3118+
m_assembler.mfhc1(dataTempRegister, op2);
3119+
#else
3120+
m_assembler.mfc1(immTempRegister, FPRegisterID(op1+1));
3121+
m_assembler.mfc1(dataTempRegister, FPRegisterID(op2+1));
3122+
#endif
3123+
m_assembler.orInsn(cmpTempRegister, immTempRegister, dataTempRegister);
3124+
#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
3125+
m_assembler.mthc1(cmpTempRegister, dest);
3126+
#else
3127+
m_assembler.mtc1(cmpTempRegister, FPRegisterID(dest+1));
3128+
#endif
3129+
}
3130+
30833131
void subDouble(FPRegisterID src, FPRegisterID dest)
30843132
{
30853133
m_assembler.subd(dest, dest, src);

‎Source/JavaScriptCore/assembler/testmasm.cpp

+48
Original file line numberDiff line numberDiff line change
@@ -2291,6 +2291,52 @@ void testOrImmMem()
22912291
CHECK_EQ(memoryLocation, 0x12341234);
22922292
}
22932293

2294+
void testAndOrDouble()
2295+
{
2296+
double arg1, arg2;
2297+
2298+
auto andDouble = compile([&] (CCallHelpers& jit) {
2299+
emitFunctionPrologue(jit);
2300+
jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg1), FPRInfo::fpRegT1);
2301+
jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg2), FPRInfo::fpRegT2);
2302+
2303+
jit.andDouble(FPRInfo::fpRegT1, FPRInfo::fpRegT2, FPRInfo::returnValueFPR);
2304+
2305+
emitFunctionEpilogue(jit);
2306+
jit.ret();
2307+
});
2308+
2309+
auto operands = doubleOperands();
2310+
for (auto a : operands) {
2311+
for (auto b : operands) {
2312+
arg1 = a;
2313+
arg2 = b;
2314+
uint64_t expectedResult = bitwise_cast<uint64_t>(arg1) & bitwise_cast<uint64_t>(arg2);
2315+
CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(andDouble)), expectedResult);
2316+
}
2317+
}
2318+
2319+
auto orDouble = compile([&] (CCallHelpers& jit) {
2320+
emitFunctionPrologue(jit);
2321+
jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg1), FPRInfo::fpRegT1);
2322+
jit.loadDouble(CCallHelpers::TrustedImmPtr(&arg2), FPRInfo::fpRegT2);
2323+
2324+
jit.orDouble(FPRInfo::fpRegT1, FPRInfo::fpRegT2, FPRInfo::returnValueFPR);
2325+
2326+
emitFunctionEpilogue(jit);
2327+
jit.ret();
2328+
});
2329+
2330+
for (auto a : operands) {
2331+
for (auto b : operands) {
2332+
arg1 = a;
2333+
arg2 = b;
2334+
uint64_t expectedResult = bitwise_cast<uint64_t>(arg1) | bitwise_cast<uint64_t>(arg2);
2335+
CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(orDouble)), expectedResult);
2336+
}
2337+
}
2338+
}
2339+
22942340
void testByteSwap()
22952341
{
22962342
#if CPU(X86_64) || CPU(ARM64)
@@ -2652,6 +2698,8 @@ void run(const char* filter)
26522698

26532699
RUN(testOrImmMem());
26542700

2701+
RUN(testAndOrDouble());
2702+
26552703
if (tasks.isEmpty())
26562704
usage();
26572705

‎Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h

+6-2
Original file line numberDiff line numberDiff line change
@@ -1181,7 +1181,9 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
11811181
if (left && right && left.isNumber() && right.isNumber()) {
11821182
double a = left.asNumber();
11831183
double b = right.asNumber();
1184-
setConstant(node, jsDoubleNumber(a < b ? a : (b <= a ? b : a + b)));
1184+
// The spec for Math.min states that +0 is considered to be larger than -0.
1185+
double result = a < b || (!a && !b && std::signbit(a)) ? a : (b <= a ? b : a + b);
1186+
setConstant(node, jsDoubleNumber(result));
11851187
break;
11861188
}
11871189
setNonCellTypeForNode(node,
@@ -1210,7 +1212,9 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
12101212
if (left && right && left.isNumber() && right.isNumber()) {
12111213
double a = left.asNumber();
12121214
double b = right.asNumber();
1213-
setConstant(node, jsDoubleNumber(a > b ? a : (b >= a ? b : a + b)));
1215+
// The spec for Math.max states that +0 is considered to be larger than -0.
1216+
double result = a > b || (!a && !b && !std::signbit(a)) ? a : (b >= a ? b : a + b);
1217+
setConstant(node, jsDoubleNumber(result));
12141218
break;
12151219
}
12161220
setNonCellTypeForNode(node,

‎Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

+12-2
Original file line numberDiff line numberDiff line change
@@ -6181,9 +6181,19 @@ void SpeculativeJIT::compileArithMinMax(Node* node)
61816181
MacroAssembler::JumpList done;
61826182

61836183
MacroAssembler::Jump op1Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleLessThanAndOrdered : MacroAssembler::DoubleGreaterThanAndOrdered, op1FPR, op2FPR);
6184+
MacroAssembler::Jump opNotEqual = m_jit.branchDouble(MacroAssembler::DoubleNotEqualAndOrdered, op1FPR, op2FPR);
61846185

6185-
// op2 is eather the lesser one or one of then is NaN
6186-
MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleGreaterThanOrEqualAndOrdered : MacroAssembler::DoubleLessThanOrEqualAndOrdered, op1FPR, op2FPR);
6186+
// The spec for Math.min and Math.max states that +0 is considered to be larger than -0.
6187+
if (node->op() == ArithMin)
6188+
m_jit.orDouble(op1FPR, op2FPR, resultFPR);
6189+
else
6190+
m_jit.andDouble(op1FPR, op2FPR, resultFPR);
6191+
6192+
done.append(m_jit.jump());
6193+
6194+
opNotEqual.link(&m_jit);
6195+
// op2 is either the lesser one or one of then is NaN
6196+
MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleGreaterThanAndOrdered : MacroAssembler::DoubleLessThanAndOrdered, op1FPR, op2FPR);
61876197

61886198
// Unordered case. We don't know which of op1, op2 is NaN. Manufacture NaN by adding
61896199
// op1 + op2 and putting it into result.

‎Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

+26-9
Original file line numberDiff line numberDiff line change
@@ -2870,25 +2870,42 @@ class LowerDFGToB3 {
28702870
case DoubleRepUse: {
28712871
LValue left = lowDouble(m_node->child1());
28722872
LValue right = lowDouble(m_node->child2());
2873-
2873+
28742874
LBasicBlock notLessThan = m_out.newBlock();
2875+
LBasicBlock isEqual = m_out.newBlock();
2876+
LBasicBlock notEqual = m_out.newBlock();
28752877
LBasicBlock continuation = m_out.newBlock();
2876-
2878+
28772879
Vector<ValueFromBlock, 2> results;
2878-
2880+
28792881
results.append(m_out.anchor(left));
28802882
m_out.branch(
28812883
m_node->op() == ArithMin
28822884
? m_out.doubleLessThan(left, right)
28832885
: m_out.doubleGreaterThan(left, right),
28842886
unsure(continuation), unsure(notLessThan));
2885-
2886-
LBasicBlock lastNext = m_out.appendTo(notLessThan, continuation);
2887-
results.append(m_out.anchor(m_out.select(
2887+
2888+
// The spec for Math.min and Math.max states that +0 is considered to be larger than -0.
2889+
LBasicBlock lastNext = m_out.appendTo(notLessThan, isEqual);
2890+
m_out.branch(
2891+
m_out.doubleEqual(left, right),
2892+
rarely(isEqual), usually(notEqual));
2893+
2894+
lastNext = m_out.appendTo(isEqual, notEqual);
2895+
results.append(m_out.anchor(
28882896
m_node->op() == ArithMin
2889-
? m_out.doubleGreaterThanOrEqual(left, right)
2890-
: m_out.doubleLessThanOrEqual(left, right),
2891-
right, m_out.constDouble(PNaN))));
2897+
? m_out.bitOr(left, right)
2898+
: m_out.bitAnd(left, right)));
2899+
m_out.jump(continuation);
2900+
2901+
lastNext = m_out.appendTo(notEqual, continuation);
2902+
results.append(
2903+
m_out.anchor(
2904+
m_out.select(
2905+
m_node->op() == ArithMin
2906+
? m_out.doubleGreaterThan(left, right)
2907+
: m_out.doubleLessThan(left, right),
2908+
right, m_out.constDouble(PNaN))));
28922909
m_out.jump(continuation);
28932910

28942911
m_out.appendTo(continuation, lastNext);

0 commit comments

Comments
 (0)