Closed Bug 1122401 Opened 5 years ago Closed 5 years ago

Differential Testing: Different output message involving Math.ceil

Categories

(Core :: JavaScript Engine: JIT, defect, major)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

function f(x) {
    return Math.ceil((x >>> 0) / 2) >> 0;
}
f(2);
print(f(1));

$ ./js-dbg-opt-32-dm-armSim-darwin-cac6192956ab --fuzzing-safe --no-threads --ion-eager testcase.js
0

$ ./js-dbg-opt-32-dm-armSim-darwin-cac6192956ab --fuzzing-safe --no-threads --baseline-eager testcase.js
1

Tested this on m-c rev cac6192956ab.

My configure flags are:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/fuzz2/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-debug --enable-optimize --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e3e21c3ada8c
user:        Benjamin Bouvier
date:        Thu Dec 19 15:32:59 2013 +0100
summary:     Bug 936740: inline call to libc's ceil for Math.ceil(); r=jandem

Benjamin, is bug 936740 a likely regressor?
Flags: needinfo?(benj)
Can't reproduce locally. Bisection lead to this:

The first good revision is:
changeset:   224478:9ccd38ce999f
user:        Nicolas Devillers <devillers.nicolas+moz@gmail.com>
date:        Mon Jan 19 11:16:32 2015 +0100
summary:     Bug 1096129 - IonMonkey: Implement Ceil Recover Instruction. r=nbp

Trying to understand, because it might just be hiding the bug...
The issue somehow got hidden by RCeil (i couldn't find why but didn't search
too much), but the same can raise with Math.round instead of ceil.  The problem
is that we're performing an unsigned soft division, calling uidivmod, but in
the case of a division, we're not checking that the remainder is non zero, in
which case we should bail out and we're currently not.

Douglas, I saw that you added the check for 0 above as well.  Are there other
checks we should add there?
Attachment #8560555 - Flags: review?(dtc-moz)
Assignee: nobody → benj
Status: NEW → ASSIGNED
(thanks for the report, gary!)
Flags: needinfo?(benj)
Comment on attachment 8560555 [details] [diff] [review]
Check that the remainder is null in unsigned soft divisions on ARM or bailout

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

Good catch.
Attachment #8560555 - Flags: review?(dtc-moz) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Created attachment 8560555 [details] [diff] [review]
> Check that the remainder is null in unsigned soft divisions on ARM or bailout
...
> Douglas, I saw that you added the check for 0 above as well.  Are there other
> checks we should add there?

It does look like there is an opportunity to optimize the zero-rhs check a little based on canBeDivideByZero(), as is done in visitUDiv() and visitUMod().

There are checks and bailouts based on the result in visitUDiv() and visitUMod(). These appear to have been added added in bug 941877. Might these be needed in visitSoftUDirMod() too?
Also visitUDiv() does not appear to have a check that the remainder is zero, so is that an issue too? The udiv instruction on the ARM gives only a 32 bit result, and not the remainder.
Attached patch other-checks.patch (obsolete) — Splinter Review
Thanks! Indeed there are other checks that we need to do (check that the result can fit in an int32 + optimizations for the zero case).  The code is kind of redundant, so...
Attachment #8562087 - Flags: review?(dtc-moz)
Attached patch factorout.patchSplinter Review
... so I've factored it out.
Attachment #8562090 - Flags: review?(dtc-moz)
Regarding comment 7: the ARM v7 ARM doesn't seem to show two output registers for the result of udiv.  The only output register appears to be the divide result rounded towards zero.  So as long as we're sure we want to use unsigned div, we're good there... (wonder if we'd need to use umod before using udiv, to make sure that the modulus indeed is zero)
Duplicate of this bug: 1132396
Note that the test case in the duplicate bug is fixed by patch 2.  I've added the test case there.  Douglas, please let me know if I should forward review?
After discussion with dougc, we agreed it'd be nice to have somebody familiar with range analysis / bailout confirm all of this is correct and we're not missing out other cases.  Switching review to nbp.
Attachment #8562087 - Attachment is obsolete: true
Attachment #8562087 - Flags: review?(dtc-moz)
Attachment #8564137 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8562090 [details] [diff] [review]
factorout.patch

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

(for consistency, switching review here as well)
Attachment #8562090 - Flags: review?(dtc-moz) → review?(nicolas.b.pierron)
Attachment #8562090 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8564137 [details] [diff] [review]
other-checks.patch

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

Sounds good.
Attachment #8564137 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #16)
> Comment on attachment 8564137 [details] [diff] [review]
> other-checks.patch
> 
> Review of attachment 8564137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sounds good.

What about visitUDiv in the case that the remainder is not zero, see comment 10? This check appears to exist on the x86 and in the visitSoftUDivOrMod() path.
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/5d0cfe96d1fe
https://hg.mozilla.org/mozilla-central/rev/fbc610bf6dfd
https://hg.mozilla.org/mozilla-central/rev/49426db6bbef
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(In reply to Douglas Crosher [:dougc] from comment #7)
> Also visitUDiv() does not appear to have a check that the remainder is zero,
> so is that an issue too? The udiv instruction on the ARM gives only a 32 bit
> result, and not the remainder.

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> Regarding comment 7: the ARM v7 ARM doesn't seem to show two output
> registers for the result of udiv.  The only output register appears to be
> the divide result rounded towards zero.  So as long as we're sure we want to
> use unsigned div, we're good there... (wonder if we'd need to use umod
> before using udiv, to make sure that the modulus indeed is zero)

(In reply to Douglas Crosher [:dougc] from comment #17)
> What about visitUDiv in the case that the remainder is not zero, see comment
> 10? This check appears to exist on the x86 and in the visitSoftUDivOrMod()
> path.

The fact that ARM is lacking this feature is just due to the laziness to port the code to ARM, at the time of the initial dev, and then the code got out-of sync that nobody took the time to port it back.
Flags: needinfo?(nicolas.b.pierron)
The test case for this bug fails on ARMv7 with --ion-eager --ion-offthread-compile=off: bug 1202643.
(In reply to Lars T Hansen [:lth] from comment #21)
> The test case for this bug fails on ARMv7 with --ion-eager
> --ion-offthread-compile=off: bug 1202643.

Might want to look at some of the queries noted above as the patch did not look done to me at the time.
You need to log in before you can comment on or make changes to this bug.