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

chore: handle LOAD_METHOD opcode in CPython 3.12 #129

Closed

Conversation

P403n1x87
Copy link
Contributor

Allow handling the LOAD_METHOD pseudo-instruction in CPython 3.12 by converting it to the equivalent LOAD_ATTR instruction.

Resolves #128.

Comment on lines +808 to +810
if isinstance(arg, str):
# Assume the bitflag is not set
arg = (False, arg)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change from what currently documented

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.09% ⚠️

Comparison is base (e14a0a2) 95.60% compared to head (bdc6775) 95.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
- Coverage   95.60%   95.51%   -0.09%     
==========================================
  Files           6        6              
  Lines        2000     2005       +5     
  Branches      481      484       +3     
==========================================
+ Hits         1912     1915       +3     
- Misses         53       54       +1     
- Partials       35       36       +1     
Files Changed Coverage Δ
src/bytecode/instr.py 97.17% <0.00%> (-0.51%) ⬇️
src/bytecode/concrete.py 96.68% <100.00%> (+0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Allow handling the `LOAD_METHOD` pseudo-instruction in CPython
3.12 by converting it to the equivalent `LOAD_ATTR` instruction.
@P403n1x87 P403n1x87 force-pushed the chore/handle-load-method branch from 51297f0 to bdc6775 Compare September 25, 2023 10:58
@@ -1143,6 +1143,17 @@ def concrete_instructions(self) -> None:
free_instrs: List[int] = []

for instr in self.bytecode:
if sys.version_info >= (3, 12) and isinstance(instr, Instr):
if instr.name == "LOAD_METHOD":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a better place to handle these sorts of transformations? Just thinking of ways of keeping this tidy in case of future similar changes.

@MatthieuDartiailh
Copy link
Owner

I am extremely hesitant to allow this kind of things. In particular I fear that "legalizing" the bytecode when converting to concrete may be too late since we need a CFG to compute the stack size and I would not bet on dis.stack_effect being accurate for pseudo instructions.

This and your request about TryBegin/End to me sound more like we could have a more abstract builder for bytecode that we could then safely compile before doing any computation on it. Food for though.

@P403n1x87
Copy link
Contributor Author

I've had this idea for a while now and finally had a chance to experiment with it https://github.com/P403n1x87/spasmlang. Perhaps these sorts of transformations can be handled at this higher level.

@P403n1x87
Copy link
Contributor Author

I'm closing this PR since my use cases are now covered by P403n1x87/spasmlang#2.

@P403n1x87 P403n1x87 closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LOAD_METHOD is still a valid opname but doesn't seem to be fully supported
3 participants