1122401 - Differential Testing: Different output message involving Math.ceil

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

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?

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Can't reproduce locally. Bisection lead to this:

The first good revision is:
changeset:   224478:9ccd38ce999f
user:        Nicolas Devillers <[e-mail]>
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...

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Check that the remainder is null in unsigned soft divisions on ARM or bailout

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?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(thanks for the report, gary!)

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a909b3bb6c5

Comment 5

[Douglas Crosher [:dougc]]

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.

Comment 6

[Douglas Crosher [:dougc]]

(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?

Comment 7

[Douglas Crosher [:dougc]]

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.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: other-checks.patch

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...

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: factorout.patch

... so I've factored it out.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

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)

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f1d21a21d6

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: other-checks.patch

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.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8562090 [details] [diff] [review]
factorout.patch

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

(for consistency, switching review here as well)

Comment 16

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8564137 [details] [diff] [review]
other-checks.patch

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

Sounds good.

Comment 17

[Douglas Crosher [:dougc]]

(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.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0cfe96d1fe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc610bf6dfd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/49426db6bbef

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d0cfe96d1fe
https://hg.mozilla.org/mozilla-central/rev/fbc610bf6dfd
https://hg.mozilla.org/mozilla-central/rev/49426db6bbef

Comment 20

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(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.

Comment 21

[Lars T Hansen [:lth]]

The test case for this bug fails on ARMv7 with --ion-eager --ion-offthread-compile=off: bug 1202643.

Comment 22

[Douglas Crosher [:dougc]]

(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.