Repository navigation
jit,interp: base + JIT correctness fixes; tagged small-int flip attempted then reverted (net-zero) - #548
Conversation
WalkthroughThe changes improve tagged-immediate safety, add forward bytecode decoding for interpreter and JIT loops, correct object and arithmetic edge cases, preserve short-preamble mappings, and adjust native backend lowering. ChangesTagged integer runtime safety
Forward bytecode decoding
JIT optimization and backend corrections
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 76d04fa). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e404e08778
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "object.__new__() takes exactly one argument (the type to instantiate)".to_string(), | ||
| )); | ||
| } | ||
| if object_slot_inherited(tp_init, obj_init) { |
There was a problem hiding this comment.
When a class sets __init__ = staticmethod(object.__init__), PyPy/CPython do not treat that as inheriting object.__init__ for the excess-args gate: pypy/objspace/std/objectobject.py uses _same_static_method only for tp_new and a raw space.is_w(tp_init, _object_init(space)) for init. Because object_slot_inherited unwraps staticmethods here, C(1) / object.__new__(C, 1) now raises C() takes no arguments instead of allocating, so the tp_init check needs a non-unwrapping comparison.
Useful? React with 👍 / 👎.
a8d5d7e to
a354a6a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a354a6a481
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if opcode_pc != pc && u8::from(instruction) < 44 { | ||
| return Err(crate::pycode::BytecodeCorruption); | ||
| } | ||
| return Ok((opcode_pc, instruction, op_arg)); |
There was a problem hiding this comment.
When pc is already the real opcode after an EXTENDED_ARG prefix, this returns the operand from a fresh OpArgState, so the high bits contributed by the prefix are lost. That can happen for valid control-flow targets: jump/trace targets are recorded at the real opcode, and eval_loop/pyre-jit now feed frame.next_instr() directly into this helper. For example, a branch to a LOAD_CONST/STORE_NAME whose index is >=256 will execute with only the low-byte operand and read/write the wrong constant/name; the old decode_instruction_for_dispatch handled this by scanning backward from the target opcode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eb655d30b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // lowering for both tagged and boxed operands. | ||
| let vi = op.pos.get().raw(); | ||
| if !OpRef::raw_is_constant(vi) { | ||
| emit_resolve(&mut sink, constants, op.arg(0).to_opref()); |
There was a problem hiding this comment.
When the wasm backend is compiled, this call no longer matches emit_resolve's signature (sink, constants, value_types, opref in the same file); it is the only three-argument call while the surrounding cases pass value_types, so majit-backend-wasm fails to build before any tests can run. Please thread value_types here as in the other emit_resolve calls.
Useful? React with 👍 / 👎.
09a0ffb to
305f7bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 305f7bf37f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// point is to avoid putting `& 1` tag checks on every object — they are | ||
| /// gated on this static, which is kept in lockstep with the GC | ||
| /// `taggedpointers` config (`pyre-jit` `build_gc`). | ||
| pub const CAN_BE_TAGGED: bool = true; |
There was a problem hiding this comment.
With tagging enabled here, interpreter warmup leaves small-int locals as tagged immediates, but the first tracing path still snapshots the frame in bound_reached without calling untag_tagged_frame_locals (that helper only runs when entering an already compiled loop/function). For an ordinary hot integer loop, trace_unbox_int_with_resume_descr therefore records the tagged GuardTrue(lowbit) path, while the loop-carried wrapint value is a heap W_IntObject; once the compiled loop is entered, the heap value fails the recorded low-bit guard and the loop immediately deopts instead of running compiled code.
Useful? React with 👍 / 👎.
088b3ae to
f2c62ed
Compare
In the generic `__bool__` tail of `bool_descr_new`, the TypeError path built the returned type's name via `(*(*result).ob_type).name`. When `__bool__` returns a tagged immediate the ob_type deref reads non-pointer bits. Add a `CAN_BE_TAGGED`-gated arm that names a tagged immediate `"int"` without the deref, matching the guard in `builtin_str`. Dead code at flag-false. Assisted-by: Claude
The two "'{}' object is not iterable" TypeError builders in `iter`
(the `__iter__ = None` arm and the terminal fall-through) named the
type via `(*(*obj).ob_type).name`, a raw ob_type deref. A tagged
immediate passes every tag-safe type probe and falls through to these,
faulting on the deref. Route both through a `CAN_BE_TAGGED`-gated
`not_iterable_type_name` that names a tagged immediate `"int"` without
the deref. Dead code at flag-false.
Assisted-by: Claude
…es through it The "must be X, not <name>" / "not iterable/subscriptable/unpackable" TypeError builders named the offending type via a raw `(*(*obj).ob_type).name`. A tagged immediate passes the tag-safe type probes that precede these builders, then reaches the raw deref and reads its non-pointer bits. Add `pyre_object::type_name_of`, a gated chokepoint that names a tagged immediate `"int"` and otherwise keeps the raw `ob_type.name` (not `r#type`, which would return the w_class subclass name). Route the reachable sites through it: builtin_str encoding/errors/ decoding, checkattrname, sequence unpack builders, and the not-subscriptable builder. Dead code at flag-false. Assisted-by: Claude
…type_name_of The CALL not-callable builder (runtime_ops.rs:425) and the GET_ITER ensure_range_iter not-iterable fall-through (:1259) named the type via a raw `(*(*obj).ob_type).name`, which a tagged immediate reaches after the tag-safe probes reject it. Route both through `type_name_of`, matching the other type-name builders. Dead code at flag-false. Assisted-by: Claude
`va % vb` panics on i64::MIN % -1 (quotient 2**63 unrepresentable in i64), the same case int_add/sub/mul already guard with checked arithmetic. Replace with checked_rem, falling back to a BigInt mod_floor (result 0) on overflow. int_floordiv is unaffected: its checked_div early-return at :418 catches the sole overflow operand pair before the `%` at :422 is reached. Assisted-by: Claude
w_list_setslice computed each strategy arm's window as start.min(len) .. end.min(len) without raising the upper bound to the lower one. A backwards slice (start > stop) then panicked in the object arm's Vec::splice (start > end range) and silently truncated the tail in the int/float arms (e - s underflowed usize). Clamp end up to start at the top of the function so a backwards slice is a pure insertion at start, matching list_ass_slice's `if (ihigh < ilow) ihigh = ilow`. Assisted-by: Claude
isinstance()/issubclass() recurse in native Rust over nested tuple and PEP-604 union classinfo without pushing a Python frame, so the frame-level stack_check() chokepoints never fire and a deeply nested classinfo blows the C stack (segfault/timeout) instead of raising RecursionError. Add stack_check() at each head after unwrap_cell. Assisted-by: Claude
object.__new__ asserted on an empty arg list (SIGABRT when called with no type) and silently returned a typeless instance for a non-type first argument; object.__init__ was a no-op that swallowed excess arguments. Port the descr__new__/descr__init__ excess-args decision from objectobject.py: no arg -> "not enough arguments"; non-type -> "X is not a type object (...)"; excess args -> "takes exactly one argument" or "takes no arguments" depending on whether the class overrides __new__/__init__, unwrapping the __new__ staticmethod for the identity compare. Type names in the messages come from r#type()+w_type_get_name (the Python type), not the raw ob_type layout name. Assisted-by: Claude
A static sweep of all 67 live `(*(*X).ob_type).name` derefs found 28 on error/fallback paths that an arbitrary user value — and so a tagged int at CAN_BE_TAGGED=true — can reach, beyond the crash-observed set the earlier batch closed. Route each through pyre_object::type_name_of, which folds to the same raw deref at flag-false (byte-identical) and returns "int" for a tagged immediate. The remaining 39 derefs are narrowed by an is_int early-return, sit on the None arm of an r#type match, or are the type_name_of chokepoint itself, and stay raw. Sites: baseobjspace setitem_slot/set___class__/delattr; typedef set.__init__/dict.__repr__/MappingProxyType/__bases__ setter/member-descr get+set+delete/int.to_bytes/bytes.replace/removeprefix/removesuffix/fromhex/ int.from_bytes/bytes.decode enc+errors/descr_get_dict; builtins float(); opcode_ops list_extend/dict_update/dict_merge; math try_get_double; _codecs encode+decode; call not-callable; type_methods str.encode. Assisted-by: Claude
py_repr/dict_repr recurse into each element in native Rust with no Python frame push and only ReprGuard (pointer-cycle detection); nothing bounded the depth, so a deeply nested structure overflows the C stack. Add stack_check() at the py_repr recursion hub so deep dict/list/tuple repr raises RecursionError, matching test_repr_deep in test_dict/list_tests. Mirrors the insert_stack_check pass on the recursive isinstance/issubclass path. Assisted-by: Claude
str() of a single-arg exception routes through this WTF-8 path; a tagged-int arg was read as a pointer via unwrap_cell / ob_type (str(ValueError(5)) segfaulted). Precheck the tagged immediate before the deref and return None to fall back to py_str, which formats the value. py_str already has this precheck; the WTF-8 sibling was missing it. Assisted-by: Claude
Flip the tagged_int::CAN_BE_TAGGED master switch. A small int is now stored as an immediate (value << 1) | 1 pointer instead of a heap W_IntObject, so w_int_new avoids the malloc for taggable values. The GcConfig.taggedpointers wire (build_gc) and the jit_fnaddr baked constant both read this const, so the collector skips tagged immediates and the JIT sees the enabled value automatically. Update the three tests that assume the untagged representation: - tagged_int enabled_after_flip asserts the switch is on - intobject identity tests gate on CAN_BE_TAGGED && fits_tagged, expecting value-identical immediates for taggable ints Gate battery: cargo test -p pyre-object 211 pass; check.py dynasm 180/180 + cranelift 180/180; int-module --full diff no new WORSENED; GC stress 30/30; perf A/B interleaved 7/7 neutral, no regression. Assisted-by: Claude
is_set_iterator was the remaining hand-rolled `(*obj).ob_type ==` predicate without the tagged-int short-circuit that is_seq_iter and the ae9b815 iterobject sweep carry. baseobjspace::iter probes a receiver through an `||` predicate chain that reaches is_set_iterator after the tag-safe py_type_check-backed predicates return false; on a tagged small int that hand-rolled deref reads ob_type through the odd pointer. Add the `CAN_BE_TAGGED && is_tagged_int` short-circuit, matching the sibling probes. Gated on CAN_BE_TAGGED so flip-OFF is byte-identical. Assisted-by: Claude
ExtendedShortPreambleBuilder::setup pushed the box-replaced operand into
short_jump_args for an unmapped Phase-1 jump arg. inline_short_preamble's
force loop then demands every non-const short_jump_arg be a key in
`mapping`, which is keyed on the static short-op result positions
(mapping[sp_op.pos]) plus seeded short-inputargs. When a loop-carried
jump arg forwards to a fresh Phase-2 SameAsI alias, that alias is neither
a replay key nor a seeded inputarg, so the lookup panics at unroll.rs
`mapping.get(jump_arg).expect("mapping missing jump_arg")`.
Push the static position `*arg` instead, mirroring shortpreamble.py:461
`self.jump_args = jump_args` (verbatim, no forwarding); get_box_replacement
is applied only after the mapping lookup via get_replacement_opref
(unroll.py:374,440). The resolved operand is retained for the emitted
JUMP sentinel's producer binding (jump_args_operand).
Surfaced under CAN_BE_TAGGED where a loop-carried small int rides the
label as a raw Type::Int op whose forwarded identity diverges from its
static replay key; boxed InputArgRef locals coincided and did not panic.
Assisted-by: Claude
…t pos inline_short_preamble seeds `mapping` with the short-preamble input -> jump_arg pairs, then binds each replayed short op's result position via `mapping.insert(sp_op.pos, new_ref)`. In RPython these two key spaces are disjoint (an input Box is never also a produced-result Box), so the replay can never overwrite a seed. pyre's flat-OpRef namespace has no such guarantee: a re-virtualizing short op (NewWithVtable) can be exported with a result position equal to a loop-input OpRef, so the insert clobbered the seed. Every later short op reading that input (GetfieldGcPureI) then resolved to the fresh, field-less allocation instead of the loop-carried box, feeding an uninitialized field into a loop slot. The uninitialized read was masked when a nursery value happened to match, but a residual CALL in the loop body overwrote the nursery and surfaced the garbage (foriter_call_body: 241830/231390 instead of 1800000). Snapshot the seeded input keys and skip the `mapping.insert` when a replayed op's result position is one of them, restoring the input-vs-result key disjointness RPython gets from Box identity. The redundant reconstruction op is left unmapped and elided by DCE. Assisted-by: Claude
The seeded_input_keys guard skipped mapping.insert for every replayed short op whose result position aliased a seeded input key. That is correct only for a re-virtualizing allocation (New..Newunicode), whose field-less materialization must not replace the loop-carried box. A colliding non-allocation op — e.g. a GetfieldGcPureI producing an Int loop slot whose result position happens to alias a seed — is a genuine recomputation whose fresh result is the correct binding; skipping it stranded the slot on the seed's Ref box and fed a Ref into an Int consumer (getintbound_handle 'i'-typed assert, store_global_hot panic on both backends). Gate the skip on sp_op.opcode.is_malloc() so only allocation ops preserve the seed. store_global_hot passes both backends; foriter_call_body stays 1800000 (the miscompile the original skip fixed). Assisted-by: Claude
Return mainline int to the boxed W_Int representation. Tagging a non-UnboxedValue mainline int has no RPython/PyPy basis (rclass.py:103 usetagging gates on UnboxedValue subclasses; W_IntObject is not one; taggedpointers defaults False). The tag-path scaffolding stays inert behind the flag.
Remove the non-orthodox per-slot Int signals added for the tagged-int flip: observed_slot_int_type (dead — no production caller), the S3 symbolic type-map per-slot reset, the vable-deopt per-slot re-box, and the Int virtual-state leaf test. With int slots no longer tagged these declare no Int slot, so the merge-point/resume paths return to the uniform-Ref contract that mirrors RPython's GCREF locals_cells_stack_w. Also drops the int_carry_deopt_probe bench that only exercised the re-box path.
The inheritance_dispatch_min bench and gc_stress probe tested a flip-ON SIGSEGV; with tagging disabled they are moot.
trace_unbox_int_with_resume on a constant boxed W_Int reads the payload with GetfieldGcPureI; the lowbit tag-discrimination guard is still elided for a constant operand (pyjitpl.py:2583 generate_guard const-skip). Update the assertion from the tagged IntRshift lowering the flip expected.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/d3e33b8f7bb107301618a6b4e6f875e7bcef090b/pyre-object/src/tagged_int.rs#L85
This new enablement test now asserts CAN_BE_TAGGED, but the constant above still remains false, and all of the new maker/GC/JIT paths are gated on that same constant. In this state the tagged-int feature is silently disabled in normal runs, and the enabled_after_flip unit test fails as soon as pyre-object tests can be built, contradicting the commit's claimed flip.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
decode_instruction_forward started a fresh OpArgState at the given pc, so a pc naming a real opcode that sits past its own EXTENDED_ARG prefix dropped the prefix's high byte and returned a truncated oparg. The interpreter dispatch loop always enters at the logical instruction start (the EXTENDED_ARG), but a JIT jump/loop-header/resume coordinate points at the real opcode past the prefix (jump_target_backward targets the opcode, not its EXTENDED_ARG), so getwidth's outer FOR_ITER (real opcode preceded by EXTENDED_ARG) decoded a truncated delta under the JIT and corrupted the loop variable. Back up over any immediately-preceding EXTENDED_ARG units to the logical start before accumulating forward. The back-up loop runs zero iterations in the common no-prefix case, so the hot path stays a single forward pass. The u8 < 44 malformed-chain guard is preserved, rekeyed on the backed-up start. Update the two oracle tests to assert the full oparg (not just the opcode) when entered directly at a real opcode past its prefix, and add forward_decode_at_real_opcode_recovers_extended_arg covering a FOR_ITER whose delta exceeds 255. Assisted-by: Claude
The tagged-int revert set CAN_BE_TAGGED=false but left the flip-era enabled_after_flip test asserting the flag is true, so it failed. Restore the pre-flip defaults_to_untagged test asserting the flag is off. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b4e73d408
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /// `rpython/rtyper/lltypesystem/rtagged.py:64-96` static `can_be_tagged` | ||
| /// gate, collapsed to the single runtime `int` class. Defaults `false`, | ||
| /// gate, collapsed to the single runtime `int` class. Enabled (#22), |
There was a problem hiding this comment.
This commit is presented as enabling tagged small ints, but the master gate immediately below still remains pub const CAN_BE_TAGGED: bool = false. With this value, w_int_new never returns tagged immediates, build_gc leaves taggedpointers disabled, and the JIT-baked constant is 0, so the feature is not actually enabled despite the new enablement paths and documentation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/state.rs (1)
2969-2969: 📐 Maintainability & Code Quality | 🔵 TrivialReminder: Re-extract Charon files and run benchmarks.
Please ensure the prepass is updated and performance is verified for these JIT changes.
As per coding guidelines, "After changing interpreter, object, or JIT source, re-extract the corresponding Charon
.ullbcfiles withscripts/extract-llbc.pybefore rebuilding the rtyper prepass." and "After JIT changes, run all eight benchmarks and do not commit if any benchmark regresses."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit-trace/src/state.rs` at line 2969, After updating the JIT logic around pyre_object::tagged_int::CAN_BE_TAGGED, re-extract the corresponding Charon .ullbc files with scripts/extract-llbc.py before rebuilding the rtyper prepass, then run all eight benchmarks and do not commit if any benchmark regresses.Source: Coding guidelines
pyre/pyre-interpreter/src/opcode_ops.rs (1)
448-448: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winTag-safe type name replacements were missed in
dict_merge_valueerror paths. IfDICT_MERGEreceives a taggedintimmediate fordictorsource, dereferencing itsob_typedirectly will cause an invalid memory access and crash the interpreter. The shared root cause is omittingpyre_object::type_name_ofin these remaining error paths.
pyre/pyre-interpreter/src/opcode_ops.rs#L448-L448: replaceunsafe { (*(*dict).ob_type).name }withunsafe { pyre_object::type_name_of(dict) }.pyre/pyre-interpreter/src/opcode_ops.rs#L492-L492: replaceunsafe { (*(*source).ob_type).name }withunsafe { pyre_object::type_name_of(source) }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/opcode_ops.rs` at line 448, Replace the direct ob_type dereference in dict_merge_value at pyre/pyre-interpreter/src/opcode_ops.rs:448 with pyre_object::type_name_of(dict), and make the same replacement for source at pyre/pyre-interpreter/src/opcode_ops.rs:492. Preserve the existing error-path behavior while ensuring tagged immediates are handled safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 14425-14440: The constructor validation gates still use
same_inherited_slot, which compares raw identities and misses
staticmethod-wrapped inherited slots. Replace those checks with
object_slot_inherited, preserving the existing object.__new__ and __init__
validation behavior while allowing equivalent staticmethod-wrapped slots.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/opcode_ops.rs`:
- Line 448: Replace the direct ob_type dereference in dict_merge_value at
pyre/pyre-interpreter/src/opcode_ops.rs:448 with
pyre_object::type_name_of(dict), and make the same replacement for source at
pyre/pyre-interpreter/src/opcode_ops.rs:492. Preserve the existing error-path
behavior while ensuring tagged immediates are handled safely.
In `@pyre/pyre-jit-trace/src/state.rs`:
- Line 2969: After updating the JIT logic around
pyre_object::tagged_int::CAN_BE_TAGGED, re-extract the corresponding Charon
.ullbc files with scripts/extract-llbc.py before rebuilding the rtyper prepass,
then run all eight benchmarks and do not commit if any benchmark regresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7fdae09-8731-46ff-afc0-1d529ecb3645
📒 Files selected for processing (27)
majit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-wasm/src/codegen.rsmajit/majit-metainterp/src/optimizeopt/shortpreamble.rsmajit/majit-metainterp/src/optimizeopt/unroll.rspyre/pyre-interpreter/src/argument.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/module/_codecs/mod.rspyre/pyre-interpreter/src/module/math/interp_math.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/pyopcode.rspyre/pyre-interpreter/src/runtime_ops.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/assembler.rspyre/pyre-object/src/intobject.rspyre/pyre-object/src/iterobject.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/pyobject.rspyre/pyre-object/src/setobject.rspyre/pyre-object/src/tagged_int.rs
| fn object_slot_inherited( | ||
| class_slot: Option<PyObjectRef>, | ||
| object_slot: Option<PyObjectRef>, | ||
| ) -> bool { | ||
| let unwrap = |s: Option<PyObjectRef>| -> PyObjectRef { | ||
| match s { | ||
| Some(f) if unsafe { pyre_object::function::is_staticmethod(f) } => unsafe { | ||
| pyre_object::function::w_staticmethod_get_func(f) | ||
| }, | ||
| Some(f) => f, | ||
| None => PY_NULL, | ||
| } | ||
| }; | ||
| let a = unwrap(class_slot); | ||
| !a.is_null() && crate::baseobjspace::is_w(a, unwrap(object_slot)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline pyre/pyre-interpreter/src/typedef.rs \
--items all --type function --match 'object_slot_inherited|same_inherited_slot'
rg -n -C4 \
'\b(fn\s+)?(object_slot_inherited|same_inherited_slot)\b' \
pyre/pyre-interpreter/src/typedef.rsRepository: youknowone/pyre
Length of output: 3380
🏁 Script executed:
#!/bin/bash
sed -n '14425,14580p' pyre/pyre-interpreter/src/typedef.rs
printf '\n--- usages ---\n'
rg -n '\bobject_slot_inherited\b|\bsame_inherited_slot\b' pyre/pyre-interpreter/src/typedef.rsRepository: youknowone/pyre
Length of output: 7405
🏁 Script executed:
#!/bin/bash
rg -n -C4 'fn lookup_in_type|lookup_in_type\(' pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
sed -n '1,260p' pyre/pyre-interpreter/src/baseobjspace.rsRepository: youknowone/pyre
Length of output: 9098
🏁 Script executed:
#!/bin/bash
ast-grep outline pyre/pyre-interpreter/src/baseobjspace.rs --items all --type function --match 'lookup_in_type'Repository: youknowone/pyre
Length of output: 552
🏁 Script executed:
#!/bin/bash
rg -n -C6 'lookup_in_type' pyre/pyre-interpreter/src/baseobjspace.rs pyre/pyre-interpreter/src/*.rsRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' pyre/pyre-interpreter/src/baseobjspace.rs | nl -ba | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 189
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path('pyre/pyre-interpreter/src/baseobjspace.rs')
text = p.read_text()
for needle in ['lookup_in_type', 'staticmethod', 'is_staticmethod', 'w_staticmethod_get_func']:
idx = text.find(needle)
print('NEEDLE', needle, 'IDX', idx)
PYRepository: youknowone/pyre
Length of output: 287
🏁 Script executed:
#!/bin/bash
rg -n -C8 'fn lookup_in_type|lookup_in_type\(' pyre/pyre-interpreter/src/baseobjspace.rs pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
rg -n -C6 'staticmethod|descriptor|lookup.*type|type lookup|lookup_in_type' pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
rg -n -C4 'staticmethod.*__new__|__new__.*staticmethod|object\(\)|takes exactly one argument|takes no arguments' pyre -g '!**/target/**'Repository: youknowone/pyre
Length of output: 50372
Use object_slot_inherited in the constructor gates. same_inherited_slot is still a raw identity check, so it misses staticmethod(...)-wrapped inherited slots and can reject valid object.__new__ / __init__ call forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/typedef.rs` around lines 14425 - 14440, The
constructor validation gates still use same_inherited_slot, which compares raw
identities and misses staticmethod-wrapped inherited slots. Replace those checks
with object_slot_inherited, preserving the existing object.__new__ and __init__
validation behavior while allowing equivalent staticmethod-wrapped slots.
Source: Coding guidelines
The comment attributed the real-opcode-past-prefix entry coordinate to jump_target_backward. Jump and loop-header targets point at the logical instruction start (the EXTENDED_ARG word): resolve_jump_offsets uses the instruction's i_offset, and write_instr emits the EXTENDED_ARG units before the real opcode. The real-opcode coordinate comes from skip_python_trivia_forward, which advances past EXTENDED_ARG as trivia in pyre's pc-indexed metadata keying. Assisted-by: Claude
youknowone
changed the title
Summary
This branch attempted the tagged small-int representation (immediate
(value << 1) | 1instead of a heapW_IntObject), then reverted the flip after an orthodoxy review found that tagging mainline ints is a pyre-specific deviation from PyPy (PyPy elides int allocation via virtualization/escape-analysis, andtaggedpointersdefaults off;W_IntObjectis a plain heap box, not anUnboxedValue).CAN_BE_TAGGEDis therefore back tofalse— net-unchanged fromorigin/main.What the branch delivers instead is the set of correctness fixes surfaced while pursuing the flip, all of which stand independently of it, plus the tagged-int scaffolding left inert behind the false flag (byte-identical to the flag-false representation; kept, not ripped out, to avoid unrequested churn).
What lands (independent of the flip)
Base correctness (crash/miscompile fixes that reproduce with the flag false too):
int: guard int_mod against i64::MIN % -1 overflowlist: clamp backwards slice assignment to an empty insertionbuiltins: stack-check isinstance/issubclass native recursionobject: raise TypeError from object.__new__/__init__ on bad argsinterp: guard recursive container repr with stack_check— deep dict/list/tuplereprraises RecursionError instead of overflowing the C stack.JIT correctness:
jit: short-preamble seed-preserve narrowed to allocation ops(3 commits) — fixes theforiter_call_body/store_global_hotgetintbound optimizer panic (E1/Task#4 short-preamble input/result key disjointness), without regressing the nested for-range merge point.interp: forward-onlydecode_instruction_forwardfor the hot dispatch path (gh#394)+ its prefix fix: back the decoder up over anEXTENDED_ARGprefix to the logical start before accumulating, so a JIT loop-header/resume coordinate that names the real opcode past its prefix (normalized throughskip_python_trivia_forward) recovers the full oparg. Fixes thegetwidthFOR_ITER miscompile (test_fnmatch/test_strtodJIT-on).Tag-safety hardening (inert with the flag false, byte-identical):
type_name_ofand 28+ tagged-reachable type-name deref routings; tagged prechecks inexception_descr_str_wtf8; iterator/is_list/is_set_iteratortag guards.The reverted flip (net-zero)
object: enable tagged small-int representation (CAN_BE_TAGGED=true)→object: disable … (CAN_BE_TAGGED=false).observed_slot_int_type, S3 symbolic type-map reset, vable-deopt per-slot re-box, Int virtual-state test) was excised — it post-dated the primitive scaffolding and had no RPython basis. Thetagged_int.rsprimitive scaffolding stays inert behind the false flag.Verification
check.py --backend dynasm,cranelift: dynasm 225/225 + cranelift 225/225 ALL PASSED, byte-identical both backends.cargo test --all(dynasm) + cranelift subset: green.test_fnmatch/test_strtodPASS.Orthodoxy note
Tagging mainline ints is closed as non-orthodox; it is not re-attempted. The primitive scaffolding is kept inert (removing it is unrequested churn), but the flag is never re-enabled and the per-slot Int machinery is gone.