Bug 204457 – Math.max() can yield the wrong result for max(0, -0).

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Spec says 0 is greater than -0.  See https://www.ecma-international.org/ecma-262/6.0/#sec-math.max.

POC:
function foo(x, y) {
    return Math.max(x, y);
}
for (var i = 0; i < 30000; ++i) {
    x = 100/foo(0.0, -0.0);
    if (x < 0)
        throw "FAILED";
}

Expected: should run to completion silently.
Actual: throws "FAILED".

[Mark Lam]

This issue was brought to my attention by Xan Lopex in https://bugs.webkit.org/show_bug.cgi?id=203406#c8.

[Mark Lam]

I've reproduced the issue on ARM64 as well.

[Saam Barati]

If you look at how we do this in Wasm, I think we can do the same here for JS.

[Xan Lopez]

FWIW, a couple comments about this:

I believe the DFG AbstractInterpreter has the same bug. Something simple like this should work:

diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
index e5284ad85d1..da8106c2a17 100644
--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
@@ -1108,7 +1108,8 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
             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)));
+                double result = a < b || (!a && !b && std::signbit(a)) ? a : (b <= a ? b : a + b);
+                setConstant(node, jsDoubleNumber(result));
                 break;
             }
             setNonCellTypeForNode(node,
@@ -1137,7 +1138,8 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
             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)));
+                double result = a > b || (!a && !b && !std::signbit(a)) ? a : (b >= a ? b : a + b);
+                setConstant(node, jsDoubleNumber(result));
                 break;
             }
             setNonCellTypeForNode(node,

Also, looking at the FTL code I think it also has the same bug again (see compileArithMinOrMax in FTLLowerDFGToB3.cpp).

Should we open one bug for each? Also since the code in the DFG AI only runs in a couple of passes (CFA and constant folding) I guess it should have its own test, if you have any preferred way of testing that feel free to comment. I understood you wanted to fix the DFG JIT but if not I'm happy to work on all these in whatever way you want.

[Xan Lopez]

Created attachment 390207 [details]
Consider negative zero in DFG JIT Math.{max,min}

This just fixes the bug in DFG JIT, as mentioned earlier I believe there same bug is present at least in the DFG AI.

[Xan Lopez]

Comment on attachment 390207 [details]
Consider negative zero in DFG JIT Math.{max,min}

This test is already showing the bug in FTL I mentioned earlier (with the eager run), and a fail in the DFG with --validateGraph. I'll have a look at both.

[Xan Lopez]

Created attachment 390911 [details]
Fix Math.{max,min} in DFG and FTL

Consider the negative zero special case in Math.{max,min} in DFG and FTL, plus a new test for it.

[Xan Lopez]

Created attachment 391038 [details]
Fix Math.{max,min} for negative zero in DFG and FTL

Fix some style nits. My local script still gives some warnings but AFAICT it's in existing code and they don't make much sense to me.

[Yusuke Suzuki]

Will look.

[Xan Lopez]

(In reply to Yusuke Suzuki from comment #9)
> Will look.

Ping about this?

[Yusuke Suzuki]

Looks like ARMv7 is failing. Can you fix it?

[Yusuke Suzuki]

Comment on attachment 391038 [details]
Fix Math.{max,min} for negative zero in DFG and FTL

View in context: https://bugs.webkit.org/attachment.cgi?id=391038&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5865
> +            m_jit.andDouble(op1FPR, op2FPR, resultFPR);

andDouble/orDouble is defined only in x86 and ARM64. You need to define it for MIPS and ARMv7.

[Xan Lopez]

Comment on attachment 391038 [details]
Fix Math.{max,min} for negative zero in DFG and FTL

Thanks! Will have a look at the ARMv7/MIPS issue.

[Xan Lopez]

Created attachment 401446 [details]
Fix Math.{max,min} to consider that -0.0 < 0,0

With andDouble/orDouble implementations for MIPS and ARMv7.

[Paulo Matos]

I have taken a look at the patch and it looks good to me.
Patch applies to ToT, builds for ARMv7 and MIPS and tests pass.

However, style is currently failing. You probably should address this.

[Caio Lima]

Comment on attachment 401446 [details]
Fix Math.{max,min} to consider that -0.0 < 0,0

View in context: https://bugs.webkit.org/attachment.cgi?id=401446&action=review

Informal review. LGTM overall. I left some comments regarding documentation.

> Source/JavaScriptCore/ChangeLog:10
> +        is not true for normal double arithmetic).

Do you mind placing a link to spec text about it? It is mentioned here https://tc39.es/ecma262/#sec-numeric-types-number-lessThan

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1161
> +    }

I don't remember if MacroAssembler tests are running on ARMv7 and MIPS, but it would be nice to add those in this Patch before we forget it.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:3079
> +    void andDouble(FPRegisterID op1, FPRegisterID op2, FPRegisterID dest)

This seems very straight forward into ARMv7, but more complex into MIPS. I think it would be nice have a comment about the following code and why it is necessary (IIRC, the reason is because MIPS doesn't have a double "and" or a double "or" instruction, right?)

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:3102
> +    {

Ditto

[Xan Lopez]

Created attachment 402636 [details]
Math.{max,min} fixes, v2

Added requested comments and links to the spec, testmasm tests. Gave another shot at the style issue and still seems to me the code (both new and existing) is following the guide, so we should probably either tweak the script or change the whole file accordingly.

[Xan Lopez]

Created attachment 403678 [details]
Math.{max,min} fixes, v3

Rebased against master.

[Mark Lam]

Comment on attachment 403678 [details]
Math.{max,min} fixes, v3

View in context: https://bugs.webkit.org/attachment.cgi?id=403678&action=review

Some comments for now while I think thru the DFG and FTL changes.

> JSTests/stress/math-max-min-negative-zero.js:12
> +    x = 100/testMax(0.0, -0.0);
> +    if (x < 0)
> +        throw "FAILED";

Please also add a case for `x = 100/testMax(-0.0, 0.0);`

> JSTests/stress/math-max-min-negative-zero.js:15
> +    x = 100/testMin(0.0, -0.0);
> +    if (x > 0)
> +        throw "FAILED";

Please also add a case for `x = 100/testMin(-0.0, 0.0);`

> Source/JavaScriptCore/ChangeLog:10
> +        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).

I don't think this is what the spec says.  According to https://tc39.es/ecma262/#sec-numeric-types-number-lessThan,

for Number::lessThan ( x, y ),

4. If x is +0 and y is -0, return false.
5. If x is -0 and y is +0, return false.

This means +0 is not less than -0, and -0 is not less than +0.

I think the relevant spec is https://tc39.es/ecma262/#sec-math.max and https://tc39.es/ecma262/#sec-math.min, where it states "The comparison of values to determine the largest value is done using the Abstract Relational Comparison algorithm except that +0 is considered to be larger than -0."

Oh, maybe you're quoting https://tc39.es/ecma262/#sec-numeric-types-number-lessThan to show that it considers -0 and +0 equal.  Please add the urls to /#sec-math.max and #sec-math.min as well.  I think those 2 are the mode relevant specs.

> Source/JavaScriptCore/assembler/testmasm.cpp:2319
> +    arg1 = 0.0;
> +    arg2 = 0.0;
> +    CHECK_EQ(invoke<double>(andDouble), 0.0);
> +
> +    arg1 = 0.0;
> +    arg2 = -0.0;
> +    CHECK_EQ(invoke<double>(andDouble), 0.0);
> +
> +    arg1 = -0.0;
> +    arg2 = -0.0;
> +    CHECK_EQ(invoke<double>(andDouble), -0.0);

Instead of just doing this, please iterate over doubleOperands() for arg1 and arg2 values.  The expected result should be bitwise_cast<double>(bitwise_cast<uint64_t>(arg1) & bitwise_cast<uint64_t>(arg2)).

> Source/JavaScriptCore/assembler/testmasm.cpp:2342
> +    arg1 = 0.0;
> +    arg2 = -0.0;
> +    CHECK_EQ(invoke<double>(orDouble), -0.0);
> +
> +    arg1 = -0.0;
> +    arg2 = -0.0;
> +    CHECK_EQ(invoke<double>(orDouble), -0.0);
> +
> +    arg1 = 0.0;
> +    arg2 = 0.0;
> +    CHECK_EQ(invoke<double>(orDouble), 0.0);

Ditto: please iterate over doubleOperands() for arg1 and arg2 values.  The expected result should be bitwise_cast<double>(bitwise_cast<uint64_t>(arg1) | bitwise_cast<uint64_t>(arg2)).

[Mark Lam]

Comment on attachment 403678 [details]
Math.{max,min} fixes, v3

View in context: https://bugs.webkit.org/attachment.cgi?id=403678&action=review

r- for now because I think this patch can use a bit more work.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6188
> +        MacroAssembler::Jump opEqual = m_jit.branchDouble(MacroAssembler::DoubleEqualAndOrdered, op1FPR, op2FPR);
> +
>          MacroAssembler::Jump op1Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleLessThanAndOrdered : MacroAssembler::DoubleGreaterThanAndOrdered, 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);
> +        // 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);

