modulefinder should reuse the dis module

[vstinner]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.