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

[Pedro Giffuni]

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.