Can we implement this code as follows instead?

    MacroAssembler::Jump op1Less = m_jit.branchDouble(node->op() == ArithMin ? MacroAssembler::DoubleLessThanAndOrdered : MacroAssembler::DoubleGreaterThanAndOrdered, op1FPR, op2FPR);
    MacroAssembler::Jump opNotEqual = m_jit.branchDouble(MacroAssembler::DoubleNotEqualOrUnordered, op1FPR, op2FPR);

    // If both doubles are equal there's a chance one of them is negative zero, so consider that case.
    if (node->op() == ArithMax)
        m_jit.andDouble(op1FPR, op2FPR, resultFPR);
    else
        m_jit.orDouble(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);

This way the a < b (for min) or a > b (for max) case still take the same number of instructions.
Only the >= (for min) or <= (for max) case incurs the extra == check.

If I were to guess, I would guess that a almost never == b.  So, optimistically checking for a < b or a > b first would be a win half of the time.

Please do the same for the FTL code below.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6214
> +        done.append(m_jit.jump());
> +
> +        // If both doubles are equal there's a chance one of them is negative zero, so consider that case.
> +        opEqual.link(&m_jit);
> +        if (node->op() == ArithMax)
> +            m_jit.andDouble(op1FPR, op2FPR, resultFPR);
> +        else
> +            m_jit.orDouble(op1FPR, op2FPR, resultFPR);
> +

Remove this.

[Xan Lopez]

Created attachment 404452 [details]
Math.{max,min} fixes, v4

This should address all your comments. I'm comparing the testmasm results in uint64_t form, though (instead of casting back to double), because otherwise a lot of the results are just NaN and we get bogus (I think) errors. I think this should be fine but a sanity check is welcome.

[Mark Lam]

Comment on attachment 404452 [details]
Math.{max,min} fixes, v4

View in context: https://bugs.webkit.org/attachment.cgi?id=404452&action=review

r=me with fixes and if EWS bits are green.

> Source/JavaScriptCore/assembler/testmasm.cpp:2315
> +            uint64_t result = bitwise_cast<uint64_t>(arg1) & bitwise_cast<uint64_t>(arg2);
> +            CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(andDouble)), result);

nit: can you rename "result" to "expectedResult"?

> Source/JavaScriptCore/assembler/testmasm.cpp:2335
> +            uint64_t result = bitwise_cast<uint64_t>(arg1) | bitwise_cast<uint64_t>(arg2);
> +            CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(orDouble)), result);

Ditto.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1184
> +                double result = a < b || (!a && !b && std::signbit(a)) ? a : (b <= a ? b : a + b);

Please add a comment here to say "The spec for min states that +0 is considered to be larger than -0."

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1214
> +                double result = a > b || (!a && !b && !std::signbit(a)) ? a : (b >= a ? b : a + b);

Please add a comment here to say "The spec for max states that +0 is considered to be larger than -0."

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6186
> +        // If both doubles are equal there's a chance one of them is negative zero, so consider that case.

I think you can replace this comment with "The spec for min and max states that +0 is considered to be larger than -0." which tells us everything we need to know about why we need to do this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6190
> +        if (node->op() == ArithMax)
> +            m_jit.andDouble(op1FPR, op2FPR, resultFPR);
> +        else
> +            m_jit.orDouble(op1FPR, op2FPR, resultFPR);

Please change this to check for ArithMin instead and flip the 2 cases.  This will make the code match the FTL case and the other branch above that checks for ArithMin also.  I think that makes it slightly easier to follow and review for correctness.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2889
> +            m_out.branch(

Please also add a comment above here to say "The spec for min and max states that +0 is considered to be larger than -0."

[Xan Lopez]

Created attachment 404550 [details]
Math.{max,min} fixes, v5

Another update.

[Xan Lopez]

*** Bug 203406 has been marked as a duplicate of this bug. ***

[EWS]

Committed r264507: <https://trac.webkit.org/changeset/264507>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404550 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65724890>