From owner-freebsd-numerics@freebsd.org Tue Feb 13 09:23:05 2018 Return-Path: Delivered-To: freebsd-numerics@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id ADD7DF0E51B for ; Tue, 13 Feb 2018 09:23:05 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 2AA7B6DD24 for ; Tue, 13 Feb 2018 09:23:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id D93FF429C40; Tue, 13 Feb 2018 20:23:02 +1100 (AEDT) Date: Tue, 13 Feb 2018 20:23:01 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler cc: freebsd-numerics@freebsd.org Subject: Re: signed overflow in atan2 In-Reply-To: Message-ID: <20180213192144.N1911@besplex.bde.org> References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=DIX/22Fb c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=NEAV23lmAAAA:8 a=Sjz6Rct6sMvockCiqwcA:9 a=CjuIK1q_8ugA:10 X-BeenThere: freebsd-numerics@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "Discussions of high quality implementation of libm functions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Feb 2018 09:23:05 -0000 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