Date: Tue, 1 Jan 2019 01:20:53 -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: <e6bd09b3-35c6-922b-1b4d-de27e424a627@FreeBSD.org> In-Reply-To: <20190101055225.GA5982@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> <d01c8770-5431-0634-4ecc-37069ff76458@FreeBSD.org> <20190101055225.GA5982@troutmask.apl.washington.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On 1/1/19 12:52 AM, Steve Kargl wrote: > On Tue, Jan 01, 2019 at 12:14:38AM -0500, Pedro Giffuni wrote: >>> 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. > I'am aware of what modern standards allow. I'm also > aware of the code style of fdlibm, which I think > should be maintain. Well, the alternative is the musl-libc patch, which declares the variables upon use (twice). I think my patch is a little bit more acceptable. >>> 2) j is already declared as int32_t >> And it has to be a signed integer: we later check for negative values. > Yeah, I know. That's why I pointed out the collision. > >>> 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. > Ah, no. e_pow.c uses u_int32_t on line 107. Duh! I completely misread, sorry about that I probably was still looking at musl-libc instead. Yes, we should use u_int32_t. > This would be easy > to see if all declarations are grouped together. In additional, > don't you need to include stdint.h to get uint32_t? Well, my patch did compile, but I will match the existing style if the change is accepted/desired. > % grep uint32_t /usr/src/lib/msun/src/e_pow.c > % grep u_int32_t /usr/src/lib/msun/src/e_pow.c > u_int32_t lx,ly; > n = ((u_int32_t)hx>>31)-1; > > Code churn to placate lint for an event that cannot occur > seems dubious to me. > UBsan should be able to detect it. I think Undefined Behavior is considered a portability bug. There is no urgency but it is still a bug. Pedro.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?e6bd09b3-35c6-922b-1b4d-de27e424a627>