classification
Title: PY_LLONG_MAX & co - preprocessor constants or not?
Type: compile error Stage: resolved
Components: Interpreter Core Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: hfuru, mark.dickinson
Priority: normal Keywords: patch

Created on 2010-11-05 13:56 by hfuru, last changed 2010-11-20 10:43 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
issue10325.patch mark.dickinson, 2010-11-07 12:02 review
Messages (10)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-11-20 10:43
Fixed in r86552.  Thanks!
History
Date User Action Args
2010-11-20 10:43:58mark.dickinsonsetstatus: open -> closed
messages: + msg121604

assignee: mark.dickinson
resolution: fixed
stage: resolved
2010-11-09 08:40:13hfurusetmessages: + msg120844
2010-11-08 14:30:10hfurusetmessages: + msg120744
2010-11-08 09:11:29hfurusetmessages: + msg120720
2010-11-08 08:35:35hfurusetmessages: + msg120718
2010-11-07 12:03:00mark.dickinsonsetfiles: - issue10325.patch
2010-11-07 12:02:39mark.dickinsonsetfiles: + issue10325.patch
2010-11-07 12:01:13mark.dickinsonsetfiles: + issue10325.patch
keywords: + patch
messages: + msg120672

versions: - Python 2.6
2010-11-06 19:06:25hfurusetmessages: + msg120648
2010-11-06 18:43:11hfurusetmessages: + msg120646
2010-11-05 17:47:13mark.dickinsonsetmessages: + msg120517
2010-11-05 14:22:20r.david.murraysetnosy: + mark.dickinson
2010-11-05 13:56:11hfurucreate