946969 - Differential Testing: Different output message involving Math.abs and Math.tan

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f() {
    return Math.abs(~(Math.tan()));
}
x = [];
x.push(f());
x.push(f());
print(x);

shows "1,1" without --ion-eager but shows "1,-1" with --ion-eager. Tested on js debug shell on m-c changeset 1426ffa9caf2 on ARM Ubuntu Linux.

My configure flags are:

CC="gcc -mfloat-abi=softfp -B/usr/lib/gcc/arm-linux-gnueabi/4.7" AR=ar CXX="g++ -mfloat-abi=softfp -B/usr/lib/gcc/arm-linux-gnueabi/4.7" sh ./configure --target=arm-linux-gnueabi --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/348b2ba27515
user:        David Caabeiro
date:        Mon Jul 15 10:03:14 2013 -0500
summary:     Bug 717379, part 1 - Implement the new ES6 math functions. r=jorendorff.

This may have been a bug from whenever ES6 math functions landed in bug 717379. Setting needinfo? from ARM folks.

Comment 1

[Douglas Crosher [:dougc]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> function f() {
>     return Math.abs(~(Math.tan()));
> }
> x = [];
> x.push(f());
> x.push(f());
> print(x);
> 
> shows "1,1" without --ion-eager but shows "1,-1" with --ion-eager. Tested on
> js debug shell on m-c changeset 1426ffa9caf2 on ARM Ubuntu Linux.
...

Good catch, thanks.  The problem is the AbsI operation on the ARM.  It uses the GreaterThanOrEqual condition after a 'tst' instruction, but the V flag is undefined on the ARM after this instruction.  It should be an easy fix to just use the 'Unsigned' condition test.  Shall do some testing and check the x86.

Comment 2

[Douglas Crosher [:dougc]]

Attachment: Correct AbsI, integer absolute value

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Correct AbsI, integer absolute value

Sorry, correcting conditional.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This issue blocks running the compareJIT fuzzer on ARM platforms, setting [fuzzblocker].

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8343685 [details] [diff] [review]
Correct AbsI, integer absolute value

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

::: js/src/jit/CodeGenerator.cpp
@@ +3799,5 @@
>      masm.test32(input, input);
> +#ifdef JS_CPU_ARM
> +    masm.j(Assembler::Unsigned, &positive);
> +#else
> +    masm.j(Assembler::NotSigned, &positive);

Thanks for fixing this! But instead of adding another #ifdef JS_CPU_ARM can we please use either Unsigned or NotSigned everywhere? We try to avoid such #ifdefs as much as possible.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Removing checkin-needed pending comment 5.

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Correct AbsI, integer absolute value

Address feedback in comment 5 - use the common nomenclature of 'NotSigned'.

Carrying forward r+.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80115f5943ed

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80115f5943ed