This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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) 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. The attached revised patch has changes for both of these for your consideration. 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.) Craig -----Original Message----- From: newlib-owner@sourceware.org on behalf of Dave Korn Sent: Fri 7/16/2010 12:13 PM To: newlib@sourceware.org Subject: [PATCH] Fix undefined behaviour in lround,llround. Hi all, Back at the start of June we had a report(*) of a bug in the gfortran nint() function, which I tracked down to a problem in the underlying implementation of lround() in newlib. The bug occurs here: > else if (exponent_less_1023 < (8 * sizeof (long int)) - 1) > { > if (exponent_less_1023 >= 52) > result = ((long int) msw << (exponent_less_1023 - 20)) | (lsw << (exponent_less_1023 - 52)); > else > { > unsigned int tmp = lsw + (0x80000000 >> (exponent_less_1023 - 20)); > if (tmp < lsw) > ++msw; > result = ((long int) msw << (exponent_less_1023 - 20)) | (tmp >> (52 - exponent_less_1023)); > } > } > else When exponent_less_1023 is exactly 52, the subexpression "((long int) msw << (exponent_less_1023 - 20))" becomes a left shift by 32. On systems where a long int is 32 bits, that's undefined behaviour; the shift amount must always be less than the size of the left-hand operand (n1256, 6.5.7#3), and in practice, this means that a standard x86 shift opcode is used, which has the property of implicitly AND-ing the shift amount with 31; the result of a shift by 32 bits thus isn't zero as the code expects, but the original unshifted value of msw, which gets OR-d into completely the wrong place and splots bits all over the correctly-shifted lsw part of the result. In this patch I define a couple of safe shift macros that return zero and avoid the undefined behaviour when the shift amount is too great. I've actually been slightly over-cautious in applying it a couple of places, I think, but this isn't the fast math code and anyway value-range analysis is a job for a compiler, which it can do bearing in mind the actual type sizes in effect during compilation. newlib/ChangeLog: * libm/common/fdlibm.h (SAFE_LEFT_SHIFT): New macro definition. (SAFE_RIGHT_SHIFT): Likewise. * libm/common/s_llround.c (llround): Use them to avoid undefined behaviour. * libm/common/s_lround.c (lround): Likewise. OK? cheers, DaveK -- (*) - http://cygwin.com/ml/cygwin/2010-06/threads.html#00315
Attachment:
fpmath-lround-fix2.diff
Description: fpmath-lround-fix2.diff
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |