classification
Title: modulefinder should reuse the dis module
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Demur Rumed, jvr, meador.inge, python-dev, serhiy.storchaka, theller, vstinner
Priority: normal Keywords: patch

Created on 2016-04-29 06:50 by vstinner, last changed 2016-05-11 19:30 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
dis_unpack_args.patch serhiy.storchaka, 2016-04-29 07:43 review
dis_unpack_args_2.patch serhiy.storchaka, 2016-05-04 07:43 review
dis_unpack_args_3.patch serhiy.storchaka, 2016-05-05 11:53 review
Messages (11)
msg264468 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-29 06:50
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 #26647 "Wordcode" changes the bytecode.
msg264471 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-29 07:43
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.
msg264645 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-02 12:50
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 issue26647 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.
msg264787 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-04 07:43
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?
msg264870 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2016-05-05 01:50
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.
msg264905 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-05 11:53
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).
msg265168 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-08 21:02
New changeset 3579cdaf56d9 by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #26881: modulefinder now works with bytecode with extended args.
https://hg.python.org/cpython/rev/f06baed1bb0c
msg265332 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-05-11 17:58
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.
msg265338 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-11 19:21
New changeset f0a4d563b32e by Serhiy Storchaka in branch '3.5':
Issue #26881: Restored the name of scan_opcodes_25().
https://hg.python.org/cpython/rev/f0a4d563b32e
msg265341 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-11 19:26
New changeset d60040b3bb8c by Serhiy Storchaka in branch '3.5':
Removed duplicated NEWS entity for issue #26881.
https://hg.python.org/cpython/rev/d60040b3bb8c
msg265342 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-11 19:30
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.
History
Date User Action Args
2016-06-05 15:30:34serhiy.storchakalinkissue26448 superseder
2016-05-11 19:30:44serhiy.storchakasetmessages: + msg265342
2016-05-11 19:26:04python-devsetmessages: + msg265341
2016-05-11 19:21:37python-devsetmessages: + msg265338
2016-05-11 17:58:13vstinnersetmessages: + msg265332
2016-05-08 21:03:49serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-05-08 21:02:33python-devsetnosy: + python-dev
messages: + msg265168
2016-05-05 11:53:43serhiy.storchakasetfiles: + dis_unpack_args_3.patch

messages: + msg264905
2016-05-05 01:50:21meador.ingesetnosy: + meador.inge
messages: + msg264870
2016-05-04 07:43:50serhiy.storchakasetfiles: + dis_unpack_args_2.patch
nosy: + theller, jvr
messages: + msg264787

2016-05-02 12:50:06serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg264645
versions: + Python 2.7, Python 3.5
2016-04-30 16:07:32serhiy.storchakalinkissue26647 dependencies
2016-04-29 07:43:34serhiy.storchakasetfiles: + dis_unpack_args.patch
messages: + msg264471

components: + Library (Lib)
keywords: + patch
type: behavior
stage: patch review
2016-04-29 06:50:49vstinnercreate