signed overflow in atan2

[Bruce Evans]

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.

Bruce