fix(codegen): initialize typed-shape layout on the standalone-ctor exit (#6921) - #6930
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesTyped layout initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant lower_new_impl
participant TypedShapeLayout
participant GC
lower_new_impl->>TypedShapeLayout: initialize layout for unconstructed instance
TypedShapeLayout->>GC: publish descriptor or conservative layout state
GC->>GC: trace published pointer slots
Possibly related issues
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/perry-runtime/src/gc/tests/layout_trace.rs`:
- Around line 1765-1790: Update the test around the child allocation and trace
call to root both ptr_only and child with RuntimeHandleScope. After
js_string_from_bytes and again after trace_marked_objects, reload both handles
before using them; derive child_header from the refreshed child handle and use
refreshed ptr_only when writing the slot.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da142ed0-738f-4c7d-b95d-028e875498ea
📒 Files selected for processing (3)
changelog.d/6930-typed-shape-layout-ctor-exit.mdcrates/perry-codegen/src/lower_call/new.rscrates/perry-runtime/src/gc/tests/layout_trace.rs
| // The child is reachable ONLY through that slot; write it with a raw store | ||
| // so no `layout_note_slot` runs, then prove tracing still finds it. | ||
| let child = crate::string::js_string_from_bytes(b"6921-child".as_ptr(), 10); | ||
| let child_header = unsafe { header_from_user_ptr(child as *mut u8) }; | ||
| let fields = unsafe { | ||
| (ptr_only as *mut u8).add(std::mem::size_of::<crate::object::ObjectHeader>()) as *mut u64 | ||
| }; | ||
| unsafe { | ||
| std::ptr::write(fields, STRING_TAG | (child as u64 & POINTER_MASK)); | ||
| } | ||
|
|
||
| let valid_ptrs = build_valid_pointer_set(); | ||
| let parent_bits = POINTER_TAG | (ptr_only as u64 & POINTER_MASK); | ||
| assert!( | ||
| try_mark_value(parent_bits, &valid_ptrs), | ||
| "test setup: the instance marks as a root" | ||
| ); | ||
| trace_marked_objects(&valid_ptrs); | ||
| unsafe { | ||
| assert_ne!( | ||
| (*child_header).gc_flags & GC_FLAG_MARKED, | ||
| 0, | ||
| "the typed descriptor alone must make the pointer slot traceable — \ | ||
| this is the liveness `layout_note_slot` would otherwise have had \ | ||
| to establish store-by-store" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline crates/perry-runtime/src/gc/tests/layout_trace.rs \
--match typed_shape_layout_init_on_unconstructed_instance_is_conservative \
--view expanded
rg -n -C 3 'RuntimeHandleScope' crates/perry-runtime/src
rg -n -C 3 'fn js_string_from_bytes|trace_marked_objects' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline target function =="
ast-grep outline crates/perry-runtime/src/gc/tests/layout_trace.rs \
--match typed_shape_layout_init_on_unconstructed_instance_is_conservative \
--view expanded || true
echo
echo "== target section =="
sed -n '1700,1815p' crates/perry-runtime/src/gc/tests/layout_trace.rs
echo
echo "== RuntimeHandleScope definition/refreshed APIs =="
rg -n "enum RuntimeHandleScope|struct RuntimeHandleScope|impl RuntimeHandleScope|root_raw_mut_ptr|root_raw_const_ptr|root_nanbox_f64|refreshed_nanbox_f64_slice|get_raw_mut_ptr|get_raw_const_ptr|get_" crates/perry-runtime/src/gc -g '*.rs' | head -200
echo
echo "== js_string_from_bytes and header_from_user_ptr =="
rg -n -C 4 "fn js_string_from_bytes|unsafe fn js_string_from_bytes|fn header_from_user_ptr|unsafe fn header_from_user_ptr|header_from_user_ptr\\(" crates/perry-runtime/src -g '*.rs'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RuntimeHandleScope exact source =="
cat -n crates/perry-runtime/src/gc/roots/runtime_handles.rs | sed -n '1,250p'
echo
echo "== copy/evacuation relocation methods =="
rg -n -C 3 "fn.*forward|forwarding|rewrite|rewrite|evacuat|copy.*object|copy_one|swap|write_barrier|maybe_rewrite|rewrite.*heap" crates/perry-runtime/src/gc/copying.rs crates/perry-runtime/src/gc -g '*.rs' | head -240
echo
echo "== string allocation =="
cat -n crates/perry-runtime/src/string/alloc.rs | sed -n '75,115p'Repository: PerryTS/perry
Length of output: 32037
Root both heap objects across the allocation and tracing boundaries.
js_string_from_bytes() can allocate, and trace_marked_objects() relocates marked nursery objects. Keep ptr_only and child in RuntimeHandleScope, reload them before writing the child slot after allocation and before reading its flags after tracing, and derive child_header from the refreshed child handle.
🤖 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 `@crates/perry-runtime/src/gc/tests/layout_trace.rs` around lines 1765 - 1790,
Update the test around the child allocation and trace call to root both ptr_only
and child with RuntimeHandleScope. After js_string_from_bytes and again after
trace_marked_objects, reload both handles before using them; derive child_header
from the refreshed child handle and use refreshed ptr_only when writing the
slot.
Sources: Coding guidelines, Learnings
…it (#6921) `lower_new_impl` had one exit that returned a freshly allocated class instance without emitting `js_gc_init_typed_shape_layout`. Every other `new` exit emits it. An instance produced there is left at `GC_LAYOUT_POINTER_FREE` with no `TypedLayoutDescriptor` — the one layout state where the per-store `layout_note_slot` call is load-bearing for GC correctness rather than a precision hint, because it is the only writer of the pointer-mask bit the collector reads. That blocks the full pointer-masked layout-note elision (the larger half of Phase 4b.1 in #6919, which had to narrow to the value-only predicate because of it). Emit the layout init on that exit too, so the invariant "a user-class instance reaching a class-field store carries a typed descriptor, or is explicitly GC_LAYOUT_UNKNOWN" is total. REACHABILITY — I could not build a reproducer, and on investigation the arm appears to be DEAD, which is a stronger claim than #6921 makes. The issue correctly notes that `force_ctor_call` and `ctor_alias_collision` both pre-check `local_constructor_symbol_exists` and so cannot reach it, leaving the `ctx.class_stack` recursion guard as the only way in. But the missing step is that `call_local_constructor_symbol` returns `None` only when `ctx.methods` lacks `(class.name, "<Class>_constructor")`, and: - `lower_new_impl` resolves `class` exclusively from `ctx.classes` (new.rs:237 is the only binding, `ctx.classes.get(class_name)`); - `ctx.classes` IS the `class_table` (codegen/mod.rs passes `&class_table` to compile_function / compile_method); - `build_method_names` iterates `class_table.values()` and inserts `(c.name, "{c.name}_constructor")` UNCONDITIONALLY for every entry, local and imported alike (method_registry.rs:112-119). So every class that can reach the branch has the key, and the `None` arm cannot be taken. Empirically: an instrumented compiler that logs on reaching the arm reported ZERO hits across all 430 `test-files/test_gap_*.ts` (codegen-only, `--no-link --no-cache`) plus three hand-written self-referential construction shapes (self-construction in a method, a recursive own constructor, and a field initializer constructing its own class) — all of which do enter the recursion-guarded branch, and all of which take the `Some` arm that already emitted the init. The emitter lands anyway: the invariant should hold by construction at this exit rather than by an accident of the registry that a change to `build_method_names`, or a new `ctx.classes` population path, could silently revoke. Cost is zero — dead path today, and `emit_typed_shape_layout_init` is itself a no-op for a class with no `class_keys_globals` entry. Because the arm is unreachable, NO behavioral test can fail before and pass after; claiming otherwise would be dishonest. What is testable, and what actually carries the risk, is the premise the fix rests on: the instance reaching this exit has had NO constructor run, so the layout init sees all-`undefined` fields. `gc::tests::layout_trace:: typed_shape_layout_init_on_unconstructed_instance_is_conservative` pins all three properties of that: 1. `js_object_alloc_class_inline_keys` really does leave the instance at GC_LAYOUT_POINTER_FREE with no descriptor (why the note is load-bearing there, i.e. why the gap mattered); 2. a raw-f64 mask over `undefined` fields is REFUSED and the object is downgraded to GC_LAYOUT_UNKNOWN — the conservative state — never left POINTER_FREE and never given a mask that misdescribes it (why emitting the init here is SAFE); 3. a pointer-only mask over `undefined` fields IS installed, and a subsequent raw pointer store into that slot is traced with no `layout_note_slot` call at all (why emitting it is USEFUL). Verified green under the full GC stress matrix: PERRY_GC_FORCE_EVACUATE=1, PERRY_GC_VERIFY_EVACUATION=1, PERRY_GEN_GC=0, PERRY_WRITE_BARRIERS=0, and four combinations including all-four-at-once.
CodeRabbit review on #6930. The test held `ptr_only` and `child` as raw pointers across GC-capable calls — `js_string_from_bytes` allocates, and evacuation moves arena objects, so the subsequent raw field write and the `POINTER_TAG | ptr_only` mark could both have named from-space. This is the same unrooted-handle-across-allocation bug class as #6655, in a test whose whole job is to validate GC correctness. An unsound test is worse than no test: it can pass for the wrong reason and mask exactly the layout/tracing behavior it exists to pin. Fixed properly rather than minimally: - every instance is rooted in a `RuntimeHandleScope` the moment it is allocated, and re-read through the handle after any later allocation (`handle_user`), so no raw pointer crosses a GC point; - `child_header` is derived from the refreshed child handle at the point of use instead of being captured before the trace; - the hand-driven mark starts from `clear_marks()` so a collection during the allocations cannot leave the instance pre-marked and turn the `try_mark_value` setup assertion into a false negative. The registration is the part that is easy to get wrong, so it is now explicit and documented: `GcTestIsolationGuard` (via `ScopedRootScannerRegistryGuard`) `mem::take`s the thread's `MUTABLE_ROOT_SCANNERS`, and the runtime-handle scanner goes with it. A `RuntimeHandleScope` opened inside that guard roots NOTHING until the scanner is put back. `register_runtime_handle_root_scanner_for_tests` moves from `tests/runtime_roots.rs` to `tests/support.rs` (21 call sites there are unaffected — it is glob-imported) and carries a doc comment spelling out the trap. Crucially, the rooting is now LOAD-BEARING rather than decorative. The test drives a real `collect_minor_trace(GcTriggerKind::Direct)` after the child write, with `ConservativeScanDisabledGuard` pinning the native-stack scan OFF — so the raw Rust locals are no longer a safety net and the handle scope is the only thing keeping the objects alive. Verified: with the `register_runtime_handle_root_scanner_for_tests()` line removed the test FAILS (`test_layout_pointer_slot_count` → `None` — the instance and its descriptor were reclaimed); with it, it passes. That collection also buys a new assertion worth having: the typed descriptor must survive the collection and any relocation, since the note-elision premise depends on it staying intact, not merely on being installed once. Stress matrix re-run, 9/9 green: PERRY_GC_FORCE_EVACUATE=1, PERRY_GC_VERIFY_EVACUATION=1, PERRY_GEN_GC=0, PERRY_WRITE_BARRIERS=0 and four combinations including all-four-at-once. `gc::tests::layout_trace` + `gc::tests::runtime_roots` together: 113 passed, 0 failed. (Widening a stress arm to the whole `runtime_roots` suite surfaces 34 failures under `PERRY_GEN_GC=0` / `PERRY_WRITE_BARRIERS=0` — all pre-existing `*_copied_minor_gc` tests that require the copying nursery, which is ineligible without generational GC and barriers. None of them are tests touched here.)
0fbddc7 to
96e10b8
Compare
|
Rebased onto
|
| state | result |
|---|---|
register_runtime_handle_root_scanner_for_tests() removed |
FAILS — test_layout_pointer_slot_count → None: instance and descriptor reclaimed |
| as committed | passes |
That collection also buys a genuinely useful new assertion: the typed descriptor must survive the collection and any relocation, since the note-elision premise depends on the descriptor staying intact, not merely on it having been installed once.
Audit of the other new tests
Checked all of them for the same pattern, as asked.
This PR — typed_shape_layout_init_on_unconstructed_instance_is_conservative was the only one, now fixed.
#6929's tests (already on main) — one genuine hazard and two brittle-but-currently-sound spots. Filed as a separate follow-up PR against main rather than smuggled in here:
mutable_root_mark_and_rewrite_accept_the_same_word_forms— real hazard.mark_target/rewrite_targetarearena_alloc_gcobjects held as rawusizeacross two further allocations (rewrite_target, thenmovedviaarena_alloc_gc_old), and the test has no trigger suppression. Note that rooting alone would not have been enough here: the test hand-builds aValidPointerSetsnapshot and hand-sets a forwarding address, both of which a real collection would invalidate regardless of liveness. The fix is trigger suppression (keeps the hand-built snapshot coherent) plus rooting.bare_address_in_shadow_slot_survives_a_real_collectionandbare_address_in_global_root_survives_a_real_collection— sound today, brittle.liveisgc_malloc-backed, and the copying collector marks malloc objects in place and returns the address unchanged (copying.rs:466,CopyingPointerKind::Malloc => Some(addr)), so it never relocates; triggers are suppressed and the collection is driven explicitly. They pass for a correct reason, not by luck. Still worth reloading the address from the root slot after the cycle — it removes the dependence on that policy and is a stronger assertion, since it also proves the slot itself was maintained.decode_root_word_round_trips_each_representation— pure bit math on synthetic addresses, no allocation, nothing to root.
Verification
Stress matrix re-run on the reworked test, 9/9 green: PERRY_GC_FORCE_EVACUATE=1, PERRY_GC_VERIFY_EVACUATION=1, PERRY_GEN_GC=0, PERRY_WRITE_BARRIERS=0, plus FORCE+VERIFY, GEN_GC=0+VERIFY, BARRIERS=0+FORCE, and all four at once.
gc::tests::layout_trace + gc::tests::runtime_roots: 113 passed, 0 failed. One thing worth recording so nobody re-derives it: widening a stress arm to the whole runtime_roots suite shows 34 failures under PERRY_GEN_GC=0 / PERRY_WRITE_BARRIERS=0. Those are all pre-existing *_copied_minor_gc tests — the copying nursery is ineligible without generational GC and write barriers, so those arms are expected-red for that suite. None of the tests touched here are among them.
Gap sweep still not run (shared box: disk under the 25 GB floor, load average 56); CI adjudicates.
* test(gc): handle hygiene in the #6910 root-word tests Follow-up to #6929, prompted by a CodeRabbit finding on the sibling PR #6930: a GC test that holds raw pointers across GC-capable calls is the #6655 bug class inside the very tests meant to validate GC correctness. Audited all four tests landed in #6929; one had a genuine hazard and two were sound-but-brittle. 1. `mutable_root_mark_and_rewrite_accept_the_same_word_forms` — REAL hazard. `mark_target` / `rewrite_target` are `arena_alloc_gc` objects held as raw `usize` across two further allocations (`rewrite_target`, then `moved` via `arena_alloc_gc_old`), with no trigger suppression. Evacuation moves arena objects, so either could have named from-space by the time the walks ran. Fixed with `GcTriggerThresholdTestGuard::suppress_automatic_triggers()` rather than a `RuntimeHandleScope`, and the comment says why: rooting alone would NOT have been sufficient here. This test hand-builds a `ValidPointerSet` snapshot and hand-sets a forwarding address, neither of which survives a real collection no matter how well the objects are rooted. The hazard is the collection itself, so the region is made collection-free. Also moved `build_valid_pointer_set()` after the last allocation so the snapshot describes the heap the walks actually run against. Behaviour is unchanged — `try_rewrite_raw_addr` returns `Some(moved)` whether or not `moved` is in the set (it stops at the first non-forwarded header either way) — but "snapshot the heap, then allocate into it" was misleading. 2/3. `bare_address_in_shadow_slot_survives_a_real_collection` and `bare_address_in_global_root_survives_a_real_collection` — sound today, brittle. `live` is `gc_malloc`-backed and the collector marks malloc objects in place (`copying.rs:466`, `CopyingPointerKind::Malloc => Some(addr)`), never relocating them; triggers are already suppressed and the collection is driven explicitly. They passed for a correct reason, not by luck. Still changed to read the survivor back out of the ROOT SLOT (`js_shadow_slot_get(0)` / `global_slot`) instead of reusing the raw pointer captured before the cycle. That removes the dependence on the malloc-never-moves policy and is the stronger assertion anyway: it also proves the slot itself was maintained across the collection. The global case additionally gains the closure-magic intactness check the shadow case already had. 4. `decode_root_word_round_trips_each_representation` — pure bit math over synthetic addresses, no allocation, nothing to root. Unchanged. Detection power verified intact, not just preserved: with `mark_mutable_root_bits` temporarily reverted to the pre-#6910 NaN-box-only mark, the hardened tests still fail with the original signature ("shadow-stack slot: mark walk disagreed with the contract for bare address"). (Three tests go red under that crude revert rather than two — but only because collapsing `mark_mutable_root_bits` also strips the bare acceptance module-variable globals have always had, which the real pre-fix `mark_global_root_bits` retained. The global-root test remains a refactor guard, not a #6910 regression test.) Stress matrix 9/9 green: PERRY_GC_FORCE_EVACUATE=1, PERRY_GC_VERIFY_EVACUATION=1, PERRY_GEN_GC=0, PERRY_WRITE_BARRIERS=0 and four combinations including all-four-at-once. Tests only — no runtime or codegen change. * docs(changelog): fragment for #6932 (root-word test handle hygiene) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Closes #6921.
Split out from the #6910 GC-hardening work (PR #6929) because the risk profiles are opposite: #6929 changes the GC mark path taken on every collection and needs real scrutiny; this one changes a codegen path that — as detailed below — appears to be unreachable today, so it has no behavioral effect and its value is entirely in making an invariant total for a future optimization. Bundling them would have let a discussion about this arm hold up a soundness fix.
The gap
lower_new_implhas one exit that returns a freshly allocated class instance without emittingjs_gc_init_typed_shape_layout. Every othernewexit emits it. An instance produced there is left atGC_LAYOUT_POINTER_FREEwith noTypedLayoutDescriptor— the one layout state where the per-storelayout_note_slotcall is load-bearing for GC correctness rather than a precision hint, because it is the only writer of the pointer-mask bit the collector reads. With a descriptor installed the note is a pure no-op for pointer-masked slots; without one, dropping it would leave the collector scanning zero slots on an object that holds live children.That is what blocks the full pointer-masked layout-note elision — the larger half of Phase 4b.1 in #6919, which had to narrow to the value-only predicate because this exit made the invariant "a user-class instance reaching a class-field store carries a typed descriptor, or is explicitly
GC_LAYOUT_UNKNOWN" untrue in principle.Reachability: I could not build a reproducer, and the arm looks dead
Stating this plainly because the issue asked for a reproducer and I do not have one — and because what I found is a stronger claim than the issue makes, which is worth recording either way.
The issue's analysis is right as far as it goes:
force_ctor_callandctor_alias_collisionboth pre-checklocal_constructor_symbol_exists, so neither can reach theNonearm, leaving thectx.class_stackrecursion guard as the only way in. The missing step is what makes the symbol exist.call_local_constructor_symbolreturnsNoneonly whenctx.methodslacks(class.name, "<Class>_constructor"), and:lower_new_implresolvesclassexclusively fromctx.classes—new.rs:237,ctx.classes.get(class_name).copied(), the only binding ofclassin the function;ctx.classesis theclass_table(codegen/mod.rspasses&class_tableintocompile_function/compile_method/compile_entry);build_method_namesiteratesclass_table.values()and inserts(c.name, "{c.name}_constructor")unconditionally for every entry — local and imported alike (method_registry.rs:112-119).So every class that can reach the branch has the key, and the
Nonearm cannot be taken.Empirically, an instrumented compiler that logs on reaching the arm reported zero hits across:
test-files/test_gap_*.ts(codegen-only,--no-link --no-cacheso codegen actually runs for each);new Tree(depth - 1)insideTree's ctor), and a field initializer constructing its own class.All three shapes do enter the recursion-guarded branch —
compile_methodseedsclass_stack: vec![class.name], so anynew C()inside a method ofClands there — and all three take theSomearm, which already emitted the init.Why land it anyway
The invariant should hold by construction at this exit, not by an accident of the registry that a change to
build_method_names, or a newctx.classespopulation path, could silently revoke. Cost is zero: the path is dead today, andemit_typed_shape_layout_initis itself a no-op for a class with noclass_keys_globalsentry.Testing: what is honest here
Because the arm is unreachable, no behavioral test can fail before and pass after. I am not going to dress one up.
What is testable, and what actually carries the risk, is the premise the fix rests on: the instance reaching this exit has had no constructor run, so the layout init sees all-
undefinedfields. Ifinit_typed_shape_layoutpromoted on that input, this change would be actively harmful.gc::tests::layout_trace::typed_shape_layout_init_on_unconstructed_instance_is_conservativepins all three properties:js_object_alloc_class_inline_keysreally does leave the instance atGC_LAYOUT_POINTER_FREEwith no descriptor — establishing why the note is load-bearing there, i.e. why the gap mattered at all;undefinedfields is refused and the object downgraded toGC_LAYOUT_UNKNOWN, the conservative state — never leftPOINTER_FREE, never given a mask that misdescribes the live words. This is why emitting the init here is safe;undefinedfields is installed, and a subsequent raw pointer store into that slot is traced with nolayout_note_slotcall at all. This is why emitting it is useful.Green across the full GC stress matrix:
PERRY_GC_FORCE_EVACUATE=1,PERRY_GC_VERIFY_EVACUATION=1,PERRY_GEN_GC=0,PERRY_WRITE_BARRIERS=0, and four combinations including all four at once.cargo fmt --all --checkclean;scripts/check_file_size.shgains no new offenders.Not run: gap suite
Not run here either, for the same reason as #6929 — shared box, 13 GB free against a 25 GB floor, load average 56. Given that this PR is a no-op on every input I could construct or find in the corpus, the gap risk is as close to nil as a codegen change gets; CI's gates should confirm cheaply.
Follow-up worth considering (not done here)
If reviewers agree the arm is dead, the cleaner end state is to make that structural — hoist the layout init so the branch has a single exit, or have
call_local_constructor_symbolreturn a type that cannot beNonefor aclass_table-resolved class. I did not do either: both are refactors with more blast radius than the one-line emitter, and I would rather land the safe version than gamble on a restructure ofnewlowering. Anunreachable!()would be worse — a panic in the compiler if my analysis is wrong somewhere I did not look.Summary by CodeRabbit