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

[Pedro Giffuni]

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.