Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python/importlib.h is different on 32bit and 64bit #59671

Closed
amauryfa opened this issue Jul 27, 2012 · 17 comments
Closed

Python/importlib.h is different on 32bit and 64bit #59671

amauryfa opened this issue Jul 27, 2012 · 17 comments

Comments

@amauryfa
Copy link
Member

BPO 15466
Nosy @loewis, @brettcannon, @amauryfa, @mdickinson, @pitrou, @ericsnowcurrently, @serhiy-storchaka
Files
  • marshal.diff
  • marshal_use_int64.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2012-07-28.17:47:54.066>
    created_at = <Date 2012-07-27.12:50:58.598>
    labels = ['release-blocker']
    title = 'Python/importlib.h is different on 32bit and 64bit'
    updated_at = <Date 2012-07-28.19:57:33.794>
    user = 'https://github.com/amauryfa'

    bugs.python.org fields:

    activity = <Date 2012-07-28.19:57:33.794>
    actor = 'eric.snow'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-07-28.17:47:54.066>
    closer = 'loewis'
    components = []
    creation = <Date 2012-07-27.12:50:58.598>
    creator = 'amaury.forgeotdarc'
    dependencies = []
    files = ['26553', '26554']
    hgrepos = []
    issue_num = 15466
    keywords = ['patch']
    message_count = 17.0
    messages = ['166559', '166561', '166563', '166565', '166643', '166646', '166647', '166655', '166656', '166657', '166667', '166668', '166669', '166671', '166672', '166673', '166686']
    nosy_count = 8.0
    nosy_names = ['loewis', 'brett.cannon', 'amaury.forgeotdarc', 'mark.dickinson', 'pitrou', 'python-dev', 'eric.snow', 'serhiy.storchaka']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue15466'
    versions = ['Python 3.3']

    @amauryfa
    Copy link
    Member Author

    As shown in a patch in bpo-15431, 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.

    @amauryfa
    Copy link
    Member Author

    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).

    @pitrou
    Copy link
    Member

    pitrou commented Jul 27, 2012

    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.

    @amauryfa
    Copy link
    Member Author

    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.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 28, 2012

    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).

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 28, 2012

    Here is a patch to stop generating TYPE_INT64. Ok for 3.30b2?

    @loewis loewis mannequin added the release-blocker label Jul 28, 2012
    @amauryfa
    Copy link
    Member Author

    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...

    @serhiy-storchaka
    Copy link
    Member

    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.

    @amauryfa
    Copy link
    Member Author

    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)

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 28, 2012

    Serhiy: this is safe to ignore.

    @ericsnowcurrently
    Copy link
    Member

    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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 28, 2012

    New changeset 461e84fc8c60 by Martin v. Löwis in branch 'default':
    Issue bpo-15466: Stop using TYPE_INT64 in marshal,
    http://hg.python.org/cpython/rev/461e84fc8c60

    @amauryfa
    Copy link
    Member Author

    Removing TYPE_INT64 was indeed the most reasonable choice.

    @loewis loewis mannequin closed this as completed Jul 28, 2012
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 28, 2012

    I put the complete removal of TYPE_INT64 into bpo-15480

    @ericsnowcurrently
    Copy link
    Member

    Looks like the Misc/NEWS entry got truncated. Also, does this change need a new PYC magic number (now in Lib/importlib/_bootstrap.py)?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 28, 2012

    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.

    @ericsnowcurrently
    Copy link
    Member

    I don't see why a new pyc magic number should be necessary. Python
    continues to support the existing files.

    Good point.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants