signed overflow in atan2

[Steve Kargl]

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