fix(gc): re-derive a string-literal operand below the collection point (#7114) - #7116
Conversation
#7114) lower_exprs_rooted suppressed Expr::String from temp rooting -- correctly, the handle global is a registered root -- but never applied the compensating re-load that RootedOperands::reread has always applied. The register loaded before the sibling operand's call was reused after it, so an evacuating minor left the concat reading the string's pre-move address. Route both helper families through one operand_protection() decision (Root / Reload / Reuse) so the pair cannot drift again. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
📝 WalkthroughWalkthroughString-literal operands are now reloaded after allocating sibling expressions when their registered storage can be rewritten by evacuating GC. Other operands use temporary roots or register reuse as appropriate. Regression tests cover concatenation, arrays, WTF-8 literals, and end-to-end GC stress. ChangesString operand GC handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ExpressionLowering
participant AllocatingSibling
participant EvacuatingGC
participant StringConcat
ExpressionLowering->>ExpressionLowering: classify string literal as Reload
ExpressionLowering->>AllocatingSibling: lower sibling expression
AllocatingSibling->>EvacuatingGC: allocate and trigger collection
EvacuatingGC-->>AllocatingSibling: relocate registered handles
ExpressionLowering->>ExpressionLowering: re-lower string literal handle
ExpressionLowering->>StringConcat: pass refreshed operands
Possibly related PRs
🚥 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 |
![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
🤖 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-codegen/src/expr/temp_root.rs`:
- Around line 636-698: Update operand_needs_root, operand_is_reloadable,
expr_may_trigger_gc, and expr_is_inert_primitive to treat Expr::WtfString the
same as Expr::String. Preserve the existing protection and reload behavior so
WtfString operands use their GC-registered handle correctly when an allocating
sibling may collect.
🪄 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: 61504e59-7d50-4918-b7e9-66af52b90610
📒 Files selected for processing (6)
changelog.d/7116-string-literal-operand-stale-after-evacuation.mdcrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/tests/temp_root_operand_temporaries.rstest-files/test_gap_gc_string_literal_operand_rooting.tstest-parity/gc_repsel_corpus.txt
…#7116 review) Comment + test only; no codegen behaviour change. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/tests/temp_root_operand_temporaries.rs (1)
596-610: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the IR assertions to the tested expression.
ir_forreturns the complete module IR. The three.find()calls select the first matching operation in that module. If another generated function emits one of these calls, the test can pass without proving that theExpr::WtfStringoperand is rooted and re-read after its allocating sibling.Extract the IR for the function containing this expression, or assert the operand-specific handle and expected call counts before comparing the order.
🤖 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-codegen/tests/temp_root_operand_temporaries.rs` around lines 596 - 610, Scope the IR checks in the WtfString temporary-rooting test to the generated function containing the tested expression instead of searching the complete module returned by ir_for. Update the push, alloc, and get lookups so they cannot match calls from unrelated functions, then preserve the required push → allocating sibling → re-read ordering assertion.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/tests/temp_root_operand_temporaries.rs`:
- Around line 596-610: Scope the IR checks in the WtfString temporary-rooting
test to the generated function containing the tested expression instead of
searching the complete module returned by ir_for. Update the push, alloc, and
get lookups so they cannot match calls from unrelated functions, then preserve
the required push → allocating sibling → re-read ordering assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16814ab6-cd7d-4b45-9d25-291de1f7302d
📒 Files selected for processing (2)
crates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/tests/temp_root_operand_temporaries.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen/src/expr/temp_root.rs
|
Thanks — this landed in the right place, but the conclusion doesn't hold for the current code, and I have IR evidence rather than an argument. I've taken the part that is real and gated it. Rejected:
if !collects { return Reuse }
if operand_needs_root(ctx, expr) { return Root }
if operand_is_reloadable(expr) { return Reload }
Reuse
Verified in the emitted IR, not by reading: the new test The same reasoning covers Rejected: Accepted: the drift hazard is real, so it is now a gate rather than a comment. You are right that the asymmetry is where a future edit gets this wrong — and the dangerous edit is half-closing it: adding a literal form to
Gates re-run after the change (
|
|
Post-merge note, for anyone grepping this PR for gate evidence.
Re-confirmed after the merge, on merged One CodeRabbit finding arrived at 05:03:58, a minute after the last commit, and |
…7120) * test(gc): scope the #7114 IR assertions to the function under test (#7116 review) ir_for returns the WHOLE module, so a bare .find() over it answers with whichever function comes first in the file. Every ordering assertion in the #7114 section is now sliced to @main via function_ir/init_ir and paired with an exact count of the operand-specific opcode inside that slice, so 'this is the operand I meant' is a property of the test rather than of module layout. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 * test(gc): make the IR-slice narrowing itself falsifiable Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Fixes #7114.
console.log("acc:" + run(10_000_000))printed an empty line and exited 0.No crash, no diagnostic — every gate that checks exit status passed.
What was wrong
The literal's
__perry_init_strings_*handle was loaded above the call andmasked to a pointer below it:
The handle global is a registered GC root, so the string was never swept and
the global was rewritten when the copying minor relocated it. The register
taken beforehand was not.
Root cause: one contract, two implementations, drifted
expr/temp_root.rssuppresses a string literal from temp rooting — correctly,it is already a root — and compensates by re-deriving it below the collection
point.
RootedOperands(new C(a, b), native collection methods) did bothhalves.
lower_exprs_rooted— behindlower_operand_pair_rooted, thearray-literal element list and the string-concat chain — did only the
suppression, and its own comment argued the residual staleness "is not the
hazard #6951 is about". It was #7114.
The invariant
A root buys three things and they are not the same: liveness, a rewritten
location, and the value the consuming call actually observes. Only the third
was missing here, which is why the object survived, the root was updated, and
the answer was still wrong.
The fix routes both helper families through one
operand_protection()decision(
Root/Reload/Reuse), so the pair cannot drift again.Which mechanism owns this case: the codegen shadow-stack / temp-root
mechanism, not
RuntimeHandleScope. The stale value lived in an LLVM SSAregister in generated code;
RuntimeHandleScopeis a Rust-side thread-localhandle stack for runtime helpers and cannot see that. The boundary between the
two is already sound —
js_array_push_f64_temp_rooteddocuments that itsvalueargument is rooted byjs_array_push_f64's ownRuntimeHandleScope—which is why the variadic-argument path was unaffected.
Cost: zero runtime calls.
Reloadre-emits theloadthat was going to beemitted anyway, and only when a later operand can collect, so
"user_" + iandevery other non-collecting concat keep their previous IR byte for byte.
Evidence
Host:
perry@perry-macos.local(Apple M1, 8 cores) — which is also the pinnedgc-ratchetbaseline host.--release, Node 26.5.1 (the.node-versionpin).Base =
ff85fd483. CI is not the gate right now (runner backlog); everythingbelow was gathered locally.
Reproduced and fixed, byte-diffed against the oracle
ff85fd483!119999700000acc:119999700000node --experimental-strip-typesacc:119999700000Genuinely different binaries:
perrysha2562df0831b…(base) vs86d9575a…(fix).
libperry_runtime.ais byte-identical (143b184c…) — correct, this is acodegen-only change, so a stale
.acannot make the A/B vacuous here.A copying collection actually ran
Same run, default GC settings, no env at all: 60 cycles, 2 255 476 objects
copied by the copying minor (
[gc-copy-minor] ran copied_objects=). Under theevacuating arm at
--pressure 8: 1 457 095 copied — base MISMATCH, fix MATCH.A non-moving collection cannot expose a stale pointer, so
minor_cycles > 0alone would not have been evidence.
Three independent shapes were broken, not one
Each promoted to first position in its own file, N = 400 000:
ff85fd483"acc:" + run(N)["arr", run(N)].join("|")`tpl:${run(N)}`run(N) + ":right""a" + "b" + run(N)The last two were always correct: in those orders the literal is loaded after
the call.
Sabotage, three directions
WtfStringfromoperand_needs_rootonly)Every assertion is anchored with
unwrap_or_else(panic!)on its fixture, so afixture that stopped producing the subject fails rather than passing on an
unreachable assertion.
Why the probe is the first statement
Every literal in a module is materialized together by
__perry_init_strings_*before user code runs, so the first evacuating cycle relocates all of them at
once; afterwards they are old-gen and a nursery scavenge cannot move them again.
A program gets exactly one chance to observe this. Measured at
--release,default settings: N=50 000 → 0 collections (vacuous pass); N=100 000 → 7 cycles,
590 472 scavenged, still passes; N=150 000 is the first failing size; the
file ships at N=400 000 for ~3x margin. A collector retune degrades it to
UNVERIFIED, the harness's honest state, not to a silent pass.GC x representation-selection matrix
scripts/gc_repsel_matrix.sh --arms all --pressure 8:Baseline on
mainwasPASS=359 UNVER=100 XFAIL=1 FAIL=0over 460 cells. Thedelta is exactly the 20 cells of the new corpus row, all PASS;
UNVER,XFAILand
FAILare unchanged. The singleXFAILis the pre-existing #6976 entry.The new row is live, not merely green — every
requires=movearm relocatedobjects on it (
evac_minor: 66 cycles / 1 506 487 scavenged;default(requires=scavenge): 50 cycles / 1 121 856 scavenged). Arm liveness across the
corpus: all seven
requires=movearms at 23/24 copy-minor,defaultandverify_evacat 14/24.gc-ratchet (run locally, pinned-host profile)
run_gc_ratchet_baseline.sh --checkon both arms — exit 0 for both.wall_msrss_bytesnode v26.5.1The ungated spread is the point: bit-identical gated counters are also what a
vacuous A/B looks like when a stale
.amakes both arms the same binary, so anonzero ungated spread on every row is the evidence the binaries really differed.
Review response
CodeRabbit raised one Major finding — that
Expr::WtfStringshould be added tothe same predicates. Rejected on IR evidence (
operand_needs_rootreturnstrueforWtfString, so it already takes the strictly strongerRootpath,and the
Reloadarm is unreachable behind it), but the drift it was worriedabout is now a gate rather than a comment — see the third sabotage row. Full
reasoning in the thread.