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.

Sign up for GitHub

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

Jump to bottom

Alternative fix for BUILD_MAP_UNPACK_WITH_CALL opcode in 3.5 #73723

Closed
serhiy-storchaka opened this issue Feb 12, 2017 · 10 comments
Closed

Alternative fix for BUILD_MAP_UNPACK_WITH_CALL opcode in 3.5 #73723

serhiy-storchaka opened this issue Feb 12, 2017 · 10 comments
Assignees
@ncoghlan
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 29537
Nosy @warsaw, @mdickinson, @ncoghlan, @larryhastings, @encukou, @serhiy-storchaka, @irushchyshyn
PRs
  • [3.5] bpo-29537: Tolerate legacy invalid bytecode #169
  • bpo-29537: Also mention 3.5.2 in NEWS entry #558
    		•
    Files
  • BUILD_MAP_UNPACK_WITH_CALL-no-function_location.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 = 'https://github.com/ncoghlan'
closed_at = <Date 2017-03-08.06:44:44.813>
created_at = <Date 2017-02-12.14:28:54.772>
labels = ['interpreter-core', 'type-bug']
title = 'Alternative fix for BUILD_MAP_UNPACK_WITH_CALL opcode in 3.5'
updated_at = <Date 2017-03-24.22:42:19.360>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-03-24.22:42:19.360>
actor = 'ncoghlan'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2017-03-08.06:44:44.813>
closer = 'ncoghlan'
components = ['Interpreter Core']
creation = <Date 2017-02-12.14:28:54.772>
creator = 'serhiy.storchaka'
dependencies = []
files = ['46634']
hgrepos = []
issue_num = 29537
keywords = ['patch']
message_count = 10.0
messages = ['287637', '287649', '288116', '288506', '288508', '289170', '289200', '289206', '290264', '290271']
nosy_count = 7.0
nosy_names = ['barry', 'mark.dickinson', 'ncoghlan', 'larry', 'petr.viktorin', 'serhiy.storchaka', 'ishcherb']
pr_nums = ['169', '558']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29537'
versions = ['Python 3.5']
    @serhiy-storchaka
    Copy link
    Member Author

    Proposed patch is an alternative fix of bpo-27286 for Python 3.5. Rather than fixing the compiler and bumping the magic number it just avoids using incorrect value in eval loop. The patch can be useful for disro maintainers that don't want to recompile all .pyc files when update Python to 3.5.3. I think it would be the best way to fix bpo-27286. Now it can be combined with either reverting bpo-27286 changes or applying the workaround encukou@magic-number-workaround .

    The patch makes the high byte of oparg be used only when it is unambiguous (equal to 1). In that case the error message contains a function name. Otherwise the error message doesn't contain a function name. This is small usability regression, but the BUILD_MAP_UNPACK_WITH_CALL opcode is very rarely used and shouldn't raise an exception in normal case.

    @serhiy-storchaka serhiy-storchaka added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 12, 2017
    @ncoghlan
    Copy link
    Contributor

    Thanks Serhiy, I think that will work well downstream in combination with Petr's patch to accept both magic numbers.

    @ncoghlan
    Copy link
    Contributor

    In putting together a PR for this, I think it *only* makes sense if we also push Petr's change upstream to accept the legacy bytecode files in 3.5.4+.

    The reason is that the patch actually causes a test case to fail due to "f()" change to "function" in an error message.

    @ncoghlan ncoghlan self-assigned this Feb 24, 2017
    @ncoghlan ncoghlan added the type-bug An unexpected behavior, bug, or error label Feb 24, 2017
    @ncoghlan
    Copy link
    Contributor

    For easier cross-referencing, note that http://bugs.python.org/issue29514 is the issue proposing a test case that ensures future maintainers are aware of the practical problems created by bumping the bytecode magic number in a maintenance release.

    @ncoghlan
    Copy link
    Contributor

    I updated the PR to cover both Serhiy's change to make the legacy bytecode safe to interpret, and a tweaked version of Petr's change to allow the legacy bytecode to be imported.

    The main tweaks to the latter were:

    • more in depth comments explaining the changes
    • switching the C level changes to rely on a couple of extern variables exported from import.c rather than scattering the backwards compatibility numbers throughout the code

    I'm also cc'ing Larry into the discussion as 3.5 release manager. Larry, most of the context for this change is actually in http://bugs.python.org/issue29514 where we reported the problems with the magic number change that prompted Petr to create a downstream patch for Fedora to restore compatibility with the legacy bytecode magic number.

    While the answer for 3.6+ is "Let's try to avoid ever doing this again", it turns out this is tricky enough to handle that it would be nice to have a shared solution upstream in 3.5.4+ that redistributors can collectively adopt, rather than everyone needing to come up with their own workaround for the problem.

    @irushchyshyn
    Copy link
    Mannequin

    irushchyshyn mannequin commented Mar 7, 2017

    If I may ask, what was the decision on this matter?

    We are planning to rebase Python 3.5 for Fedora and this currently blocks us, if we do not work this around with a patch.
    Let me know if there is anything I can help with to speed up the process.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Mar 8, 2017

    I just realised I need to add some test cases to test_importlib/source/test_file_loader.py before merging it, but since Larry hasn't objected to the proposed approach, I'm going to go ahead and implement this fix.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Mar 8, 2017

    Merged (with test cases) in 93602e3

    The test cases even cover ensuring the backwards compatibility also applies to frozen bytecode :)

    @ncoghlan ncoghlan closed this as completed Mar 8, 2017
    @ncoghlan
    Copy link
    Contributor

    New changeset 0410bee by Nick Coghlan in branch '3.5':
    bpo-29537: Also cover 3.5.2 in NEWS entry
    0410bee

    @ncoghlan
    Copy link
    Contributor

    New changeset 93602e3 by Nick Coghlan in branch '3.5':
    [3.5] bpo-29537: Tolerate legacy invalid bytecode (#169)
    93602e3

    @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
    Assignees

    @ncoghlan ncoghlan

    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @ncoghlan @serhiy-storchaka