| msg120492 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-05 13:56 |
| Include/pyport.h invites potential compile errors with the definitions #define PY_LLONG_MIN LLONG_MIN #define PY_LLONG_MAX LLONG_MAX #define PY_ULLONG_MAX ULLONG_MAX which can fall back to gcc variants or to #else /* Otherwise, rely on two's complement. */ #define PY_ULLONG_MAX (~0ULL) #define PY_LLONG_MAX ((long long)(PY_ULLONG_MAX>>1)) #define PY_LLONG_MIN (-PY_LLONG_MAX-1) Code developed with the former #definitions might use them in '#if's, and then break when it meets a host where the fallbacks are used. It would be safer if either all the macros and pyconfig variants used casts, or all used predefined constants - from configure if needed. The signed variants would break because '#if's do not accept casts. PY_ULLONG_MAX is more insidious: If it meets a host which supports a type bigger than unsigned long long, then preprocessor arithmetic will happen in that type. ~0ULL in #if statements is then actually the same as ~ULLL or whatever it would be spelled. This one definitely needs a cast to protect from the surprise that preprocessor value != value outside preprocessor. You get the same effect with ~0U vs ~0UL on a 64-bit compiler, and ~0U vs ~0ULL on a C99 compiler: #if (~0U) == (~0ULL) # error "oops" #endif Incidentally, the "two's complement" comment is wrong. It also relies on unsigned long long being widest type with no padding bits, and -LLONG_MAX-1 not being a trap representation. ~0ULL is not two's complement since it is unsigned, it works because it has the same result as -1ULL which is defined to have the max value. The PY_LLONG_MIN definitions rely on two's complement. If anyone cared, one could avoid that with #define PY_LLONG_MIN (-PY_LLONG_MAX-(/*two's complement*/(-1LL & 3)==3)) Anyway. If they use casts, fix PY_TIMEOUT_MAX in 3.2a3 pythread.h: #define PY_MIN(x, y) ((x) < (y) ? (x) : (y)) #define PY_TIMEOUT_MAXTMP instead of PY_TIMEOUT_MAX, and then #ifndef NT_THREADS #define PY_TIMEOUT_MAX PY_TIMEOUT_MAXTMP #else #define PY_TIMEOUT_MAX PY_MIN(Py_LL(0xFFFFFFFF)*1000, PY_TIMEOUT_MAXTMP) #endif |
|
|
| msg120517 - (view) |
Author: Mark Dickinson (mark.dickinson) *  |
Date: 2010-11-05 17:47 |
| Thanks for the report; I agree that there's a potential issue here, and I also think that all these definitions *should* be preprocessor defines. (Idle question: does C99 require that LONG_MAX and friends are usable in the preprocessor? I see it in e.g. 7.18.2p2 for INT32_MAX and friends, but I'm not sure if there's something similar for LONG_MAX and co.) Can you suggest a suitable fix for the PY_ULLONG_MAX and PY_LLONG_MAX defines? Note that a configure-based fix may need to take into account the special needs of Windows---for which configure isn't used, of course---and OS X---where the same code, based on a single run of configure, should be able to compile to the correct thing both in 32-bit and 64-bit mode, so that universal builds work; see PC/pyconfig.h and Include/pymacconfig.h respectively for dealing with these issues. BTW, do you know of any modern non-Windows platforms that don't define LLONG_MIN and LLONG_MAX? It may well be that the "two's complement" fallback hasn't been exercised in recent years. > Incidentally, the "two's complement" comment is wrong. > It also relies on unsigned long long being widest type with no > padding bits, and -LLONG_MAX-1 not being a trap representation. Agreed---that comment needs to be better. I think it's fine, though, for practical purposes to assume an absence of padding bits and no trap representation; IIRC there are places internally (e.g., in the bitwise operators section of the 'int' type implementation) that already assume two's complement + no padding bits + no trap representation. (I *thought* I had an old issue open somewhere about documenting---and possibly testing---these assumptions, but now I can't find it.) |
|
|
| msg120646 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-06 18:43 |
| Mark Dickinson writes: > Thanks for the report; I agree that there's a potential issue here, and > I also think that all these definitions *should* be preprocessor > defines. Indeed, my suggestion to castify everything for uniformity was silly: My own PY_TIMEOUT_MAX fix illustrates why that won't promote portability. It breaks code which #ifdefs between using LONG_MAX and PY_LLONG_MAX. > (Idle question: does C99 require that LONG_MAX and friends are > usable in the preprocessor? ...) 5.2.4.2.1p1: Sizes of integer types <limits.h>. > Can you suggest a suitable fix for the PY_ULLONG_MAX and PY_LLONG_MAX > defines? (...) As far as I can tell, PC/pyconfig.h already solves it for Windows. For pyport.h, since you do #define SIZEOF_LONG_LONG: #define PY_LLONG_MAX \ (1 + 2 * ((Py_LL(1) << (CHAR_BIT*SIZEOF_LONG_LONG-2)) - 1)) #define PY_ULLONG_MAX (PY_LLONG_MAX * 2ULL + 1) You could check PY_ULLONG_MAX with a compile-time assertion if you want: #ifndef __cplusplus /* this requires different magic in C++ */ /* Compile-time assertion -- max one per post-preprocessed line */ #define Py_static_assert(expr) Py_static_assert1_(expr, __LINE__) #define Py_static_assert1_(expr, line) Py_static_assert2_(expr, line) #define Py_static_assert2_(expr, line) struct Py_static_assert##line { \ int Assert1_[(expr) ? 9 : -9]; int Assert2_: (expr) ? 9 : -9; } Py_static_assert(PY_ULLONG_MAX == (unsigned long long)-1); #endif /* __cplusplus */ > BTW, do you know of any modern non-Windows platforms that don't define > LLONG_MIN and LLONG_MAX? It may well be that the "two's complement" > fallback hasn't been exercised in recent years. Anyting compiled with strict ANSI pre-C99 mode, e.g. gcc -ansi, which you do have a workaround for. But gcc isn't the only one to be slow in upgrading to C99. And unfortunately, even if Python is built without a compiler's equivalent of -ansi, a user embedding Python might be compiling with it. Beyond that: No, I know none, but I don't know many platforms anyway. >> Incidentally, the "two's complement" comment is wrong. >> It also relies on unsigned long long being widest type with no >> padding bits, and -LLONG_MAX-1 not being a trap representation. > > Agreed---that comment needs to be better. I think it's fine, though, > for practical purposes to assume an absence of padding bits and no trap > representation; IIRC there are places internally (e.g., in the bitwise > operators section of the 'int' type implementation) that already assume > two's complement + no padding bits + no trap representation. I expect so, yes. It's easy to find breakage with non-two's complement, just grep the C code for '~'. I just get peeved when people get this wrong, then document and promote the errors:) |
|
|
| msg120648 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-06 19:06 |
| I wrote: > #define PY_LLONG_MAX \ > (1 + 2 * ((Py_LL(1) << (CHAR_BIT*SIZEOF_LONG_LONG-2)) - 1)) > #define PY_ULLONG_MAX (PY_LLONG_MAX * 2ULL + 1) Eh, Py_ULL(2). > (...) I just get peeved when people get this > wrong, then document and promote the errors:) Just to be clear, I'm peeving at the comment, not the code. |
|
|
| msg120672 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-11-07 12:01 |
| Here's a patch (against py3k) incorporating your suggestions. Would you be willing to review? [Removing Python 2.6 from versions since it's no longer maintained for non-security issues.) |
|
|
| msg120718 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-08 08:35 |
| Mark Dickinson writes: > Here's a patch (against py3k) incorporating your suggestions. Would you > be willing to review? Looks fine to me. (Actually the gcc branch makes the same assumptions as the final branch, but then I expect gcc itself does too.) |
|
|
| msg120720 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-08 09:11 |
| I wrote: >> BTW, do you know of any modern non-Windows platforms that don't define >> LLONG_MIN and LLONG_MAX? It may well be that the "two's complement" >> fallback hasn't been exercised in recent years. > > Anyting compiled with strict ANSI pre-C99 mode, e.g. gcc -ansi, (...) which also disables 'long long', so such examples are moot. duh. |
|
|
| msg120744 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-08 14:30 |
| Hallvard B Furuseth writes: > Looks fine to me. Hold on.. #elif defined SIZEOF_LONG_LONG would be a bit safer than #else. |
|
|
| msg120844 - (view) |
Author: Hallvard B Furuseth (hfuru) |
Date: 2010-11-09 08:40 |
| No, PY_LLONG_MAX lacks a paren. Found by the revolutionary method of actually testing it (made the previous branches #if 0's). bugs.python.org is not responding, but here's what I'm using now: Index: Include/pyport.h =================================================================== --- Include/pyport.h (revision 86319) +++ Include/pyport.h (working copy) @@ -62,15 +62,20 @@ #define PY_LLONG_MAX LLONG_MAX #define PY_ULLONG_MAX ULLONG_MAX #elif defined(__LONG_LONG_MAX__) -/* Otherwise, if GCC has a builtin define, use that. */ +/* Otherwise, if GCC has a builtin define, use that. (Definition of + * PY_LLONG_MIN assumes two's complement with no trap representation.) */ #define PY_LLONG_MAX __LONG_LONG_MAX__ -#define PY_LLONG_MIN (-PY_LLONG_MAX-1) -#define PY_ULLONG_MAX (__LONG_LONG_MAX__*2ULL + 1ULL) -#else -/* Otherwise, rely on two's complement. */ -#define PY_ULLONG_MAX (~0ULL) -#define PY_LLONG_MAX ((long long)(PY_ULLONG_MAX>>1)) -#define PY_LLONG_MIN (-PY_LLONG_MAX-1) +#define PY_LLONG_MIN (-PY_LLONG_MAX - 1) +#define PY_ULLONG_MAX (PY_LLONG_MAX * Py_ULL(2) + 1) +#elif defined(SIZEOF_LONG_LONG) +/* Otherwise compute from SIZEOF_LONG_LONG, assuming two's complement, no + padding bits, and no trap representation. Note: PY_ULLONG_MAX was + previously #defined as (~0ULL) here; but that'll give the wrong value in a + preprocessor expression on systems where long long != intmax_t. */ +#define PY_LLONG_MAX \ + (1 + 2 * ((Py_LL(1) << (CHAR_BIT * SIZEOF_LONG_LONG - 2)) - 1)) +#define PY_LLONG_MIN (-PY_LLONG_MAX - 1) +#define PY_ULLONG_MAX (PY_LLONG_MAX * Py_ULL(2) + 1) #endif /* LLONG_MAX */ #endif #endif /* HAVE_LONG_LONG */ |
|
|
| msg121604 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-11-20 10:43 |
| Fixed in r86552. Thanks! |
|
|