This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Objects/longobject.c not compiled without long long
Type: compile error Stage: resolved
Components: Interpreter Core Versions: Python 3.2, Python 3.3, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: mark.dickinson, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-10-18 11:59 by serhiy.storchaka, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
long_fromvoidptr.patch serhiy.storchaka, 2012-10-18 11:59 review
long_fromvoidptr_2.patch serhiy.storchaka, 2012-10-18 19:21 review
long_fromvoidptr_3.patch serhiy.storchaka, 2012-10-18 19:21 review
Messages (14)
msg173257 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-18 11:59
Preprocessor directives emit error in PyLong_FromVoidPtr:

PyLong_FromVoidPtr: sizeof(void*) > sizeof(long), but no long long

if HAVE_LONG_LONG not defined.

Here is a patch which adds missing branch (lost somewhere in 2->3 translation).  Also removed non-needed optimization which can cause undefined behavior (C Standard not guarantee (long long)NULL == 0).
msg173266 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 12:56
First part of the patch looks fine to me.

For the second part, I don't see any undefined behaviour here.  Can you explain where the undefined behaviour comes from?  And are you sure that this is really just an optimization?  It looks as though it might be deliberately there to make sure that the conversion still produces a Python 0 even on systems where the NULL pointer *doesn't* give 0 when converted to an integer.
msg173267 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 13:22
Actually, I think that special case needs to be added to the first branch as well.
msg173268 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-18 13:51
> For the second part, I don't see any undefined behaviour here.  Can you
> explain where the undefined behaviour comes from?  And are you sure that
> this is really just an optimization?  It looks as though it might be
> deliberately there to make sure that the conversion still produces a
> Python 0 even on systems where the NULL pointer *doesn't* give 0 when
> converted to an integer.

If on some platform (uintptr_t)NULL != 0, then some other address can be 
reflected to 0. In shuch case PyLong_FromVoidPtr() returns zero integer for 
both NULL and this non-NULL addresses. Of course this is a hypothetic 
situation.

What about (Py_uintptr_t)p - (Py_uintptr_t)(void *)NULL? Then we should change 
PyLong_AsVoidPtr() too.
msg173270 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 14:03
> If on some platform (uintptr_t)NULL != 0, then some other address can be 
> reflected to 0.

This doesn't seem very likely, since then the C implementation wouldn't roundtrip when converting that other pointer to an integer and back to a pointer.  (C99 6.3.2.3 says that (void *)0 is a null pointer constant.)

The code you removed is not undefined behaviour, and is not just an optimization---removing it would change the semantics of PyLong_FromVoidPtr.  There may be code that depends on PyLong_FromVoidPtr(NULL) being 0.  I believe the code should stay.

> What about (Py_uintptr_t)p - (Py_uintptr_t)(void *)NULL?

What about it?  What's the relevance to this issue?
msg173271 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-18 14:38
> This doesn't seem very likely, since then the C implementation wouldn't
> roundtrip when converting that other pointer to an integer and back to a
> pointer.  (C99 6.3.2.3 says that (void *)0 is a null pointer constant.)

"0" lexeme is not 0 integer. In expression "(void*)0" "0" means null pointer. 
You can't do printf("%p", 0), because in this case compiler does not know what 
0 you have in mind.

> The code you removed is not undefined behaviour, and is not just an
> optimization---removing it would change the semantics of
> PyLong_FromVoidPtr.  There may be code that depends on
> PyLong_FromVoidPtr(NULL) being 0.  I believe the code should stay.

I think it was wrong implementation, even if only in theoretical 
considerations. I believe Python does not support platforms where it matters. 
The removed code creates the impression that the problem is solved, but in 
fact it is not. Yes, we can leave it, but it doesn't make sense.

> > What about (Py_uintptr_t)p - (Py_uintptr_t)(void *)NULL?
> What about it?  What's the relevance to this issue?

This provides the invariant PyLong_FromVoidPtr(NULL) == PyLong_FromLong(0) and 
does not cost anything on platforms where (Py_uintptr_t)(void *)NULL == 0.
msg173274 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 14:56
> "0" lexeme is not 0 integer.

Ah yes, true.  C99 6.3.2.3 specifies an "integer *constant* expression".  So I was mistaken in thinking that converting an arbitrary integer-valued expression with value 0 to (void*) must always give a NULL pointer.

> I think it was wrong implementation, even if only in theoretical 
> considerations.

Okay, but do you agree that (1) there's no undefined behaviour, and (2) removing this code has the potential to change results on platforms where converting NULL to an integer doesn't give zero?

If you agree with those two things, then this is a behaviour change, not a bugfix, and you should open a new issue aimed at 3.4 for that.

> I believe Python does not support platforms where it matters.

What's your justification for this belief?

> This provides the invariant PyLong_FromVoidPtr(NULL) == PyLong_FromLong(0) and 
does not cost anything on platforms where (Py_uintptr_t)(void *)NULL == 0.

Sorry, but this is making no sense to me.  You wrote: "(Py_uintptr_t)p - (Py_uintptr_t)(void *)NULL".  Is that supposed to be existing code that you're proposing changing?  An example to prove some point (what point?)  Code you're proposing to add somewhere?  (If so, where are you proposing to add it?)  Why are you subtracting these two pointers?  What's p?  I'm finding it hard to make sense of what you're writing.
msg173277 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-18 15:24
> Okay, but do you agree that (1) there's no undefined behaviour, and (2)
> removing this code has the potential to change results on platforms where
> converting NULL to an integer doesn't give zero?

Undefined behavior (or may be just the wrong behavior), we obtain later, when 
converting in PyLong_AsVoidPtr() zero integer back to pointer. On platforms 
where converting NULL to an integer doesn't give zero it's a bug.

