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?

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?

(thanks for the report, gary!)

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.

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

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

... so I've factored it out.

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)

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.

Comment on attachment 8562090 [details] [diff] [review] factorout.patch Review of attachment 8562090 [details] [diff] [review]: ----------------------------------------------------------------- (for consistency, switching review here as well)

Comment on attachment 8564137 [details] [diff] [review] other-checks.patch Review of attachment 8564137 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good.

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

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

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

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

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.