Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make dis.Instruction more useful #102676

Closed
gvanrossum opened this issue Mar 14, 2023 · 6 comments · Fixed by #103969
Closed

Make dis.Instruction more useful #102676

gvanrossum opened this issue Mar 14, 2023 · 6 comments · Fixed by #103969
Labels
type-feature A feature request or enhancement

Comments

@gvanrossum
Copy link
Member

gvanrossum commented Mar 14, 2023

I recently was playing with bytecode instructions and found that the dis.Instruction did almost what I needed, but not quite -- I ended up reimplementing it, mostly reinventing the wheel. I propose to improve this class in dis.py as follows:

  • start_offset: start of the instruction including EXTENDED_ARG prefixes
  • jump_target: bytecode offset where a jump goes (can be property computed from opcode and arg)
  • baseopname, baseopcode: name and opcode of the "family head" for specialized instructions (can be properties)
  • cache_offset, end_offset: start and end of the cache entries (can be properties)
  • oparg: alias for arg (in most places we seem to use oparg)

Of these, only start_offset needs to be a new data field -- the others can be computed properties. For my application, start_offset was important to have (though admittedly I could have done without if I had treated EXTENDED_ARG as a NOP). It just feels wrong that offset points to the opcode but oparg includes the extended arg -- this means one has to explicitly represent EXTENDED_ARG instructions in a sequence of instructions even though their effect (arg) is included in the Instruction.

I also added a __str__ method to my Instruction class that shows the entire instruction -- this could call the _disassemble method with default arguments. Here I made one improvement over _disassemble: if the opname is longer than _OPNAME_WIDTH but the arg is shorter than _OPARG_WIDTH, I let the opname overflow into the space reserved for the oparg, leaving at least one space in between. This makes for fewer misaligned opargs, since the opname is left-justified and the oparg is right-justified.

@isidentical @serhiy-storchaka @iritkatriel @markshannon @brandtbucher

Linked PRs

@gvanrossum gvanrossum added the type-feature A feature request or enhancement label Mar 14, 2023
@tomasr8
Copy link
Contributor

tomasr8 commented Apr 18, 2023

Hi! These suggestions seem very useful and fairly doable for a bytecode newbie :) I'd like to have a stab at this!

@gvanrossum
Copy link
Member Author

@tomasr8 Please give it a try. Are you familiar with https://devguide.python.org/ ?

@tomasr8
Copy link
Contributor

tomasr8 commented Apr 20, 2023

Yes, I've read it :) FWIW I have most of it working, just need to add some tests and update the docs. Though I'm away for a couple of days so ETA 1 week.

@tomasr8
Copy link
Contributor

tomasr8 commented May 11, 2023

Hi @gvanrossum, if you have time, would you mind having a look at the PR? Thanks! :)

@gvanrossum
Copy link
Member Author

I will, but not in time for the 3.12 feature freeze (which is Monday May 22). This means we'll be aiming for 3.13. Sorry.

@tomasr8
Copy link
Contributor

tomasr8 commented May 17, 2023

Thanks for letting me know! And it's fine, I don't really care in which version this ends up :)

gvanrossum pushed a commit that referenced this issue Jun 11, 2023
)

Adds start_offset, cache_offset, end_offset, baseopcode,
baseopname, jump_target and oparg to dis.Instruction.

Also slightly improves the disassembly output by allowing
opnames to overflow into the space reserved for opargs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants