Closed Bug 946969 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: gkw, Assigned: dougc)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file, 2 obsolete files)

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.
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
(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.
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
Assignee: general → dtc-moz
Attachment #8343684 - Flags: review?(mrosenberg)
Sorry, correcting conditional.
Attachment #8343684 - Attachment is obsolete: true
Attachment #8343684 - Flags: review?(mrosenberg)
Attachment #8343685 - Flags: review?(mrosenberg)
This issue blocks running the compareJIT fuzzer on ARM platforms, setting [fuzzblocker].
Blocks: 465479
Whiteboard: [fuzzblocker]
Attachment #8343685 - Flags: review?(mrosenberg) → review+
Keywords: checkin-needed
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.
Removing checkin-needed pending comment 5.
Flags: needinfo?(dtc-moz)
Keywords: checkin-needed
Address feedback in comment 5 - use the common nomenclature of 'NotSigned'.

Carrying forward r+.
Attachment #8343685 - Attachment is obsolete: true
Attachment #8344246 - Flags: review+
Flags: needinfo?(dtc-moz)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/80115f5943ed
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/80115f5943ed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.