[PATCH] Fix undefined behaviour in lround,llround.

Dave Korn dave.korn.cygwin@gmail.com
Fri Jul 16 21:12:00 GMT 2010

On 16/07/2010 18:05, Howland Craig D (Craig) wrote:
> Dave:
> A small suggestion for the macros (not a problem as used now,
> but paranoia for future):
> -(((amt) < 8 * sizeof(op)) ? (op << amt) : 0)
> +(((amt) < 8 * sizeof(op)) ? ((op) << (amt)) : 0)
> -(((amt) < 8 * sizeof(op)) ? (op >> amt) : 0)
> +(((amt) < 8 * sizeof(op)) ? ((op) >> (amt)) : 0)

  Yeah, that one's a no-brainer really, I should have done it that way in the
first place, who knows how those macros might end up getting used in future.

> Changes look good to me, but, yes, they are over-cautious in
> applying the macro, as they are only needed in one place each in the
> files--the first one.  In the final if (two shifts inside),
> 20 <= exponent_less_1023 <= 51, so the possible shift amounts are only
> 0 to 31, not needing the protection.

  I make it that the "SAFE_RIGHT_SHIFT (tmp, (52 - exponent_less_1023))" is
necessary because 52 - 20 == 32.

  It was the SAFE_LEFT_SHIFT of that pair that I thought was unnecessary, and
the SAFE_RIGHT_SHIFT of tmp above, both of which have amt ==
(exponent_less_1023 - 20), but I left it in for consistency and hoped the
compiler would be clever enough to get rid of it.  I'd be happy to omit those
two if that's how folks prefer the consistency/efficiency trade-off.

> I think that this error is likely in more files (lrint is likely),
> as there are many similarites.  (I'll have to take a look later.)

  Thanks.  I convinced myself that the rounding functions that use the TWO52[]
array are safe, but a second pair of eyeballs would certainly be good.


More information about the Newlib mailing list