Closed Bug 1437729 Opened 5 years ago Closed 5 years ago

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

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(1 file)

[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.
Attached patch PatchSplinter Review
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.
Attachment #8950473 - Flags: review?(arai.unmht)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8950473 [details] [diff] [review]
Patch

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

Makes sense.
Thanks!
Attachment #8950473 - Flags: review?(arai.unmht) → review+
Oh no, I bet this regresses performance on the 1990s-era CPUs and compilers on which fdlibm was originally perf-tuned!
Priority: -- → P1
See Also: → 1438599
now https://github.com/freebsd/freebsd/pull/130 is merged,
I'll import the upstream change in bug 1438599.
Pushed by jwalden@mit.edu:
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
https://hg.mozilla.org/mozilla-central/rev/c2ba48e8eb78
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.