Date: Tue, 1 Jan 2019 10:01:45 -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: <9cdce84b-2cff-d752-67a9-4c9ef391cc2c@FreeBSD.org> In-Reply-To: <e6bd09b3-35c6-922b-1b4d-de27e424a627@FreeBSD.org> 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> <e6bd09b3-35c6-922b-1b4d-de27e424a627@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------82A09505CD03AE157475B862
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
On 1/1/19 1:20 AM, Pedro Giffuni wrote:
>
> 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.
>
Actually, perhaps a cast is just enough ...
See below ...
>>>> 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.
>
In the new patch: the first j comparison has to be made against an
unsigned value anyways and the second comparison will never be against
the magical 31 so there is no need to cast that one.
Pedro.
--------------82A09505CD03AE157475B862
Content-Type: text/x-patch;
name="msun-pow-ub.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="msun-pow-ub.diff"
Index: lib/msun/src/e_pow.c
===================================================================
--- lib/msun/src/e_pow.c (revision 342665)
+++ lib/msun/src/e_pow.c (working copy)
@@ -133,7 +133,7 @@
k = (iy>>20)-0x3ff; /* exponent */
if(k>20) {
j = ly>>(52-k);
- if((j<<(52-k))==ly) yisint = 2-(j&1);
+ if(((u_int32_t)j<<(52-k))==ly) yisint = 2-(j&1);
} else if(ly==0) {
j = iy>>(20-k);
if((j<<(20-k))==iy) yisint = 2-(j&1);
--------------82A09505CD03AE157475B862--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9cdce84b-2cff-d752-67a9-4c9ef391cc2c>