1437729 - Math.atan2(1, -0) triggers signed integer overflow in fdlibm

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect, P1)

Description

[Jeff Walden [:Waldo]]

[jwalden@find-waldo-now src]$ dbg/js/src/js -e 'Math.atan2(1, -0)'
/home/jwalden/moz/after/modules/fdlibm/src/e_atan2.cpp:73:8: runtime error: signed integer overflow: -2147483648 - 1072693248 cannot be represented in type 'int'

Didn't look closely at the fdlibm code, except to see it's doing grody bit-level things (real shocker).

This is one of two (count 'em) signed integer overflows in the entirety of SpiderMonkey running jstests.py.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

fdlibm's handling of this is very silly.  It appears to want to convert |atan2(y, 1.0)| to simply |atan(y)|, by detecting when |x == 1.0| and then returning the latter expression.  But for unexplained reasons it doesn't just do a simple floating point comparison.  Nor does it individually compare the high/low halves of the bits of |x| to their expected IEEE-754 patterns.  Nope, that'd be too simple.

Instead, it bitwise-ors together the lower-half bits, with the upper-half bits exclusive-or'd with the bit pattern that would be present for the biased exponent zero.  For 1.0, the significand should be all zeroes, the sign bit should be zero, and the exponent field should exactly equal the bias.  For IEEE-754 double precision, this is 1023 -- exactly equal to 0x3FF.

Then, this bitwise-or'd combination is compared to zero.

So the only way this condition is true, is if there are no bits set in the lower half, *and* if |high bits - 0x3FF00000| is zero (i.e. the high bits must *be* that value).

So then, there's no reason not to just compare the low/high bits to the exact demanded values.  (Or to just do |x == 1.0|, but the style seems to be not to do that, so ¯\_(ツ)_/¯.)

This patch is hand-generated, and the patch in modules/fdlibm/patches was the result of a file-targeted |hg diff|.  I tried running update.sh (hence the +x on it), but the script currently horks due to upstream changes.  If I have to fall on that sword, I'd prefer to do it *after* fixing this, or at least separate from it.

These algorithms could really do with a bunch of commenting and cleanup, but I guess with upstream fixed on C, and our not wanting to really fork this long-term, we should keep things minimal.  I've submitted the patch upstream as https://github.com/freebsd/freebsd/pull/130 with precisely this minimal change.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8950473 [details] [diff] [review]
Patch

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

Makes sense.
Thanks!

Comment 3

[Jason Orendorff [:jorendorff]]

Oh no, I bet this regresses performance on the 1990s-era CPUs and compilers on which fdlibm was originally perf-tuned!

Comment 4

[Tooru Fujisawa [:arai]]

now https://github.com/freebsd/freebsd/pull/130 is merged,
I'll import the upstream change in bug 1438599.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ba48e8eb78
Make fdlibm::atan2(y, x)'s handling for |x == 1.0| not potentially invoke signed integer overflow.  r=arai

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c2ba48e8eb78