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

Jump to bottom

modulefinder should reuse the dis module #71068

Closed
vstinner opened this issue Apr 29, 2016 · 11 comments
Closed

modulefinder should reuse the dis module #71068

vstinner opened this issue Apr 29, 2016 · 11 comments
Assignees
@serhiy-storchaka
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

BPO 26881
Nosy @theller, @vstinner, @meadori, @serhiy-storchaka, @serprex
Files
  • dis_unpack_args.patch
  • dis_unpack_args_2.patch
  • dis_unpack_args_3.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/serhiy-storchaka'
closed_at = <Date 2016-05-08.21:03:49.199>
created_at = <Date 2016-04-29.06:50:49.318>
labels = ['type-bug', 'library']
title = 'modulefinder should reuse the dis module'
updated_at = <Date 2016-05-11.19:30:44.035>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-05-11.19:30:44.035>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-05-08.21:03:49.199>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2016-04-29.06:50:49.318>
creator = 'vstinner'
dependencies = []
files = ['42645', '42707', '42735']
hgrepos = []
issue_num = 26881
keywords = ['patch']
message_count = 11.0
messages = ['264468', '264471', '264645', '264787', '264870', '264905', '265168', '265332', '265338', '265341', '265342']
nosy_count = 7.0
nosy_names = ['theller', 'jvr', 'vstinner', 'meador.inge', 'python-dev', 'serhiy.storchaka', 'Demur Rumed']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26881'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']
    @vstinner
    Copy link
    Member Author

    The scan_opcodes_25() method of the modulefinder module implements a disassembler of Python bytecode. The implementation is incomplete, it doesn't support EXTENDED_ARG. I suggest to drop the disassembler and reuse the dis module.

    See also the issue bpo-26647 "Wordcode" changes the bytecode.

    @serhiy-storchaka
    Copy link
    Member

    Proposed patch adds private helper function dis._unpack_args() that unpacks long arguments. This function is now reused in two places in dis and in modulefinder.

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 29, 2016
    @serhiy-storchaka
    Copy link
    Member

    This is pretty simple patch and I'm going to push it in short time if there are no objections. This can make the patch for bpo-26647 simpler since handling extended args is isolated in one function.

    Alternative names for _unpack_args() are welcome.

    An alternative way is to use public function get_instructions(), but this adds more overhead and don't work for 2.7.

    @serhiy-storchaka serhiy-storchaka self-assigned this May 2, 2016
    @serhiy-storchaka
    Copy link
    Member

    modulefinder in 2.7 should be compatible with Python 2.2 (see PEP-291). Thus we can't just add new function in dis and reuse it in modulefinder. We should duplicate this function in modulefinder.

    I don't know if there is corresponding rule for 3.x. Should modulefinder keep compatibility with say 3.1?

    @meadori
    Copy link
    Member

    meadori commented May 5, 2016

    Overall, this patch LGTM. Some new tests would be nice. Especially since the NEWS entry claims that we now support extended args.

    As for for the compatibility concerns, I don't know.

    @serhiy-storchaka
    Copy link
    Member

    Added test.

    But this test is fragile. Clever optimizer can made it passing even with modulefinder not supporting extended args (I'm going to provide such optimization).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 8, 2016

    New changeset 3579cdaf56d9 by Serhiy Storchaka in branch '3.5':
    Issue bpo-26881: The modulefinder module now supports extended opcode arguments.
    https://hg.python.org/cpython/rev/3579cdaf56d9

    New changeset c27e3773d0f9 by Serhiy Storchaka in branch 'default':
    Issue bpo-26881: The modulefinder module now supports extended opcode arguments.
    https://hg.python.org/cpython/rev/c27e3773d0f9

    New changeset f06baed1bb0c by Serhiy Storchaka in branch '2.7':
    Issue bpo-26881: modulefinder now works with bytecode with extended args.
    https://hg.python.org/cpython/rev/f06baed1bb0c

    @vstinner
    Copy link
    Member Author

    It looks like the change is mentionned twice in Misc/NEWS.

    IMHO it's worth to add an optional argument to skip extended opcodes in
    _unpack_opcodes() to simplify the code in modulefinder.

    Is the scancode method public? If yes, I'm not sure that it's ok to rename
    it in a minor release.

    Thanks for the change, it prepares the work for wordcode.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 11, 2016

    New changeset f0a4d563b32e by Serhiy Storchaka in branch '3.5':
    Issue bpo-26881: Restored the name of scan_opcodes_25().
    https://hg.python.org/cpython/rev/f0a4d563b32e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 11, 2016

    New changeset d60040b3bb8c by Serhiy Storchaka in branch '3.5':
    Removed duplicated NEWS entity for issue bpo-26881.
    https://hg.python.org/cpython/rev/d60040b3bb8c

    @serhiy-storchaka
    Copy link
    Member

    Thank you Victor. Fixed.

    IMHO it's worth to add an optional argument to skip extended opcodes in
    _unpack_opcodes() to simplify the code in modulefinder.

    But this would complicate the code in dis.

    If the disassembler would changed to skip extended opcodes, we would add this option in _unpack_opargs().

    If there would be some Python 3 analogue of PEP-291, modulefinder would be changed to use public dis API.

    @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

    @serhiy-storchaka serhiy-storchaka

    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @vstinner @meadori @serhiy-storchaka