Closed Bug 995826 Opened 10 years ago Closed 10 years ago

Differential Testing: Different output message involving Math.round and Math.tan

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- affected
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected

People

(Reporter: gkw, Assigned: h4writer)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

function f(x) {
    return Math.round(-Math.tan(x > 0))
}
f(2)
print(uneval(f(-1)))

$ ./js-opt-32-dm-ts-darwin-ebdf2740dc3e --fuzzing-safe --ion-parallel-compile=off 1906.js
-0

$ ./js-opt-32-dm-ts-darwin-ebdf2740dc3e --fuzzing-safe --ion-parallel-compile=off --ion-eager 1906.js
0

(Tested this on 32-bit Mac js opt threadsafe deterministic shell off m-c rev ebdf2740dc3e, and I think it also reproduces on Linux)

My configure flags (Mac) are:

LD=ld CROSS_COMPILE=1 CC="clang -msse2 -mfpmath=sse -Qunused-arguments -arch i386" RANLIB=ranlib CXX="clang++ -msse2 -mfpmath=sse -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -msse2 -mfpmath=sse -Qunused-arguments" HOST_CXX="clang++ -msse2 -mfpmath=sse -Qunused-arguments" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

This seems to go back further than http://hg.mozilla.org/mozilla-central/rev/541248fb29e4 .

Hannes, is this related to some other Math bugs you're looking at (e.g. bug 995675, though unlikely)?
Flags: needinfo?(hv1989)
diff --git a/js/src/jit/x86/MacroAssembler-x86.cpp b/js/src/jit/x86/MacroAssembler-x86.cpp
--- a/js/src/jit/x86/MacroAssembler-x86.cpp
+++ b/js/src/jit/x86/MacroAssembler-x86.cpp
@@ -391,26 +391,33 @@ MacroAssemblerX86::testNegativeZero(cons
 {
     // Determines whether the single double contained in the XMM register reg
     // is equal to double-precision -0.
 
     Label nonZero;
 
     // Compare to zero. Lets through {0, -0}.
     xorpd(ScratchFloatReg, ScratchFloatReg);
-    // If reg is non-zero, then a test of Zero is false.
+
+    // If reg is non-zero, jump to nonZero.
     branchDouble(DoubleNotEqual, reg, ScratchFloatReg, &nonZero);
 
-    // Input register is either zero or negative zero. Test sign bit.
+    // Input register is either zero or negative zero. Retrieve sign of input.
     movmskpd(reg, scratch);
-    // If reg is -0, then a test of Zero is true.
-    cmpl(scratch, Imm32(1));
+
+    // If reg is 1 or 3, input is negative zero.
+    // If reg is 0 or 2, input is a normal zero.
+    // To not need two tests and bearing in mind that the branchDouble
+    // also shouldn't fail. Wich has ZF=0 and PF=0. We need a test
+    // that gives ZF=1 or PF=1. Here is opted to set PF=1 for negative zero.
+    // So when reg equals 1 or 3 PF should give 1.
+    orl(Imm32(2), scratch);
 
     bind(&nonZero);
-    return Zero;
+    return Parity;
 }
 
 Assembler::Condition
 MacroAssemblerX86::testNegativeZeroFloat32(const FloatRegister &reg, const Register &scratch)
 {
     movd(reg, scratch);
     cmpl(scratch, Imm32(1));
     return Overflow;
Blocks: 745362
Flags: needinfo?(hv1989)
Attached patch Patch (obsolete) — Splinter Review
Sorry for previous comment. Now properly in an attachment.

The issue is here that "testNegativeZero" assumed the upper part of the register was zero or not negative zero. As a result when requesting the signs it only tested "1". But since the upper part can be garbage, it can also include negative zero. In that case it will contain "3". And we didn't bail properly.

I tried to have no extra instruction to test for both values :D.
Assignee: nobody → hv1989
Attachment #8406117 - Flags: review?(benj)
(In reply to Hannes Verschore [:h4writer] from comment #2)
> The issue is here that "testNegativeZero" assumed the upper part of the
> register was zero AND DEFINITELY not negative zero.
Comment on attachment 8406117 [details] [diff] [review]
Patch

Review of attachment 8406117 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the andl change. Thanks for asking this review, it had me check my cmp / or / and / test instructions knowledge :)
Don't forget to add the test:

function f(x) {
    return Math.round(-Math.tan(x > 0))
}
f(2);
assertEq(f(-1), -0);

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +408,5 @@
> +    // To not need two tests and bearing in mind that the branchDouble
> +    // also shouldn't fail. Wich has ZF=0 and PF=0. We need a test
> +    // that gives ZF=1 or PF=1. Here is opted to set PF=1 for negative zero.
> +    // So when reg equals 1 or 3 PF should give 1.
> +    orl(Imm32(2), scratch);

I understand why this works because of the comment, but I think there is something easier we could do. I would rather use andl(Imm32(1), scratch) that also sets the Zero flag and then return NonZero, which is equivalent and seems to better express the fact that we only care for the last bit (lower float64). You can also remove the long comment this way! How does it sound?

@@ +416,4 @@
>  }
>  
>  Assembler::Condition
>  MacroAssemblerX86::testNegativeZeroFloat32(const FloatRegister &reg, const Register &scratch)

For what it's worth, I've checked this code and it is correct: if reg is -0, scratch will contain 0x80000000 and thus cmpl(scratch, 1) will carry out ((0x80000000 | 0) - 1) | 0, which overflows in the negative range.
Attachment #8406117 - Flags: review?(benj) → review+
Attached patch PatchSplinter Review
Like discussed on irc andl wouldn't work.
Updated comment to explain that hopefully better
Attachment #8406117 - Attachment is obsolete: true
Attachment #8406943 - Flags: review?(benj)
Comment on attachment 8406943 [details] [diff] [review]
Patch

Review of attachment 8406943 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +397,5 @@
>      // Compare to zero. Lets through {0, -0}.
>      xorpd(ScratchFloatReg, ScratchFloatReg);
> +
> +    // If reg is non-zero, jump to nonZero.
> +    // Sets ZF=0 and PF=0.

hmm, remove the "Sets ZF=0 and PF=0.". It is repeated lateron
Comment on attachment 8406943 [details] [diff] [review]
Patch

Review of attachment 8406943 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for not catching that. Indeed we want to be able to distinguish cases where we have negative zeros with all other cases, and the ANDL strategy doesn't allow that (cases where input != 0 and input == -0 yields the same ZF value).

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +414,5 @@
> +    // Here we need to be able to test if the input is a negative zero.
> +    // - branchDouble merges here for non zero values. Which has ZF=0 and PF=0.
> +    //   In that case the test should fail.
> +    // - orl sets PF=1 on negative zero.
> +    // => So testing Parity will return if input is negative zero or not.

How about:

Here we need to be able to test if the input is a negative zero:
- branchDouble joins here for non-negative values, in which case it sets ZF=0 and PF=0.
- orl sets PF=1 on negative zero and PF=0 otherwise (positive zero).
Thus PF == 1 if, and only if, the input is -0, thus we should test for Parity.
Attachment #8406943 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/bd09a43b2b21
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.