Date: Tue, 1 Jan 2019 00:14:38 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: sgk@troutmask.apl.washington.edu Cc: freebsd-numerics@freebsd.org Subject: Re: Undefined Behavior in lib/msun/src/e_pow.c (was Re: New math library from ARM) Message-ID: <d01c8770-5431-0634-4ecc-37069ff76458@FreeBSD.org> In-Reply-To: <20190101045425.GA5767@troutmask.apl.washington.edu> References: <797a7755-db93-1b9c-f3b9-8850d948e098@FreeBSD.org> <20181231151904.GB823@troutmask.apl.washington.edu> <20181231152230.GC823@troutmask.apl.washington.edu> <06c8b6a2-ed26-f255-3947-c79b593a9dea@FreeBSD.org> <20190101045425.GA5767@troutmask.apl.washington.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On 31/12/2018 23:54, Steve Kargl wrote: > On Mon, Dec 31, 2018 at 10:32:06PM -0500, Pedro Giffuni wrote: >> Hmm ... >> >> Looking at the changes in musl's libc I found this issue which seems >> real (although somewhat theoretical): >> >> https://git.musl-libc.org/cgit/musl/commit/src/math?id=688d3da0f1730daddbc954bbc2d27cc96ceee04c >> >> Is the attached patch acceptable? >> >> Also, their code is bit different here: >> >> https://git.musl-libc.org/cgit/musl/commit/src/math?id=282b1cd26649d69de038111f5876853df6ddc345 >> >> but we may also have to check fmaf(-0x1.26524ep-54, -0x1.cb7868p+11, >> 0x1.d10f5ep-29). >> >> Cheers, >> >> Pedro. >> >> Index: lib/msun/src/e_pow.c >> =================================================================== >> --- lib/msun/src/e_pow.c (revision 342665) >> +++ lib/msun/src/e_pow.c (working copy) >> @@ -130,6 +130,7 @@ >> if(hx<0) { >> if(iy>=0x43400000) yisint = 2; /* even integer y */ >> else if(iy>=0x3ff00000) { >> + uint32_t j; /* Avoid UB in bit operations below. */ >> k = (iy>>20)-0x3ff; /* exponent */ >> if(k>20) { >> j = ly>>(52-k); > > I'll defer to Bruce on this. My only comments are > 1) declarations belong at the top of the file where all declarations occur Modern standards let you declare variables within blocks. For style we do prefer to start the function with them but in this case we can't. > 2) j is already declared as int32_t And it has to be a signed integer: we later check for negative values. > 3) uint32_t should be written as u_int32_t. No, uint32_t is the standard type, u_int32_t is a BSDism. Also, the file consistently uses uint32_t. > Are you sure that UB occurs? Or, is this an attempt to > placate a static analysis tool that only see shifting of > a signed type? Do you need to make a similar change to > e_powf.c? The point of the Undefined Behavior is precisely that it doesn't occur: we are lucky and the compiler behaves as it always has but the standard will let a different compiler behave differently and then we will have surprises.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?d01c8770-5431-0634-4ecc-37069ff76458>