classification
Title: Python/importlib.h is different on 32bit and 64bit
Type: Stage:
Components: Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, brett.cannon, eric.snow, loewis, mark.dickinson, pitrou, python-dev, serhiy.storchaka
Priority: release blocker Keywords: patch

Created on 2012-07-27 12:50 by amaury.forgeotdarc, last changed 2012-07-28 19:57 by eric.snow. This issue is now closed.

Files
File name Uploaded Description Edit
marshal.diff loewis, 2012-07-28 10:47 review
marshal_use_int64.patch amaury.forgeotdarc, 2012-07-28 11:09 review
Messages (17)
msg166559 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-07-27 12:50
As shown in a patch in issue15431, frozen.c does not output the same data on different platforms.

The first difference looks like this (extracted from the patch):
-    101,73,255,255,255,255,0,0,0,0,40,10,0,0,0,117,
+    101,108,3,0,0,0,255,127,255,127,3,0,40,10,0,0,

On first row, 'I' followed by 0xFFFFFFFF on 8 bytes.
On second row, 'l' followed by 3 followed by 0xFFFFFFFF (in 3 chunks of 15 bits).
The Python number 0xFFFFFFFF is marshalled with TYPE_INT64 when SIZEOF_LONG>4 (Unix 64bit), and with TYPE_LONG on other platforms (32bit, or win64)

The C "long" type has much less importance in 3.x Python, because PyIntObject does not exist anymore.
I suggest to remove this distinction, and allow TYPE_INT64 on all platforms.

I don't see any compatibility issue, on unmarshalling both methods produce the same object.
I'll try to suggest a patch later today.
msg166561 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-07-27 12:53
I realize that uppercase I and lowercase L are easily confused:

On first row, 'I' (=TYPE_INT64) followed by 0xFFFFFFFF on 8 bytes.
On second row, 'l' (=TYPE_LONG) followed by 3 followed by 0xFFFFFFFF (in 3 chunks of 15 bits).
msg166563 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-27 13:21
> I suggest to remove this distinction, and allow TYPE_INT64 on all
> platforms.

You have to find a 64-bit C int type that works on all platforms and for which a PyLongAsXXX() function exists, though. Or perhaps you want to restrict yourself to systems which have "long long" and where it is at least 64 bits wide.
msg166565 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-07-27 13:45
My primary goal is to have a stable importlib.h, and currently all developer work on machines where PY_LONG_LONG is >64bit.  So the restriction is OK.
msg166643 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-07-28 10:16
I suggest the opposite: never emit TYPE_INT64 at all. For compatibility, we can still support reading it in 3.3 (and drop that support in 3.4).
msg166646 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-07-28 10:47
Here is a patch to stop generating TYPE_INT64. Ok for 3.30b2?
msg166647 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-07-28 11:09
Here is a patch that write TYPE_INT64 on most platforms (where either long or long long is 64bit).
It is admittedly much larger than Martin's...
msg166655 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-28 13:22
I can see yet another problem under the hood. Marshalling will output different data on platforms with a different representation of negative numbers. Worse still, these data are non-portable, written on one platform will be read incorrectly on the other. Mark, time for your inquisitor sword.
msg166656 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-07-28 13:38
Are there really platforms which don't use two's complement?
(If there are, I'm not sure to want to share binary data with them. Same for EBCDIC)
msg166657 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-07-28 13:39
Serhiy: this is safe to ignore.
msg166667 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-07-28 17:25
> I suggest the opposite: never emit TYPE_INT64 at all. For
> compatibility, we can still support reading it in 3.3 (and drop
> that support in 3.4).

+1  essentially we maintain the status quo
msg166668 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-28 17:44
New changeset 461e84fc8c60 by Martin v. Löwis in branch 'default':
Issue #15466: Stop using TYPE_INT64 in marshal,
http://hg.python.org/cpython/rev/461e84fc8c60
msg166669 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-07-28 17:46
Removing TYPE_INT64 was indeed the most reasonable choice.
msg166671 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-07-28 17:49
I put the complete removal of TYPE_INT64 into issue15480
msg166672 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-07-28 17:54
Looks like the Misc/NEWS entry got truncated.  Also, does this change need a new PYC magic number (now in Lib/importlib/_bootstrap.py)?
msg166673 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-07-28 18:28
For the truncation, see ec7267fa8b84. I don't see why a new pyc magic number should be necessary. Python continues to support the existing files.
msg166686 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-07-28 19:57
> I don't see why a new pyc magic number should be necessary. Python
> continues to support the existing files.

Good point.
History
Date User Action Args
2012-07-28 19:57:33eric.snowsetmessages: + msg166686
2012-07-28 18:28:07loewissetmessages: + msg166673
2012-07-28 17:54:41eric.snowsetmessages: + msg166672
2012-07-28 17:49:54loewissetmessages: + msg166671
2012-07-28 17:47:54loewissetstatus: open -> closed
resolution: fixed
2012-07-28 17:46:27amaury.forgeotdarcsetmessages: + msg166669
2012-07-28 17:45:24eric.snowsetversions: + Python 3.3
2012-07-28 17:44:23python-devsetnosy: + python-dev
messages: + msg166668
2012-07-28 17:25:55eric.snowsetnosy: + eric.snow
messages: + msg166667
2012-07-28 13:39:38loewissetmessages: + msg166657
2012-07-28 13:38:42amaury.forgeotdarcsetmessages: + msg166656
2012-07-28 13:25:02serhiy.storchakasetnosy: + mark.dickinson
2012-07-28 13:22:22serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg166655
2012-07-28 11:09:45amaury.forgeotdarcsetfiles: + marshal_use_int64.patch

messages: + msg166647
2012-07-28 10:47:27loewissetpriority: high -> release blocker
files: + marshal.diff
messages: + msg166646

keywords: + patch
2012-07-28 10:16:02loewissetnosy: + loewis
messages: + msg166643
2012-07-27 17:41:39pitrouunlinkissue15431 dependencies
2012-07-27 14:59:58brett.cannonsetnosy: + brett.cannon
2012-07-27 13:45:40amaury.forgeotdarcsetmessages: + msg166565
2012-07-27 13:21:30pitrousetmessages: + msg166563
2012-07-27 12:53:52amaury.forgeotdarcsetmessages: + msg166561
2012-07-27 12:51:36amaury.forgeotdarclinkissue15431 dependencies
2012-07-27 12:50:58amaury.forgeotdarccreate