Date: Tue, 13 Feb 2018 12:21:40 -0800 From: Steve Kargl <sgk@troutmask.apl.washington.edu> To: Bruce Evans <brde@optusnet.com.au> Cc: Eitan Adler <lists@eitanadler.com>, freebsd-numerics@freebsd.org Subject: Re: signed overflow in atan2 Message-ID: <20180213202140.GA636@troutmask.apl.washington.edu> In-Reply-To: <20180213192144.N1911@besplex.bde.org> References: <CAF6rxgkDVfdN6hKEfrMj6WcNKJHZggq33e9zbM6kaaiXgkoChA@mail.gmail.com> <20180213192144.N1911@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180213202140.GA636>