> > I believe Python does not support platforms where it matters.
> What's your justification for this belief?

Can you show the contrexample? I can not. If such a platform existed, perhaps 
this bug has been already shown it.

> Sorry, but this is making no sense to me.  You wrote: "(Py_uintptr_t)p -
> (Py_uintptr_t)(void *)NULL".  Is that supposed to be existing code that
> you're proposing changing?  An example to prove some point (what point?) 
> Code you're proposing to add somewhere?  (If so, where are you proposing
> to add it?)  Why are you subtracting these two pointers?  What's p?  I'm
> finding it hard to make sense of what you're writing.

I mean using PyLong_FromUnsignedLongLong((unsigned PY_LONG_LONG)
((Py_uintptr_t)p - (Py_uintptr_t)(void *)NULL)) instead 
PyLong_FromUnsignedLongLong((unsigned PY_LONG_LONG)(Py_uintptr_t)p). Of course 
PyLong_AsVoidPtr() should be changed to return (void *)(x + (Py_uintptr_t)
(void *)NULL) instead (void *)x.
msg173286 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 18:52
> Undefined behavior (or may be just the wrong behavior), we obtain later, > when 
> converting in PyLong_AsVoidPtr() zero integer back to pointer. On
> platforms 
> where converting NULL to an integer doesn't give zero it's a bug.

So maybe the fix should be to special case zero in PyLong_AsVoidPtr, and turn 0L back into NULL there?

I think it's just too risky to change the current behaviour in 3.2 and 3.3, given the number of places that PyLong_FromVoidPtr is used.  Unlike you, I don't have confidence that there are no current or future platforms that don't map NULL to 0.  It might be reasonable to consider a change for 3.4.

> I mean using PyLong_FromUnsignedLongLong((unsigned PY_LONG_LONG)
> ((Py_uintptr_t)p - (Py_uintptr_t)(void *)NULL)) instead ...

Okay, thanks for explaining.
msg173287 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-10-18 18:54
New changeset bedb2903d71e by Mark Dickinson in branch '3.2':
Issue #16277: in PyLong_FromVoidPtr, add missing branch for sizeof(void*) <= sizeof(long).
http://hg.python.org/cpython/rev/bedb2903d71e

New changeset 8ce04be1321c by Mark Dickinson in branch '3.3':
Issue #16277: merge fix from 3.2
http://hg.python.org/cpython/rev/8ce04be1321c

New changeset 02e57069f43e by Mark Dickinson in branch 'default':
Issue #16277: merge fix from 3.3
http://hg.python.org/cpython/rev/02e57069f43e
msg173288 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 18:57
I've fixed the missing branch in 3.2, 3.3 and 3.4.
msg173289 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-18 19:21
> So maybe the fix should be to special case zero in PyLong_AsVoidPtr, and
> turn 0L back into NULL there?

Yes, of course. If we have a special case in PyLong_FromVoidPtr(), it is wrong 
that we do not have the special case in PyLong_AsVoidPtr(). But this will 
change the current (potentially buggy) behaviour. ;-)

The patch long_fromvoidptr_2.patch contains such changes (as you want).

But this changes are buggy too, because now PyLong_FromVoidPtr() and 
PyLong_AsVoidPtr() are multivalued. PyLong_FromVoidPtr() maps to 0 NULL and 
may be yet another pointer, PyLong_AsVoidPtr() maps to NULL 0 and may be yet 
another integer.

The patch long_fromvoidptr_3.patch is more consistent in this sense. Now both 
functions are univalued, 

> I think it's just too risky to change the current behaviour in 3.2 and 3.3,
> given the number of places that PyLong_FromVoidPtr is used.  

Buggy behaviour changed if we fix the bug.

> Unlike you, I
> don't have confidence that there are no current or future platforms that
> don't map NULL to 0.

I just want to say that I'm not sure whether to fix this bug. But if we fix it 
for purity, it should be fixed right.
msg173294 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-18 20:22
Let's keep it simple and just drop the special case for 3.4.
msg173296 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-18 20:40
I close this issue as the main bug was fixed.

For incidental bug I open issue16280.
History
Date User Action Args
2022-04-11 14:57:37adminsetgithub: 60481
2012-10-18 20:40:08serhiy.storchakasetstatus: open -> closed
versions: + Python 3.2, Python 3.3
messages: + msg173296

resolution: fixed
stage: resolved
2012-10-18 20:22:29mark.dickinsonsetmessages: + msg173294
2012-10-18 19:21:20serhiy.storchakasetfiles: + long_fromvoidptr_2.patch, long_fromvoidptr_3.patch

messages: + msg173289
2012-10-18 18:57:07mark.dickinsonsetassignee: mark.dickinson
messages: + msg173288
versions: - Python 3.2, Python 3.3
2012-10-18 18:54:14python-devsetnosy: + python-dev
messages: + msg173287
2012-10-18 18:52:31mark.dickinsonsetmessages: + msg173286
2012-10-18 15:24:09serhiy.storchakasetmessages: + msg173277
2012-10-18 14:56:33mark.dickinsonsetmessages: + msg173274
2012-10-18 14:38:32serhiy.storchakasetmessages: + msg173271
2012-10-18 14:03:33mark.dickinsonsetmessages: + msg173270
2012-10-18 13:51:37serhiy.storchakasetmessages: + msg173268
2012-10-18 13:22:54mark.dickinsonsetmessages: + msg173267
2012-10-18 12:56:31mark.dickinsonsetnosy: + mark.dickinson
messages: + msg173266
2012-10-18 11:59:50serhiy.storchakacreate