dis.findlabels ignores EXTENDED_ARG

@ncoghlan

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']

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

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

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

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]

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?

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))

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.

The patch I forgot to attach. Ha ha.

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? :)

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.

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?

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!

Sorry, the previous file had a bug.

Lookin' good so far. How about we try it on all the opcodes that have arguments?

See attached example for which I can see two obvious improvements:

It could be improved by taking apart that "args" list and using it to synthesize "sample_code" rather than having to hand duplicate the values in two places, albeit with different byte order.
Likewise, my hard-coded "offsets" table is pretty awful. :)

Also, is there already a test for the dis module in which you could just add this as a case?

Hi, sorry, been pretty busy, and will be for a few days, so my replies may not be instant!

Having looked through the dis test module, to be honest, I can't even find a call to dis.findlabels...odd. Maybe I've got it wrong, and I wouldn't mind somebody else taking a look! But either way, a quick overview shows me that it basically picks up functions, figures out the disassembled function, and compares expected output, in a similar way that we're working, actually.

I'll look through your example, which looks pretty good! It works as expected on my repo (fails and passes appropriately), and I'll see about improving it, but for now, it seems to do its job.

Oh, don't worry about time between responses, we all have lives (well, some of us anyhow :) ).

After looking at Lib/test/test_dis.py, I think it's just an oversight that there is no specific findlabels test (it's tested implicitly by dis.dis after all), and it would be good thing for you to add one. The obvious place seems like a new method at the bottom of the DisTests class, but we'll have to rely on (probably) Yury to make that call.

I just remembered that code can have more than one (up to three?) EXTENDED_ARG operators before the real opcode, so I added that generalization to build code around the args list...

Two things:

Verified this has always been a problem and still is in the development branch, so I added 2.7 and 3.6 to the versions list.
Couldn't tolerate the duplicate code handling the extended args operator, so in the interests of DRY, I moved the code scanner to a generator function (see attached dis_with_code_scanner.diff).

The patch is definitely not required to fix this bug, but it does isolate the original problem area to just one piece of code.

Could we get a get a quick review from a core dev saying either "go with Barun's less invasive preliminarypatch.diff" or "go with Eric's greater-churn dis_with_code_scanner.diff patch?"

(We still need to flesh out the testing a bit, so don't call us, we'll call you. :) )

Oops, wrong/bad patch, delete line 310 "arg = None" in _get_instruction_bytes...

So...firstly hi, and sorry for disappearing like that!

You want tests for the dis.findlabels library? What sort of tests are you looking for? Similiarly sized bytecode streams like the ones we worked with, or did you have something else in mind?

B

Barun, take a look at the latest version of the testfindlabels.py, see what you think. If it works for you, maybe move the test function into Lib/test/test_dis.py as part of the standard dis module tests. Still need to look at the code that's being tested and find out what cases could cause problems and then augment the test to make sure it covers those.

Hi!

Okay, sorry for disappearing like that(this is becoming rather a habit isn't it!), but with Gsoc applications, I've been insanely busy trying to get patches and bugs accepted for my first Gsoc ever. I do apologize for leaving you hanging, it was not my intention!

I'll try my best to give you an update by the end of this week and hopefully be able to keep working on this!

This bug was fixed in bpo-26881 with similar patch. Sorry, I didn't know about this issue. Your patches look good. In any case thank you for your effort.

Thanks, Serhiy. I sort of figured that it would get fixed with the
wordcode rework, and was going to verify that it was when 3.6 settled down.

On Sun, Jun 5, 2016 at 8:30 AM, Serhiy Storchaka <[email protected]>
wrote:

Serhiy Storchaka added the comment:

This bug was fixed in bpo-26881 with similar patch. Sorry, I didn't know
about this issue. Your patches look good. In any case thank you for your
effort.

----------
nosy: +serhiy.storchaka
resolution: -> out of date
stage: -> resolved
status: open -> closed
superseder: -> modulefinder should reuse the dis module

Python tracker <[email protected]>
<http://bugs.python.org/issue26448\>

Navigation Menu

Repository navigation

dis.findlabels ignores EXTENDED_ARG #70635

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

dis.findlabels ignores EXTENDED_ARG #70635

Description

Activity

efahl commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

efahl commented on Feb 27, 2016

efahl commented on Feb 27, 2016

efahl commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

efahl commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

BarunParruck commented on Feb 27, 2016

efahl commented on Feb 27, 2016

BarunParruck commented on Feb 28, 2016

efahl commented on Feb 28, 2016

efahl commented on Feb 28, 2016

efahl commented on Feb 28, 2016

efahl commented on Feb 28, 2016

BarunParruck commented on Mar 2, 2016

efahl commented on Mar 2, 2016

BarunParruck commented on Mar 11, 2016

serhiy-storchaka commented on Jun 5, 2016

efahl commented on Jun 5, 2016

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions