review: act on the #904 findings, and scope the descr type_id split out - #921
Conversation
…ebug builds The cache-hit path derives `expected_index_in_parent` by walking the parent SizeDescr's whole `all_fielddescrs()` list and comparing field keys. Its only reader is the `debug_assert!` directly below, which `cfg!(debug_assertions)` compiles away, so a release build performed the walk and discarded the result. The walk is not off the hot path: `make_simple_descr_group_keyed_with_headerless` calls `get_field_descr` once per field spec while holding the `gc_cache` lock, and every field after the first publication is a cache hit. Spells the same `cfg!(debug_assertions)` gate out at the binding, so a release build reads the cached descr's own slot number and skips the walk. Assisted-by: Claude
`jtransform`'s malloc + `__discriminant` lowering gated on
`owner_path.last().starts_with("Result")` plus a `matches!(name, "Ok" | "Err")`
test. The leaf test also accepts an enum whose name merely begins with the
literal — `ResultCode`, say — and such an enum with `Ok`/`Err` variants would be
rerouted through the Result-specific lowering. No name in the current LLBC
corpus does that.
`front::result_exc::result_ctor_kind` already decides the same question and
decides it precisely: it anchors the owner path head at `core::result` and
compares the instantiation-stripped leaf for equality. Reuses it here rather
than tightening the second spelling test, so the two gates cannot drift.
Assisted-by: Claude
`drain_unfinished_graphs` recorded `ConstEncodingOverflow` into `CallControl.graph_jit_shapes` as the bare literal `3`, and `cached_unsupported_jit_shape` decoded it with a hand-written match over the literals `0..=3`. Both numberings only track `UnsupportedJitShape`'s implicit discriminants by hand, in two files, with nothing linking them: renumbering, inserting or removing a variant misclassifies every cached frame shape with no build error and no test signal. Adds `call::JIT_SHAPE_CONST_ENCODING_OVERFLOW` beside the map, used at the write site, and pins it to the enum in `eval.rs` with `const _: () = assert!(..)` — the only module that sees both. Names the decode arms after the discriminants themselves so the read side carries no literals either. The written value and the decode are unchanged. Assisted-by: Claude
`test_guard_fail_args_virtual_array_with_nested_virtual_item` ended at `virtuals.len() >= 2`, which also holds when the array and the item are numbered as two unrelated virtuals. What the bridge decoder follows is the link between them: it reads the array's item slot, resolves that TAGVIRTUAL number back into `rd_virtuals` under the negative-index rule, and materializes the entry it lands on. Asserts that path instead: the array is located by its `VArrayInfo*` variant rather than by slot order, its single item slot must untag to TAGVIRTUAL, the slot that resolves to must be a different one and hold the nested `VirtualInfo`, and that virtual's one live field slot must be `field_descr(42)` carrying a TAGBOX that resolves to the `SETFIELD_GC` value `i40`. Both descr references are required to be live, since a `None` on either silently drops the materialization. Passes on the current encoding unchanged. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThe changes optimize debug-only descriptor validation, strengthen nested virtual-array resume-data checks, use exact Result constructor resolution, and replace JIT shape discriminant literals with shared constants and compile-time validation. ChangesMAJIT correctness
Result constructor lowering
JIT shape encoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 1281d0e). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
1 participant
Acts on the code review left on #904, after verifying each finding against post-merge
mainrather than against the PR diff.Twelve findings were filed (1 by Codex, 11 by CodeRabbit). Five did not survive verification, two are style suggestions whose advice does not apply, one is epic-sized and is deliberately left alone. The four here are the ones worth landing.
What lands
get_field_descrcache hit: gate the index arbitration ondebug_assertions.The hit path derived
expected_index_in_parentby walking the parent SizeDescr's wholeall_fielddescrs()list and comparing field keys. Its only reader is thedebug_assert!directly below it, whichcfg!(debug_assertions)compiles away — so release builds performed the walk and discarded the result.make_simple_descr_group_keyed_with_headerlesscallsget_field_descronce per field spec while holding thegc_cachelock, and every field after the first publication is a cache hit, so this is not off the hot path.Recognize the
Resultctor through the front-side rule.jtransform's malloc +__discriminantlowering gated onowner_path.last().starts_with("Result"). That also accepts an enum whose leaf merely begins with the literal —ResultCode, say — which, withOk/Errvariants, would be rerouted through theResult-specific lowering. No name in the current LLBC corpus does that.front::result_exc::result_ctor_kindalready decides the same question precisely: it anchors the owner path head atcore::resultand compares the instantiation-stripped leaf for equality. Reuses it, so the two gates cannot drift.Name the
graph_jit_shapesdiscriminant.drain_unfinished_graphsrecordedConstEncodingOverflowas the bare literal3;cached_unsupported_jit_shapedecoded it with a hand-written match over0..=3. Both numberings trackedUnsupportedJitShape's implicit discriminants by hand, in two files, with nothing linking them — renumbering or removing a variant misclassifies every cached frame shape with no build error and no test signal. Addscall::JIT_SHAPE_CONST_ENCODING_OVERFLOWbeside the map, pins it to the enum ineval.rswithconst _: () = assert!(..), and names the decode arms after the discriminants themselves. Written value and decode unchanged.Assert the nested virtual's link, not the
rd_virtualscount.test_guard_fail_args_virtual_array_with_nested_virtual_itemended atvirtuals.len() >= 2, which also holds when the array and the item are numbered as two unrelated virtuals. What the bridge decoder follows is the link: it reads the array's item slot, resolves that TAGVIRTUAL number back intord_virtualsunder the negative-index rule, and materializes the entry it lands on. The test now asserts that path — locating the array by itsVArrayInfo*variant rather than by slot order, requiring the resolved slot to be a different one holding the nestedVirtualInfo, and requiring its one live field slot to befield_descr(42)carrying a TAGBOX that resolves to theSETFIELD_GCvaluei40. Both descr references must be live, since aNoneon either silently drops the materialization. Passes on the current encoding unchanged.Deliberately not fixed
CodeRabbit filed the
fielddescroftest gap as a trivial coverage nit. Verifying it surfaced something larger: the SizeDescrtype_idis minted two ways.OpKind::New/OpKind::NewWithVtableship the first verbatim. The tell is inside that function: it fetches the layout throughcc.struct_layout_for, which isstruct_id_for_name(name)? -> struct_layouts[sid], and then stamps a different identity onto that layout."W_FloatObject"alone splits intopath_hash("W_FloatObject")versuspath_hash("floatobject::W_FloatObject"). ANewWithVtableand the payloadsetfield_gcemitted one instruction later on the same object therefore mint two disjointArc<SimpleFieldDescr>sets for the same offsets — andOptHeapkeyscached_fieldson Arc identity, so the store does not invalidate the load's cache slot.Converging the keys is not inert, which is why it is not in this PR: it moves every
Newspec onto the same_cache_sizekey the runtime group already occupies, activatingregister_keyed_sizearbitration where it is dormant today. That tiebreak is vtable wins, else more fields wins, and the analyzer's list is longer by exactly the GC-header words — so aNewWithVtablespec would win and replace the orthodox headerless runtime group. This belongs with the header-convention unification, as its own change with a full gate.Findings that did not survive
Codex's P1 asked to keep the nested-gateway
inline_subwalkpath disabled, on the evidence thatsynth/sre_pattern_methodsprinted279999instead of280000. That benchmark now passes — but it passes because the branch was rebased onto amaincarrying #874/#905/#906/#907, not because the blamed commits were reverted, so "fixed" and "masked" had to be separated. The path is unreachable:compute_inline_caller_framehas exactly two callers, both undertry_walker_inline_user_call, which declines unlesspyre_helperisCallFn/CallKw/CallFunctionEx;PyreHelperKind::CallFnis assigned only inpyre-jit/src/jit/flatten.rs, whilemajit-translate/src/codewriter/call.rs— which builds the gateway body's residuals — writesPyreHelperKind::Noneat all three of its construction sites.One residue is worth recording: what makes that safe today is an unrelated helper-kind filter, not a check at the resolution site. The sibling guard-capture path in
resume_snapshot.rsgates the same hazard explicitly (if !inline_subwalk && !full_body_sym.is_null()), andcompute_inline_caller_framemaps the same kind ofop_pcwithout it; upstream traps the class outright atpyjitpl.py:199.Four more were misreads:
Descr::index()andFieldDescr::index_in_parent()are different axes, not a mismatched identity; the_helper_frameguard does outlive thefbw_moderestore but no call in that tail readsframestack.last();float.as_integer_ratio(1)already raisesTypeErrorbecausebuiltin_code_callrejects any positional count differing from the recorded arity, in both directions; and no ruff configuration coverspyre/bench/synth, so B007 never runs there.Verification
cargo test --all --no-default-features --features dynasm— exit 0, 100 test binaries, no failures.python3 pyre/check.py --backend dynasm— 349/349.Both against a freshly extracted LLBC whose
pyre-object/pyre-interpreter/pyre-jitfingerprints match the working tree, run at base59d87af4d9. The base has since moved by one commit (#920,optimizeopt/virtualize.rs);cargo test -p majit-metainterp --libwas re-run on the current base — 1434 passed.— authored by Claude
Summary by CodeRabbit
Bug Fixes
Resultvariants so unrelated enum types are no longer misidentified asOkorErr.Tests