dis.findlabels ignores EXTENDED_ARG

[efahl]

BPO	26448
Nosy	@ncoghlan, @vstinner, @serhiy-storchaka, @1st1, @llllllllll, @efahl
Superseder	bpo-26881: modulefinder should reuse the dis module
Files	preliminarypatch.diff wrongtestfindlabels.py: Mistake in this one testfindlabels2.py testfindlabels.py: Single test case that iterates over all opcodes testfindlabels.py: Generalized algorithm for creating bytecode from args bad.diff: Had an obvious bug. dis_with_code_scanner.diff

^{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 = None
closed_at = <Date 2016-06-05.15:30:34.328>
created_at = <Date 2016-02-27.03:54:10.478>
labels = ['type-bug', 'library']
title = 'dis.findlabels ignores EXTENDED_ARG'
updated_at = <Date 2016-06-05.21:15:47.756>
user = 'https://github.com/efahl'

bugs.python.org fields:

activity = <Date 2016-06-05.21:15:47.756>
actor = 'eric.fahlgren'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-05.15:30:34.328>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2016-02-27.03:54:10.478>
creator = 'eric.fahlgren'
dependencies = []
files = ['42035', '42037', '42038', '42039', '42046', '42047', '42065']
hgrepos = []
issue_num = 26448
keywords = ['patch']
message_count = 24.0
messages = ['260913', '260914', '260915', '260916', '260917', '260918', '260919', '260920', '260921', '260925', '260929', '260931', '260939', '260940', '260977', '260979', '260981', '260987', '260988', '261112', '261142', '261590', '267419', '267477']
nosy_count = 7.0
nosy_names = ['ncoghlan', 'vstinner', 'serhiy.storchaka', 'yselivanov', 'llllllllll', 'eric.fahlgren', 'Barun Parruck']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = '26881'
type = 'behavior'
url = 'https://bugs.python.org/issue26448'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[efahl]

When trying out dis.dis on some synthetically long functions, I noted that spurious branch targets were being generated in the output. First one is at address 8:

157 0 LOAD_CONST 1 (1)
3 DUP_TOP
4 STORE_FAST 0 (z)
7 DUP_TOP
>> 8 STORE_FAST 1 (a)
11 DUP_TOP

I dug into findlabels and notices that it pays no attention to EXTENDED_ARG. The fix is pretty simple, basically copy pasta from dis._get_instructions_bytes, at line 369, in the 3.5.1 release code add all the "extended_arg" bits:

    extended_arg = 0
    while i < n:
        op = code[i]
        i = i+1
        if op >= HAVE_ARGUMENT:
            arg = code[i] + code[i+1]*256 + extended_arg
            extended_arg = 0
            i = i+2
            if op == EXTENDED_ARG:
                 extended_arg = arg*65536
            label = -1

[BarunParruck]

I'll take a look at it! :) Would you like unit tests as well?

[BarunParruck]

Hi, to check if I've done things right, which functions did you try out dis.dis on?

[efahl]

My test case:

def long():
    z = a = b = c = d = e = f = g = h = 1
    while x:
        x = x if x and x or not x else x
        above line repeated 2999 more times

import dis
print(dis.findlabels(long.__code__.co_code)[:10])

Buggy output:
[35510, 35509, 62, 69, 78, 81, 93, 100, 109, 112]

Correct output:
[101046, 101045, 62, 69, 78, 81, 93, 100, 109, 112]

[efahl]

Our paths crossed, I don't know exactly how you'd add a test case for this, maybe construct the monster function in a string, eval the string, the use the synthesized function in dis.findlabels?

[efahl]

Well, now that I'm thinking about it, you could synthesize a bytecode stream trivially and have a much better test. This is completely off the top of my head, so take it is guaranteed to (probably) not work as written, but it should get you started:

from opcodes import *
import dis
bytecode = (
    chr(EXTENDED_ARG) + chr(1) + chr(0) + 
    chr(JUMP_IF_TRUE_OR_POP) + chr(0) + chr(0)
)
print(dis.findlabels(bytecode))

[BarunParruck]

Allright, so I'm uploading a preliminary patch, please look through it to see if I understood what you meant me to do about dis.findlabels.

The tests seem to mostly pass, except oddly enough, urlstdlib2, which is probably system-specific?

I get the correct output that you specify for that particular test function.

I'll see about building a unit test.

[BarunParruck]

The patch I forgot to attach. Ha ha.

[BarunParruck]

Hi, I'm a little confused as to how to write a test using bytecode streams...probably due to my lack of clarity as to what exactly dis.disassemble does. Is there any way you could give me a bit more information? :)

[efahl]

The findlabels function takes a bytecode array of type bytes, usually the
actual code from a function. My original test case uses the full Python
compiler from source code to a CodeType object to create the bytecodes
(plus all that other stuff that makes up a function), then extracts just
the interesting part and passes that into findlabels.

The good part is that you can pretend you're the compiler by just putting
the correct bytes into a array and feed it into the various dis functions.
The EXTENDED_ARG operator plays with the operand of the succeeding
instruction, everything else either doesn't have an argument or has two
bytes.

Here's a real test case, I don't know how you write unit tests for the
stdlib, but you can compare the output of the findlabels call with a known
value, and that should get you pretty close.

from opcode import *
code = bytes(
    chr(opmap["JUMP_FORWARD"]) + chr(0) + chr(0) +
    chr(EXTENDED_ARG) + chr(1) + chr(0) +
    chr(opmap["JUMP_FORWARD"]) + chr(0) + chr(0) +
    chr(opmap["RETURN_VALUE"]),
    encoding="latin-1"
)
import dis
dis.dis(code)
print(dis.findlabels(code))
if dis.findlabels(code) == [0x0000+3, 0x00010000+9]:
    print("Test passed")

Take a look in the stdlib opcode.py and find the various "JUMP" operators,
those are the guys we care about for this. Try out a bunch of cases by
augmenting the above definition of "code" and you'll soon get a feel for
what's going on.

As real, executable bytecode the above is of course non-sensical, but for a
test, it's great because you can predict exactly what should be produced.
​

[BarunParruck]

Hi, yes, that works much better, and I've definitely understood!

Now, about the tests, how large would you like them to be? For instance :

code = bytes (
	chr(opmap["JUMP_FORWARD"]) + chr(0) + chr(255) + 
	chr(EXTENDED_ARG) + chr(1) + chr(1) + 
	chr(opmap["JUMP_FORWARD"]) + chr(0) + chr(0) +
	chr(EXTENDED_ARG) + chr(1) + chr(0) +
	chr(opmap["JUMP_ABSOLUTE"]) + chr(0) + chr(1) +
	chr(opmap["RETURN_VALUE"]),
	encoding="latin-1"
	)

Would return [65283, 16842761, 65792]

And I'll make sure I test all the jump instances (I'm shuddering to imagine what a real computer would do if fed with those byte codes)

Do you want the tests to be large? (array size > 10^2/2 as a result), or is the example above fine, if I add three or four more example testing each jump instance?

[BarunParruck]

Four test cases have been included in the unittests, using the module unittest. One is the original one you gave me, the others are some that I played around with it, and thought would be useful to include.

I would love some code reviews at this point!

[BarunParruck]

Sorry, the previous file had a bug.