docs(gc): correct the dominance direction, the shape summary, and the allowlist fingerprint; restore the workspace version - #7224
Open
jdalton wants to merge 1 commit into
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 (4)
📝 WalkthroughWalkthroughThe pull request refines GC root-dominance validation, allowlist reporting, and self-test behavior. It also updates documentation for dominance rules, moving collections, stale-register coverage, rooting API semantics, and fingerprint selection. ChangesGC root-dominance analysis
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CLAUDE.md`:
- Line 253: Update the invariant documentation statement around the corpus and
PERRY_GC_MOVING_LOOP_POLLS to say the flag expands coverage for loop-back-edge
safepoints only, not that it is required to express every rooting bug.
Separately document that allocating runtime helpers can provide collection
points without the flag, preserving the distinction already established near
CLAUDE.md’s root-store dominance guidance.
In `@docs/src/internals/memory-model.md`:
- Around line 139-140: Replace the introductory caveat sentence in the
memory-model documentation with “These instruments have explicit caveats,
because each has burned a prior investigation:”.
In `@docs/src/internals/rfc-rooting-by-construction.md`:
- Around line 222-233: Correct the variance description in the discussion of
PhantomData<&'e Emitter>: state that Raw<'e> is covariant in 'e, allowing a
longer lifetime to be shortened. Preserve the explanation that this lifetime
covariance does not brand the emitter instance, so a Raw from one emitter can be
accepted with a different &mut Emitter; update the corresponding provenance-fix
wording only as needed for consistency.
🪄 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: d71323fd-897d-4a53-8ce9-1a915c765c6e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CLAUDE.mdCargo.tomldocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mddocs/src/internals/rfc-rooting-by-construction.mdscripts/gc_root_dominance_allowlist.jsonscripts/gc_root_dominance_check.py
jdalton
force-pushed
the
fix/gc-review-followups-7196-7212
branch
from
August 1, 2026 23:42
5542d9e to
6a41191
Compare
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 `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 162-169: Update the paragraph around js_gc_loop_safepoint to state
that it is the only poll on a loop back edge, rather than the only in-loop
collection point. Preserve the clarification that POLL_CAPABLE_RUNTIME helpers,
including js_object_set_field_by_name, js_object_get_property, and
js_call_function, can still collect within loop bodies.
🪄 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: 244f88a6-1618-4620-8b46-1411773c1d57
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CLAUDE.mdCargo.tomldocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mddocs/src/internals/rfc-rooting-by-construction.mdscripts/gc_root_dominance_allowlist.jsonscripts/gc_root_dominance_check.py
🚧 Files skipped from review as they are similar to previous changes (5)
- Cargo.toml
- scripts/gc_root_dominance_allowlist.json
- CLAUDE.md
- docs/src/internals/rfc-rooting-by-construction.md
- scripts/gc_root_dominance_check.py
jdalton
force-pushed
the
fix/gc-review-followups-7196-7212
branch
from
August 2, 2026 00:03
6a41191 to
faf67ae
Compare
… allowlist fingerprint; restore the workspace version Follow-ups to the review threads on PerryTS#7196 and PerryTS#7212, both of which merged before the findings were worked through. Cargo.toml — PerryTS#7196 set `[workspace.package].version` back to 0.5.1277, undoing the 0.5.1278 bump PerryTS#7199 had landed four commits earlier. Nothing since has touched the line, so main is currently shipping a version number it already released. Restored. docs/src/internals/gc-rooting-invariant.md — the checker description read "reports any root store that does not dominate a preceding collection point", which is vacuous: a store can never dominate anything that precedes it, so the sentence is true of every store in the program. What the checker actually reports is the collection point — `window_hits(origin, bind)` collects the calls that can run between the instruction producing a GC value and the `js_shadow_slot_bind` that publishes it. Reworded to name the collection point as the reported object and to keep the true relation (the root store must dominate it), which is the rule stated at the top of the same page. Also noted that the gate command shown does not pass `--stale-registers`, so cases 3 and 4 only surface when it is run by hand. CLAUDE.md — the shape summary claimed all three failures present as "a rooted slot holding a dangling pointer", contradicting its own two preceding clauses: PerryTS#7184's bind is a silent no-op so nothing is bound, and the `alloca_entry` shape is never a slot at all. Split per shape. It also said the class "only bites under PERRY_GC_MOVING_LOOP_POLLS=1"; the checker's own POLL_CAPABLE_RUNTIME set treats js_object_set_field_by_name, js_object_get_property and js_call_function as moving-capable with no poll involved, and PerryTS#7211's allowlist entry is exactly such a window. Polls widen in-loop coverage; they are not a precondition. docs/src/internals/rfc-rooting-by-construction.md — `Plain(String)` cannot be `Copy`; the migration section costed it as if it were. It is `Clone`, which still imposes nothing on the caller. Added the emitter/frame branding gap to "What it cannot catch": PhantomData<&'e Emitter> records a lifetime, not an instance, and `Rooted` carries a bare SlotIdx, so the design as written catches ordering mistakes but not provenance ones. scripts/gc_root_dominance_allowlist.json — the fingerprint format line said "<first collector>". It is `sorted(set(callees))[0]`, the alphabetically first, not the first in program order. A hand-derived entry that guesses program order matches nothing, and an entry that matches nothing fails the build — so the misleading line pointed straight at a red gate. scripts/gc_root_dominance_check.py, three surgical changes: * the self-test failure text described sinking the root store; `_mutate` splices _SEED_CALL above the store and moves nothing. * `--stale-registers` returned before the `--unrooted-allocas` block, so passing both ran one pass and silently skipped the other. Now an argparse error, matching the --max-stale and --fatal-sinks guards directly above it. * the stale-allowlist-entry report returned 2 before the uncovered- violation report could run. Fix one violation and introduce another in the same PR and the log showed only the bookkeeping problem. Both reports now print; the exit code is unchanged (2 when an entry is stale, 1 when only uncovered violations remain). Verified: `--self-test` OK. Against the parent, a corpus with one stale entry and two uncovered violations printed only the stale entry and hid both violations; it now prints all three and still exits 2. Refs PerryTS#7196, PerryTS#7212, PerryTS#7199, PerryTS#7211.
jdalton
force-pushed
the
fix/gc-review-followups-7196-7212
branch
from
August 2, 2026 00:57
faf67ae to
3bdc1a8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to the review threads on #7196 and #7212. Both PRs merged before their findings were worked through, so these could not be amended onto the original branches.
Every item below was verified against
mainbefore it was changed. Two findings were refuted rather than applied, and one was refuted in the direction it proposed while still being a real defect — details in the threads.Release metadata regressed on main (the urgent part)
#7196 merged as
91170973ccarrying a downgrade of the maintainer-owned version, in two places:Cargo.toml[workspace.package].version0.5.12770.5.1278CLAUDE.mdCurrent Version0.5.12770.5.1278git log -L295,295:Cargo.tomlshows #7199 (97e843f62) bumping to0.5.1278four commits earlier, and nothing touching either line since. This is a straight revert of an already-released number, not a stale diff.Cargo.lockmoves with it (152 version strings, no dependency changes).CodeRabbit flagged both — inline on
Cargo.toml:295in #7196, and out-of-diff onCLAUDE.md:11in #7212. Neither was actioned.docs/src/internals/gc-rooting-invariant.md— the checker description was vacuousIt read "reports any root store that does not dominate a preceding collection point". A store can never dominate anything that precedes it, so that sentence is true of every store in the program and describes nothing.
What the checker reports is the collection point:
window_hits(origin, bind_ins)collects the calls that can run between the instruction producing a GC value and thejs_shadow_slot_bindthat publishes it. Reworded to name the collection point as the reported object, keep the true relation (the root store must dominate it), and tie back to the rule at the top of the same page. Also documented what the dominance computation is actually for here —dominates(idom, origin.block, bind_ins.block)is a value-identity check, not the invariant check.Separately noted that the gate command shown does not pass
--stale-registers, so cases 3 and 4 only surface when it is run by hand.CLAUDE.md— the shape summary said two things that are not truealloca_entryshape is not a slot at all. Only fix(codegen): make every GC root store dominate the collection points after it #7192 is literally a rooted slot. Split per shape.PERRY_GC_MOVING_LOOP_POLLS=1". The checker's ownPOLL_CAPABLE_RUNTIMEset classifiesjs_object_set_field_by_name,js_object_get_propertyandjs_call_functionas moving-capable with nojs_gc_loop_safepointinvolved, and codegen: ClassExprFresh leaves the class object unrooted across its own js_object_set_field_by_name stores #7211's allowlist entry is exactly that window. Polls widen in-loop coverage; they are not a precondition. This was the more dangerous of the two, since it invites reading a default-config green run as coverage.scripts/gc_root_dominance_allowlist.json— the documented fingerprint format was misleadingThe format line said
<first collector>. It issorted(set(callees))[0], the alphabetically first, not the first in program order. Because an entry matching nothing is a build failure, a maintainer deriving a fingerprint by reading the IR top-to-bottom writes an entry that turns a green run red for a reason the error message does not explain.scripts/gc_root_dominance_check.py— three surgical changes, no refactor_mutatesplices_SEED_CALLabove the store and moves nothing.--stale-registersreturned before the--unrooted-allocasblock, so passing both ran one pass and silently dropped the other. Now an argparse error, matching the--max-staleand--fatal-sinksguards directly above it.docs/src/internals/rfc-rooting-by-construction.mdPlain(String)cannot beCopy, and the migration section costed ~2500 call sites as if it were. Corrected toClone; the property the argument needs (borrows nothing, outlives every&mutemit) survives.Added the emitter/frame branding gap to "What it cannot catch", which undercuts the RFC's own "by construction" claim:
PhantomData<&'e Emitter>records a lifetime, not an instance, andRootedcarries a bareSlotIdxwithout naming itsShadowFrame. As written the design catches ordering mistakes but not provenance ones. Recorded rather than redesigned — closing it needs invariant branding and is a decision, not a quiet edit.docs/src/internals/memory-model.md"Two caveats" above a list of four bullets. Made count-free so a fifth cannot make it wrong again.
Verification
Real gate, on a corpus built from this branch (128
.llfiles, 2144 functions, 2744 root stores) — the exact CI command fromgc-root-dominance.yml:140:Byte-identical to the same corpus scanned with
origin/main's script and allowlist, so the reporting change is inert on the green path.--self-testOK. Both scan modes still run standalone; passing them together is now rejected.Full exit-code matrix, because a gate's exit codes are its contract:
On the parent, that bold row printed the stale entry only and discarded two real violations.
No Rust changed, so
cargo test -p perry-runtimeis untouched by this;cargo metadatais clean after the version restore.Deliberately not done
scripts/gc_root_dominance_check.pyis over the 2000-line cap andscripts/check_file_size.shfails on it. Worth flagging that this one is not pre-existing: the file went 1598 → 2149 lines in #7212, so that PR is what pushed it over. It needs its own split PR — folding a refactor in here would collide with the codegen work in flight on the same file, and it is not a change to make alongside correctness fixes.Two CodeRabbit nitpicks asking to trim
CLAUDE.mdand move detail intochangelog.d/are not taken. The material they point at is the per-shape breakdown a reader needs at the point of decision; corrected in place instead.Refs #7196, #7212, #7199, #7211.
Summary by CodeRabbit
Documentation
Bug Fixes