msg173257 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) |
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) * |
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) * |
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) * |
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) * |
Date: 2012-10-18 20:40 |
I close this issue as the main bug was fixed.
For incidental bug I open issue16280.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:37 | admin | set | github: 60481 |
2012-10-18 20:40:08 | serhiy.storchaka | set | status: open -> closed versions:
+ Python 3.2, Python 3.3 messages:
+ msg173296
resolution: fixed stage: resolved |
2012-10-18 20:22:29 | mark.dickinson | set | messages:
+ msg173294 |
2012-10-18 19:21:20 | serhiy.storchaka | set | files:
+ long_fromvoidptr_2.patch, long_fromvoidptr_3.patch
messages:
+ msg173289 |
2012-10-18 18:57:07 | mark.dickinson | set | assignee: mark.dickinson messages:
+ msg173288 versions:
- Python 3.2, Python 3.3 |
2012-10-18 18:54:14 | python-dev | set | nosy:
+ python-dev messages:
+ msg173287
|
2012-10-18 18:52:31 | mark.dickinson | set | messages:
+ msg173286 |
2012-10-18 15:24:09 | serhiy.storchaka | set | messages:
+ msg173277 |
2012-10-18 14:56:33 | mark.dickinson | set | messages:
+ msg173274 |
2012-10-18 14:38:32 | serhiy.storchaka | set | messages:
+ msg173271 |
2012-10-18 14:03:33 | mark.dickinson | set | messages:
+ msg173270 |
2012-10-18 13:51:37 | serhiy.storchaka | set | messages:
+ msg173268 |
2012-10-18 13:22:54 | mark.dickinson | set | messages:
+ msg173267 |
2012-10-18 12:56:31 | mark.dickinson | set | nosy:
+ mark.dickinson messages:
+ msg173266
|
2012-10-18 11:59:50 | serhiy.storchaka | create | |