signed overflow in atan2
Steve Kargl
sgk at troutmask.apl.washington.edu
Tue Feb 13 20:35:44 UTC 2018
On Tue, Feb 13, 2018 at 08:23:01PM +1100, Bruce Evans wrote:
> On Mon, 12 Feb 2018, Eitan Adler wrote:
>
> > source: https://github.com/freebsd/freebsd/pull/130
> >
> > `atan2(y, x)`'s special case for `x == 1.0` (in which case the result
> > must be `atan(y)`) is unnecessarily complicated #130
> >
> > As a component of atan2(y, x), the case of x == 1.0 is farmed out to
> > atan(y). The current implementation of this comparison is vulnerable
> > to signed integer underflow (that is, undefined behavior), and it's
> > performed in a somewhat more complicated way than it need be. Change
> > it to not be quite so cute, rather directly comparing the high/low
> > bits of x to the specific IEEE-754 bit pattern that encodes 1.0.
> >
> > Note that while there are three different e_atan* files in the
> > relevant directory, only this one needs fixing. e_atan2f.c already
> > compares against the full bit pattern encoding 1.0f, while
> > e_atan2l.cuses bitwise-ands/ors/nots and so doesn't require a change.
> >
> > (I ran into this in Mozilla's SpiderMonkey embedding of fdlibm, where
> > a test of the behavior of Math.atan2(1, -0) triggered clang's
> > signed-overflow-sanitizer runtime error.)
> >
> > -----
> >
> > patch:
> >
> > if(((ix|((lx|-lx)>>31))>0x7ff00000)||
> > ((iy|((ly|-ly)>>31))>0x7ff00000)) /* x or y is NaN */
> > return x+y;
> > - if((hx-0x3ff00000|lx)==0) return atan(y); /* x=1.0 */
> > + if(hx==0x3ff00000&&lx==0) return atan(y); /* x=1.0 */
> > m = ((hy>>31)&1)|((hx>>30)&2); /* 2*sign(x)+sign(y) */
> > /* when y = 0 */
>
> Seems reasonable.
>
> Probably hx and hy should be uint32_t, but that would be a much larger
> change (fdlibm+Cygnus fixes does it wrong in almost all places. Cygnus
> changed lots of types to fixed-width, but didn't change many from signed
> to unsigned). The next line that does ((hy>31&1) to extract the sign
> bit is also unportable.
>
> 5 other files in msun/src have a similar test. 3 of these are OK because
> they test ix instead of hx. e_acosh.c is OK because it only tests hx in
> a clause where hx > 0 is known. e_pow.c is OK because it already uses the
> && form of the test.
>
> I don't see how clang can usefully warn about this. In the tests with ix,
> it can only print the harmful warning that it proved that integer overflow
> is impossible. In the above, it can't know that hx is negative enough to
> cause overflow, just like it can't know if overflow occurs for x-1 because
> x might be INT_MIN.
>
I've had the following in my msun tree for awhile.
--- math_private.h.orig 2017-05-30 09:22:20.565401000 -0700
+++ math_private.h 2017-12-14 13:05:54.128219000 -0800
@@ -84,7 +84,7 @@
#endif
-/* Get two 32 bit ints from a double. */
+/* Get two unsigned 32 bit ints from a double. */
#define EXTRACT_WORDS(ix0,ix1,d) \
do { \
@@ -166,8 +166,7 @@
typedef union
{
float value;
- /* FIXME: Assumes 32 bit int. */
- unsigned int word;
+ uint32_t word;
} ieee_float_shape_type;
/* Get a 32 bit int from a float. */
--
Steve
More information about the freebsd-numerics
mailing list