Closed
Bug 1437729
Opened 7 years ago
Closed 7 years ago
Math.atan2(1, -0) triggers signed integer overflow in fdlibm
Categories
(Core :: JavaScript: Standard Library, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(1 file)
2.52 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
[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.
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
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+
Comment 3•7 years ago
|
||
Oh no, I bet this regresses performance on the 1990s-era CPUs and compilers on which fdlibm was originally perf-tuned!
Updated•7 years ago
|
status-firefox60:
--- → affected
Priority: -- → P1
Comment 4•7 years ago
|
||
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
Comment 6•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•