Undefined Behavior in lib/msun/src/e_pow.c (was Re: New math library from ARM)

Pedro Giffuni pfg at FreeBSD.org
Tue Jan 1 15:01:50 UTC 2019

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: msun-pow-ub.diff
Type: text/x-patch
Size: 468 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-numerics/attachments/20190101/e17bf833/attachment.bin>

More information about the freebsd-numerics mailing list