From owner-freebsd-numerics@freebsd.org Tue Feb 13 20:35:44 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 0CAF6F22485 for ; Tue, 13 Feb 2018 20:35:44 +0000 (UTC) (envelope-from sgk@troutmask.apl.washington.edu) Received: from troutmask.apl.washington.edu (troutmask.apl.washington.edu [128.95.76.21]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "troutmask", Issuer "troutmask" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 3EB3C6FC86 for ; Tue, 13 Feb 2018 20:35:43 +0000 (UTC) (envelope-from sgk@troutmask.apl.washington.edu) Received: from troutmask.apl.washington.edu (localhost [127.0.0.1]) by troutmask.apl.washington.edu (8.15.2/8.15.2) with ESMTPS id w1DKLemw000646 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 13 Feb 2018 12:21:40 -0800 (PST) (envelope-from sgk@troutmask.apl.washington.edu) Received: (from sgk@localhost) by troutmask.apl.washington.edu (8.15.2/8.15.2/Submit) id w1DKLe4r000645; Tue, 13 Feb 2018 12:21:40 -0800 (PST) (envelope-from sgk) Date: Tue, 13 Feb 2018 12:21:40 -0800 From: Steve Kargl To: Bruce Evans Cc: Eitan Adler , freebsd-numerics@freebsd.org Subject: Re: signed overflow in atan2 Message-ID: <20180213202140.GA636@troutmask.apl.washington.edu> Reply-To: sgk@troutmask.apl.washington.edu References: <20180213192144.N1911@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180213192144.N1911@besplex.bde.org> User-Agent: Mutt/1.9.2 (2017-12-15) 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 20:35:44 -0000 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