Repository navigation
modulefinder should reuse the dis module #71068
Description
| BPO | 26881 |
|---|---|
| Nosy | @theller, @vstinner, @meadori, @serhiy-storchaka, @serprex |
| Files |
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']
Activity
vstinner commentedon Apr 29, 2016
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 commentedon Apr 29, 2016
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 commentedon May 2, 2016
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 commentedon May 4, 2016
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 commentedon 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 commentedon May 5, 2016
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 commentedon 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 commentedon May 11, 2016
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 commentedon 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 commentedon 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 commentedon May 11, 2016
Thank you Victor. Fixed.
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.