Bytecode instrumented placeholder - #6741
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughExpanded the Instruction enum with 70+ specialized and instrumented RustPython-only opcodes ranging from 149–254, including Resume, BinaryOp variants, LoadAttr variants, Call variants, and instrumented operations. Moved SetExcInfo/Subscript to positions 132/133 and LoadClosure to 255. Updated TryFrom converter and InstructionMetadata to handle new opcode ranges. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 1 | ❌ 2 ❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
| SetExcInfo = 134, | ||
| Subscript = 135, | ||
| SetExcInfo = 132, | ||
| Subscript = 133, |
There was a problem hiding this comment.
The bytecode already has to be purged because LoadClosure is no longer 253. So I've moved these as well
Sorry, something went wrong.
There was a problem hiding this comment.
sure, this will also totally reordered on 3.14
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@crates/compiler-core/src/bytecode/instruction.rs`: - Around line 280-374: Instrumented jump opcodes (InstrumentedJumpForward, InstrumentedJumpBackward, InstrumentedPopJumpIfTrue, InstrumentedPopJumpIfFalse, InstrumentedPopJumpIfNone, InstrumentedPopJumpIfNotNone) are currently no-arg placeholders but should either be real control-flow ops or explicit no-op instrumentation; update them accordingly: if they represent instrumented control-flow, change their variants to carry Arg<Label>, update their stack_effects, and add them to label_arg() and unconditional_branch()/conditional-branch logic so control-flow analysis recognizes their targets (refer to these enum variants and the functions label_arg() and unconditional_branch()); if they are only markers, leave them no-arg but document that intent and add explicit comments/tests and ensure they are never used in control-flow analysis (e.g., assert they are ignored) so behavior is clear and consistent across the codebase.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 746e71a and 79df18b.
⛔ Files ignored due to path filters (2)- Lib/_opcode_metadata.py is excluded by !Lib/**
- Lib/test/test__opcode.py is excluded by !Lib/**
- crates/compiler-core/src/bytecode/instruction.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style using cargo fmt to format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/compiler-core/src/bytecode/instruction.rs
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
crates/compiler-core/src/bytecode/instruction.rs (3)248-279: Custom-opcode range update looks consistent.
Thanks for tightening the RustPython-only block and moving SetExcInfo/Subscript; the enum layout now cleanly documents the custom range.
401-448: Validator range updates look good.
The specialized/instrumented ranges are now explicitly accepted, which keeps TryFrom<u8> aligned with the expanded opcode space.
693-784: These are CPython 3.13 bytecode compatibility placeholders that are never emitted.
All the opcodes marked as "Placeholder" (BinaryOpAddFloat, CallBuiltinFast, LoadGlobalBuiltin, InstrumentedJumpForward, etc.) are included in the instruction enum to support deserializing CPython 3.13 bytecode, but they are never actually generated by the RustPython compiler itself. The stack effect of 0 for these opcodes is correct since they will never execute—the actual bytecode generation always uses the primary (non-specialized) instruction variants. No stack-depth analysis issues will occur because the code paths that emit bytecode never produce these placeholder instructions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Sorry, something went wrong.
| Resume { | ||
| arg: Arg<u32>, | ||
| } = 149, | ||
| BinaryOpAddFloat = 150, // Placeholder | ||
| BinaryOpAddInt = 151, // Placeholder | ||
| BinaryOpAddUnicode = 152, // Placeholder | ||
| BinaryOpMultiplyFloat = 153, // Placeholder | ||
| BinaryOpMultiplyInt = 154, // Placeholder | ||
| BinaryOpSubtractFloat = 155, // Placeholder | ||
| BinaryOpSubtractInt = 156, // Placeholder | ||
| BinarySubscrDict = 157, // Placeholder | ||
| BinarySubscrGetitem = 158, // Placeholder | ||
| BinarySubscrListInt = 159, // Placeholder | ||
| BinarySubscrStrInt = 160, // Placeholder | ||
| BinarySubscrTupleInt = 161, // Placeholder | ||
| CallAllocAndEnterInit = 162, // Placeholder | ||
| CallBoundMethodExactArgs = 163, // Placeholder | ||
| CallBoundMethodGeneral = 164, // Placeholder | ||
| CallBuiltinClass = 165, // Placeholder | ||
| CallBuiltinFast = 166, // Placeholder | ||
| CallBuiltinFastWithKeywords = 167, // Placeholder | ||
| CallBuiltinO = 168, // Placeholder | ||
| CallIsinstance = 169, // Placeholder | ||
| CallLen = 170, // Placeholder | ||
| CallListAppend = 171, // Placeholder | ||
| CallMethodDescriptorFast = 172, // Placeholder | ||
| CallMethodDescriptorFastWithKeywords = 173, // Placeholder | ||
| CallMethodDescriptorNoargs = 174, // Placeholder | ||
| CallMethodDescriptorO = 175, // Placeholder | ||
| CallNonPyGeneral = 176, // Placeholder | ||
| CallPyExactArgs = 177, // Placeholder | ||
| CallPyGeneral = 178, // Placeholder | ||
| CallStr1 = 179, // Placeholder | ||
| CallTuple1 = 180, // Placeholder | ||
| CallType1 = 181, // Placeholder | ||
| CompareOpFloat = 182, // Placeholder | ||
| CompareOpInt = 183, // Placeholder | ||
| CompareOpStr = 184, // Placeholder | ||
| ContainsOpDict = 185, // Placeholder | ||
| ContainsOpSet = 186, // Placeholder | ||
| ForIterGen = 187, // Placeholder | ||
| ForIterList = 188, // Placeholder | ||
| ForIterRange = 189, // Placeholder | ||
| ForIterTuple = 190, // Placeholder | ||
| LoadAttrClass = 191, // Placeholder | ||
| LoadAttrGetattributeOverridden = 192, // Placeholder | ||
| LoadAttrInstanceValue = 193, // Placeholder | ||
| LoadAttrMethodLazyDict = 194, // Placeholder | ||
| LoadAttrMethodNoDict = 195, // Placeholder | ||
| LoadAttrMethodWithValues = 196, // Placeholder | ||
| LoadAttrModule = 197, // Placeholder | ||
| LoadAttrNondescriptorNoDict = 198, // Placeholder | ||
| LoadAttrNondescriptorWithValues = 199, // Placeholder | ||
| LoadAttrProperty = 200, // Placeholder | ||
| LoadAttrSlot = 201, // Placeholder | ||
| LoadAttrWithHint = 202, // Placeholder | ||
| LoadGlobalBuiltin = 203, // Placeholder | ||
| LoadGlobalModule = 204, // Placeholder | ||
| LoadSuperAttrAttr = 205, // Placeholder | ||
| LoadSuperAttrMethod = 206, // Placeholder | ||
| ResumeCheck = 207, // Placeholder | ||
| SendGen = 208, // Placeholder | ||
| StoreAttrInstanceValue = 209, // Placeholder | ||
| StoreAttrSlot = 210, // Placeholder | ||
| StoreAttrWithHint = 211, // Placeholder | ||
| StoreSubscrDict = 212, // Placeholder | ||
| StoreSubscrListInt = 213, // Placeholder | ||
| ToBoolAlwaysTrue = 214, // Placeholder | ||
| ToBoolBool = 215, // Placeholder | ||
| ToBoolInt = 216, // Placeholder | ||
| ToBoolList = 217, // Placeholder | ||
| ToBoolNone = 218, // Placeholder | ||
| ToBoolStr = 219, // Placeholder | ||
| UnpackSequenceList = 220, // Placeholder | ||
| UnpackSequenceTuple = 221, // Placeholder | ||
| UnpackSequenceTwoTuple = 222, // Placeholder | ||
| InstrumentedResume = 236, // Placeholder | ||
| InstrumentedEndFor = 237, // Placeholder | ||
| InstrumentedEndSend = 238, // Placeholder | ||
| InstrumentedReturnValue = 239, // Placeholder | ||
| InstrumentedReturnConst = 240, // Placeholder | ||
| InstrumentedYieldValue = 241, // Placeholder | ||
| InstrumentedLoadSuperAttr = 242, // Placeholder | ||
| InstrumentedForIter = 243, // Placeholder | ||
| InstrumentedCall = 244, // Placeholder | ||
| InstrumentedCallKw = 245, // Placeholder | ||
| InstrumentedCallFunctionEx = 246, // Placeholder | ||
| InstrumentedInstruction = 247, // Placeholder | ||
| InstrumentedJumpForward = 248, // Placeholder | ||
| InstrumentedJumpBackward = 249, // Placeholder | ||
| InstrumentedPopJumpIfTrue = 250, // Placeholder | ||
| InstrumentedPopJumpIfFalse = 251, // Placeholder | ||
| InstrumentedPopJumpIfNone = 252, // Placeholder | ||
| InstrumentedPopJumpIfNotNone = 253, // Placeholder | ||
| InstrumentedLine = 254, // Placeholder |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
# First, let's check the file size and structure
wc -l crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review (280-374)
sed -n '280,374p' crates/compiler-core/src/bytecode/instruction.rs | cat -nRepository: RustPython/RustPython
Length of output: 6574
🏁 Script executed:
#!/bin/bash
# Search for how label_arg and unconditional_branch are implemented
rg -A 5 "fn label_arg|fn unconditional_branch" crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 1402
🏁 Script executed:
#!/bin/bash
# Search for Arg<Label> pattern to understand expected structure
rg "Arg<Label>" crates/compiler-core/src/bytecode/instruction.rs | head -20Repository: RustPython/RustPython
Length of output: 729
🏁 Script executed:
#!/bin/bash
# Check if InstrumentedJumpForward, InstrumentedJumpBackward are defined with or without Arg
rg "InstrumentedJump" crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Search for where these instrumented opcodes are actually used or created
rg "InstrumentedJumpForward|InstrumentedJumpBackward|InstrumentedPopJump" --type rs -B 3 -A 3Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the context around these definitions and look for any match statements
sed -n '200,280p' crates/compiler-core/src/bytecode/instruction.rs | cat -nRepository: RustPython/RustPython
Length of output: 2534
🏁 Script executed:
#!/bin/bash
# Look for the enum definition structure to see if there's a pattern
sed -n '1,200p' crates/compiler-core/src/bytecode/instruction.rs | grep -A 2 "JumpForward\|JumpBackward" | head -30Repository: RustPython/RustPython
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about instrumented instructions
rg -B 5 "InstrumentedJump|InstrumentedPopJump" crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 1288
🏁 Script executed:
#!/bin/bash
# Check if there's a match arm or implementation that handles these instrumented opcodes specially
rg "match.*self" crates/compiler-core/src/bytecode/instruction.rs -A 100 | grep -A 5 "InstrumentedJump"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for any code that might convert between regular and instrumented versions
rg "Instrumented.*Jump|Jump.*Instrumented" crates/compiler-core/src/bytecode/Repository: RustPython/RustPython
Length of output: 1331
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about these Instrumented instructions
rg -B 10 "Instrumented" crates/compiler-core/src/bytecode/instruction.rs | head -50Repository: RustPython/RustPython
Length of output: 2724
🏁 Script executed:
#!/antml/bash
# Look at the entire file context around Instrumented opcodes to understand their purpose
sed -n '1,50p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 1631
🏁 Script executed:
#!/bin/bash
# Check if these are part of a newer enum or alternative implementation
git log --oneline --all -S "InstrumentedJumpForward" | head -5Repository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about what "Placeholder" means
rg "Placeholder" crates/compiler-core/src/bytecode/instruction.rs | head -20Repository: RustPython/RustPython
Length of output: 788
🏁 Script executed:
#!/bin/bash
# Check the CPython 3.13 instrumentation opcodes to understand the intent
# These might be based on CPython's instrumented opcodes
rg -B 5 -A 5 "instrumented_start|instrumented_end" crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 1085
🏁 Script executed:
#!/antml/bash
# Look for any usage of these Instrumented opcodes in the codebase
find crates -name "*.rs" -type f -exec grep -l "InstrumentedJump" {} \;Repository: RustPython/RustPython
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check if there's any code generation or transformation that would convert Jump to InstrumentedJump
rg "to_instrumented|from_instrumented|instrument_" crates/compiler-core/Repository: RustPython/RustPython
Length of output: 47
Instrumented jump opcodes lack label targets and control-flow integration.
InstrumentedJumpForward/Backward and InstrumentedPopJump* (248–253) are defined as no-argument variants and excluded from both label_arg() and unconditional_branch(). If these are meant as instrumented versions of control-flow operations (similar to CPython 3.13's instrumented opcodes), they should carry Arg<Label> and be reflected in control-flow analysis. Currently, all are marked // Placeholder with stack_effect of 0 and no usage in the codebase. Clarify whether these should be functional control-flow opcodes or pure instrumentation markers only, and complete the implementation accordingly.
🤖 Prompt for AI AgentsIn `@crates/compiler-core/src/bytecode/instruction.rs` around lines 280 - 374, Instrumented jump opcodes (InstrumentedJumpForward, InstrumentedJumpBackward, InstrumentedPopJumpIfTrue, InstrumentedPopJumpIfFalse, InstrumentedPopJumpIfNone, InstrumentedPopJumpIfNotNone) are currently no-arg placeholders but should either be real control-flow ops or explicit no-op instrumentation; update them accordingly: if they represent instrumented control-flow, change their variants to carry Arg<Label>, update their stack_effects, and add them to label_arg() and unconditional_branch()/conditional-branch logic so control-flow analysis recognizes their targets (refer to these enum variants and the functions label_arg() and unconditional_branch()); if they are only markers, leave them no-arg but document that intent and add explicit comments/tests and ensure they are never used in control-flow analysis (e.g., assert they are ignored) so behavior is clear and consistent across the codebase.
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
is the newly introduced ops all the pseudo ops?
Sorry, something went wrong.
| SetExcInfo = 134, | ||
| Subscript = 135, | ||
| SetExcInfo = 132, | ||
| Subscript = 133, |
There was a problem hiding this comment.
sure, this will also totally reordered on 3.14
Sorry, something went wrong.
None are. All are either specialized or instrumented opcodes |
Sorry, something went wrong.
|
Thanks! I didn't know about them. |
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.