fix(gc): make argument temporaries precise roots (#6951) - #6972
Merged
Conversation
added 2 commits
July 29, 2026 07:18
With the conservative native-stack scan disabled -- precise/shadow-stack roots only -- a collection landing during argument evaluation dropped console.log's string-literal argument. No crash, no diagnostic. Harder shapes (string concat with an allocating right operand, `new C(str, f())`) segfaulted. Root cause: the shadow stack roots named locals. It has no slot for the values that exist only between two instructions, and an LLVM SSA register is not a GC root. `console.log(a, b)` lowers to `js_array_alloc(2)` plus one `js_array_push_f64` per argument, with the accumulator threaded through an SSA register. That register held the ONLY reference to everything pushed so far -- argument 0 included -- across argument 1's evaluation. A collection there swept the half-built array and the following push landed in recycled memory whose length had been reset to 0, so the label disappeared and only the number printed. Conservative stack scanning hid this: gc_check_trigger forces a full conservative scan on both automatic arms, while gc/roots.rs's nominal production default is Auto -> SkipDisabled. The scan was doing load-bearing correctness work, not acting as a safety net. New mechanism: a per-thread temp-root stack callable from generated code (gc/roots/temp_roots.rs), registered as a budgeted mutable root scanner, so slots are both MARKED and REWRITTEN. Slots decode through visit_heap_word_u64_slot, accepting both word forms the gc::root_words contract admits (NaN-boxed values and bare heap addresses -- generated code pushes both). Generated code pushes before the collection point, re-reads after (mandatory: an evacuating cycle rewrites the slot), and truncates after the consuming call. ShadowSavepoint carries the depth, so the longjmp unwind that already restores the shadow stack restores this stack with it. Rooted sites: the variadic argument accumulator (console log/info/warn/ error/debug/trace/assert/timeLog), the string-concat operand pair and the n-way concat chain (template literals, log lines), the object-literal handle across its initializers, and array-literal element values. Emission is gated on "does anything after this reach a collection point" -- `"a" + i`, `[1, 2, 3]` and all-local argument lists emit byte-identical IR to before. test-parity/gc_repsel_triage.txt: both triaged cells clear. The matrix's cons_scan_off arm (in the PR arm set) is now a hard gate on this shape.
Two gates on top of "does anything after this collect": - A value `expr_is_known_non_pointer_shadow_value` proves is not a heap reference roots nothing, so a slot for it is pure TLS traffic. This is what keeps `total + s.length` and every other numeric operand pair on byte-identical IR. - A string literal loads from a module global that `__perry_init_strings_*` already registered with `js_gc_register_global_root`, so the sweep can never take it. (The loaded register is still stale after an *evacuating* cycle, but that is true of every `Expr::String` use in the compiler and is not the hazard #6951 is about.) Template literals are mostly literal parts, so this is the difference between 3 and 7 rooting calls per interpolation. Measured on a hot loop doing `"user_" + i`, an array literal, an object literal and a template literal per iteration: 32 -> 12 emitted rooting calls, and the remaining 12 are the template literal's one coerced heap operand plus the once-per-program console.log.
📝 WalkthroughWalkthroughAdds a per-thread precise temporary-root stack, registers it with the GC, and applies it to generated argument, literal, concatenation, binary, and console lowering. Runtime, codegen, regression tests, and GC parity triage are updated for precise collection without conservative stack scanning. ChangesPrecise GC root handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExpressionLowering
participant TempRootAPI
participant GC
participant RuntimeCall
ExpressionLowering->>TempRootAPI: push evaluated temporary
ExpressionLowering->>ExpressionLowering: lower later allocating expression
GC->>TempRootAPI: scan and rewrite root slot
ExpressionLowering->>TempRootAPI: re-read updated value
ExpressionLowering->>RuntimeCall: consume rooted values
ExpressionLowering->>TempRootAPI: truncate temporary roots
Possibly related issues
Suggested reviewers: 🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/perry-codegen/src/lower_string_method.rs (1)
1408-1463: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot
lhs_boxbefore lowering the non-string rhs.In the non-string rhs path,
rhs_val/js_jsvalue_to_stringcan run whilelhs_boxis live in an SSA register, reproducing the same#6951use-after-free pattern fixed bylower_operand_pair_rooted/temp_root_*elsewhere. Uselower_operand_pair_rooted(ctx, &slot, rhs)/temp_root_release, or root the slot result before evaluatingrhsand re-read it before the dispatch/unbox.🤖 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/src/lower_string_method.rs` around lines 1408 - 1463, Root the destination slot value before lowering the non-string rhs in lower_canonical_str_self_append, preventing rhs evaluation and js_jsvalue_to_string from invalidating the live lhs_box SSA value. Use the existing lower_operand_pair_rooted and temp_root_release flow, or root the slot result and reload it before tag dispatch and unboxing; preserve the current append branching behavior.crates/perry-codegen/src/expr/binary.rs (1)
543-550: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot the left operand across the BigInt dynamic-helper call.
Use
lower_operand_pair_rooted(ctx, left, right)here and calltemp_root_release(ctx, guard)afterctx.block().call(DOUBLE, fname, ...)so a GC duringright’s evaluation cannot collect/move a pointer-bearing left operand thatfnamestill needs.🤖 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/src/expr/binary.rs` around lines 543 - 550, Update the !inline_bitwise branch in the BigInt dynamic-helper path to use lower_operand_pair_rooted(ctx, left, right), retaining the returned guard and rooted operands. Call ctx.block().call(DOUBLE, fname, ...) with those operands, then invoke temp_root_release(ctx, guard) after the call before returning, ensuring the left operand remains rooted through right evaluation and helper execution.crates/perry-codegen/src/expr/object_literal.rs (1)
472-539: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot postponed closure values before reusing them.
this_patcheskeeps method-closure handles across laterlower_exprcalls, and after the loop each value is used as a pointer argument tojs_closure_set_capture_bits. Root every pushed closure value or all deferred slots together, then refresh eachclosure_valbefore the patch loop so a later allocation cannot move an earlier queue entry.🤖 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/src/expr/object_literal.rs` around lines 472 - 539, The deferred values stored in this_patches can become stale after later lower_expr allocations. Root each pushed closure value (or the complete deferred collection) while building the object, then refresh the rooted closure value before using it in the js_closure_set_capture_bits loop; update the this_patches handling without changing the existing capture-index or patching order.
🧹 Nitpick comments (1)
crates/perry-codegen/tests/temp_root_argument_temporaries.rs (1)
139-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove this
#6951coverage intocargo-test.
crates/perry-codegen/tests/temp_root_argument_temporaries.rsis integration coverage behindci_e2e_scope.py; the scoping rules select direct suite/module/fixture changes but otherwise don’t mapperry-codegensrc/changes to suites. A regression inexpr/temp_root.rs,expr/array_literal.rs, or related codegen could leave these exact temp-root checks unrun.🤖 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_argument_temporaries.rs` around lines 139 - 227, Move the `#6951` temp-root coverage from the integration test module into the cargo-test suite so it runs under the applicable CI scope. Preserve the assertions and ordering checks from console_argument_accumulator_is_temp_rooted, non_allocating_element_list_emits_no_rooting_calls, and array_literal_roots_elements_before_an_allocating_element, adapting only the test harness needed for cargo-test.Sources: Coding guidelines, Learnings
🤖 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 99-139: Update expr_may_trigger_gc to conservatively treat
coercion-capable expressions as GC-triggering: Compare, Unary, and non-Add
Binary operations must return true unless their operands are provably inert
primitives, while still recursing through safe operand evaluations as
appropriate. Ensure loose/relational comparisons and numeric coercions cannot
incorrectly return false when runtime conversion may invoke user-defined methods
or allocate.
---
Outside diff comments:
In `@crates/perry-codegen/src/expr/binary.rs`:
- Around line 543-550: Update the !inline_bitwise branch in the BigInt
dynamic-helper path to use lower_operand_pair_rooted(ctx, left, right),
retaining the returned guard and rooted operands. Call ctx.block().call(DOUBLE,
fname, ...) with those operands, then invoke temp_root_release(ctx, guard) after
the call before returning, ensuring the left operand remains rooted through
right evaluation and helper execution.
In `@crates/perry-codegen/src/expr/object_literal.rs`:
- Around line 472-539: The deferred values stored in this_patches can become
stale after later lower_expr allocations. Root each pushed closure value (or the
complete deferred collection) while building the object, then refresh the rooted
closure value before using it in the js_closure_set_capture_bits loop; update
the this_patches handling without changing the existing capture-index or
patching order.
In `@crates/perry-codegen/src/lower_string_method.rs`:
- Around line 1408-1463: Root the destination slot value before lowering the
non-string rhs in lower_canonical_str_self_append, preventing rhs evaluation and
js_jsvalue_to_string from invalidating the live lhs_box SSA value. Use the
existing lower_operand_pair_rooted and temp_root_release flow, or root the slot
result and reload it before tag dispatch and unboxing; preserve the current
append branching behavior.
---
Nitpick comments:
In `@crates/perry-codegen/tests/temp_root_argument_temporaries.rs`:
- Around line 139-227: Move the `#6951` temp-root coverage from the integration
test module into the cargo-test suite so it runs under the applicable CI scope.
Preserve the assertions and ordering checks from
console_argument_accumulator_is_temp_rooted,
non_allocating_element_list_emits_no_rooting_calls, and
array_literal_roots_elements_before_an_allocating_element, adapting only the
test harness needed for cargo-test.
🪄 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: e3832b0e-7978-4fc3-b247-0054ee4c5a00
📒 Files selected for processing (17)
changelog.d/6972-precise-root-argument-temporaries.mdcrates/perry-codegen/src/expr/array_literal.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/object_literal.rscrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/runtime_decls/arrays.rscrates/perry-codegen/tests/temp_root_argument_temporaries.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/roots/shadow_stack.rscrates/perry-runtime/src/gc/roots/temp_roots.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/temp_roots.rstest-parity/gc_repsel_triage.txt
Comment on lines
+99
to
+139
| pub(crate) fn expr_may_trigger_gc(expr: &Expr) -> bool { | ||
| match expr { | ||
| // Immediates and plain slot reads. `LocalGet` reads an alloca, | ||
| // `GlobalGet` a module global — neither allocates. | ||
| Expr::Undefined | ||
| | Expr::Null | ||
| | Expr::Bool(_) | ||
| | Expr::Number(_) | ||
| | Expr::Integer(_) | ||
| | Expr::LocalGet(_) | ||
| | Expr::GlobalGet(_) => false, | ||
| // A string literal is materialized once into a module-global handle by | ||
| // `__perry_init_strings_*` and registered as a GC root there; the use | ||
| // site is a load. | ||
| Expr::String(_) => false, | ||
| Expr::Unary { operand, .. } => expr_may_trigger_gc(operand), | ||
| Expr::Compare { left, right, .. } => { | ||
| expr_may_trigger_gc(left) || expr_may_trigger_gc(right) | ||
| } | ||
| // `+` on unknown operands can be string concatenation, which allocates; | ||
| // every other binary operator is numeric or bitwise. | ||
| Expr::Binary { | ||
| op, left, right, .. | ||
| } => { | ||
| matches!(op, perry_hir::BinaryOp::Add) | ||
| || expr_may_trigger_gc(left) | ||
| || expr_may_trigger_gc(right) | ||
| } | ||
| Expr::Conditional { | ||
| condition, | ||
| then_expr, | ||
| else_expr, | ||
| } => { | ||
| expr_may_trigger_gc(condition) | ||
| || expr_may_trigger_gc(then_expr) | ||
| || expr_may_trigger_gc(else_expr) | ||
| } | ||
| Expr::Sequence(exprs) => exprs.iter().any(expr_may_trigger_gc), | ||
| _ => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect CompareOp variants and how loose/relational comparisons + non-Add
# binary/unary ops are lowered, to see if they can invoke user-defined
# valueOf/toPrimitive (allocating) code without being flagged by
# expr_may_trigger_gc.
rg -n 'enum CompareOp' -A 15
rg -n 'enum BinaryOp' -A 20
rg -nP '\bfn lower\b' -A 60 crates/perry-codegen/src/expr/compare.rs
rg -nP '\bfn lower\b' -A 60 crates/perry-codegen/src/expr/binary.rs
rg -nP '\bfn lower\b' -A 60 crates/perry-codegen/src/expr/unary.rs
rg -n 'js_loose_eq|js_to_primitive|js_relational_compare|js_to_number|js_abstract' crates/perry-runtime/src -g '*.rs'Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "repo files match expr/temp_root.rs:"
git ls-files | grep -F 'crates/perry-codegen/src/expr/temp_root.rs' || true
git ls-files | grep -F 'crates/perry-codegen/src/expr' | head -80 || true
echo
echo "temp_root.rs relevant section:"
if [ -f crates/perry-codegen/src/expr/temp_root.rs ]; then
nl -ba crates/perry-codegen/src/expr/temp_root.rs | sed -n '1,180p'
fi
echo
echo "hir comparison/op enums by filenames:"
git ls-files | rg 'src/.*(compare|binary|unary).*\.rs$|src/.*(hir|temporal).*\.rs$' | head -200
rg -n 'enum CompareOp|enum BinaryOp|enum UnaryOp|LooseEq|LooseNe|Strict|Loose|Relational|NumberCheck|BigInt|Integer|String|Boolean|Null|Undefined' -g '*.rs' crates | head -250Repository: PerryTS/perry
Length of output: 31185
🏁 Script executed:
#!/bin/bash
set -u
echo "temp_root.rs first 180 lines:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/temp_root.rs | sed -n '1,180p'
echo
echo "compare/binary/unary outlines:"
for f in crates/perry-codegen/src/expr/compare.rs crates/perry-codegen/src/expr/binary.rs crates/perry-codegen/src/expr/unary.rs; do
echo "--- $f"
ast-grep outline "$f" || true
done
echo
echo "compare.rs lower body:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/compare.rs | sed -n '1,240p'
echo
echo "binary.rs relevant contents:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/binary.rs | sed -n '1,260p'
echo
echo "unary.rs relevant contents:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/unary.rs | sed -n '1,260p'
echo
echo "CompareOp/BinaryOp/UnaryOp definitions in perry-hir:"
rg -n 'pub enum ComparisonOp|pub enum BinaryOp|pub enum UnaryOp|LooseEq|LooseNe|Lt|Gt|Le|Ge|NumberCheck|BigInt' crates/perry-hir crates/perry-codegen/src -g '*.rs' | head -240Repository: PerryTS/perry
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "temp_root.rs compare/binary/unary callsites and guard logic:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/temp_root.rs | sed -n '180,380p'
echo
echo "perry-hir ComparisonOp/BinaryOp/UnaryOp definitions:"
rg -n 'ComparisonOp|Bin(Op|op)|UnaryOp|LooseEq|LooseNe|Lt|Gt|Le|Ge|Pos|Neg|BitNot|Add|Eq|Ne' crates/perry-hir/src -g '*.rs' | head -220
echo
echo "binary.rs public lower body:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/binary.rs | sed -n '260,560p'
echo
echo "runtime compare coercion calls:"
rg -n 'pub unsafe extern "C" fn js_rel|pub unsafe extern "C" fn js_loose_eq|pub unsafe extern "C" fn js_dynamic|js_to_primitive|js_to_number|js_abstract' crates/perry-runtime/src -g '*.rs' | head -220
echo
echo "runtime js_rel implementation snippets:"
for f in crates/perry-runtime/src/object/compute.rs crates/perry-runtime/src/object/mod.rs crates/perry-runtime/src/*.rs; do
[ -f "$f" ] || continue
if rg -q 'js_rel|js_loose_eq|js_dynamic' "$f"; then
echo "--- $f"
rg -n 'js_rel|js_loose_eq|js_dynamic_to_primitive|js_to_number_to_js_value|js_to_primitive|js_to_number|js_value_to_string|js_value_to_numeric|js_value_to_big_int|js_value_to_number' "$f" -A 80 -B 10 | head -260
fi
doneRepository: PerryTS/perry
Length of output: 48335
🏁 Script executed:
#!/bin/bash
set -u
echo "compare.rs remaining lower body:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/compare.rs | sed -n '240,520p'
echo
echo "HIR enum definition exact snippets:"
rg -n '^pub (enum|struct) ComparisonOp|^pub (enum|struct) BinaryOp|^pub (enum|struct) UnaryOp|LooseEq|LooseNe|Lt|Gt|Le|Ge|Add|BitAnd|BitOr|BitXor|Shl|Shr|UShr|Pow' crates/perry-hir/src/codes.rs crates/perry-hir/src/ir.rs crates/perry-hir/src/ast.rs crates/perry-hir/src/expr -g '*.rs' -A 3 -B 3 | head -280 || true
echo
echo "js_rel and loose helper declarations / definitions:"
rg -n 'pub unsafe extern "C" fn js_rel|pub unsafe extern "C" fn js_loose_eq|pub unsafe extern "C" fn js_value_to|fn js_rel|fn js_loose|js_rel\(|js_loose_eq\(|Symbol::toPrimitive|toPrimitive|valueOf|toString' crates/perry-runtime/src -g '*.rs' -A 8 -B 3 | head -320
echo
echo "runtime symbol js_to_primitive implementation:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-runtime/src/symbol/iterator.rs | sed -n '440,530p'
echo
echo "runtime dynamic arithmetic coercion bodies:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-runtime/src/value/dynamic_arith.rs | sed -n '180,260p'
echo
echo "low-level coercion implementations:"
rg -n 'pub\s+unsafe\s+extern\s+"C"\s+fn\s+js_value_to_numeric|pub\s+unsafe\s+extern\s+"C" f' crates/perry-runtime/src -g '*.rs' -A 80 -B 10 | head -360Repository: PerryTS/perry
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "compare.rs final fallthrough:"
awk '{printf "%5d %s\n", NR, $0}' crates/perry-codegen/src/expr/compare.rs | sed -n '508,620p'
echo
echo "js_rel loose helper direct matches:"
rg -n 'js_rel|js_loose_eq|js_to_primitive|js_value_to_string|js_value_to_number|js_to_number' crates/perry-runtime/src/value crates/perry-runtime/src/object -g '*.rs' -A 80 -B 6 | head -420Repository: PerryTS/perry
Length of output: 49674
Root across coercion-capable operands in expr_may_trigger_gc.
Compare, unary numeric coercions, and non-add binary coercion paths can dispatch to runtime helpers that invoke user-defined Symbol.toPrimitive/valueOf/toString and allocation can occur on those operand evaluations. The current false case for Compare ignores the loose/relational ops, and the current Unary/non-Add Binary cases only recurse into operands; make these cases return true unless the operands are provably inert primitives, or handle the coercion points explicitly when computing GC roots.
🤖 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/src/expr/temp_root.rs` around lines 99 - 139, Update
expr_may_trigger_gc to conservatively treat coercion-capable expressions as
GC-triggering: Compare, Unary, and non-Add Binary operations must return true
unless their operands are provably inert primitives, while still recursing
through safe operand evaluations as appropriate. Ensure loose/relational
comparisons and numeric coercions cannot incorrectly return false when runtime
conversion may invoke user-defined methods or allocate.
This was referenced Jul 29, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Jul 29, 2026
The GC x representation matrix has not run on main since #6925. That PR added test-files/test_gap_repsel_proven_this_frozen.ts without registering it in test-parity/gc_repsel_corpus.txt, and scripts/gc_repsel_matrix.sh enforces registration by exiting 3 BEFORE running anything. So `gc-stress` was failing on every PR with an UNREGISTERED message and the corpus was never exercised -- the gate was dark, not green. That is exactly the omission the manifest exists to catch (#6954, RFC 5.6); registering the file is one line. With the gate running again a genuine red cell appears, and it is Phase 5a's own escape hatch: repsel_ptr_shape_locals x rep_ptr_shape_off (PERRY_PTR_SHAPE_LOCALS=0 + PERRY_GC_FORCE_EVACUATE=1) dies partway with `TypeError: Cannot read properties of undefined (reading 'area')`, losing its last five output lines. Bisected: rc=0 at 8327ced (parent of #6925), rc=1 at 1a533a3 (#6925), rc=1 on current main. NOT #6972 -- a full --arms all run on that branch (based on 8327ced) was 361/361 byte-exact with FAIL=0, which included this cell. Filed as #6976 and triaged against it, with an explicit instruction to remove the entry when it is fixed: an arm that turns a representation OFF is supposed to be the safest cell in the matrix. Result: 380 cells, 379 byte-exact, PASS=29 UNVER=350 XFAIL=1 FAIL=0, and repsel_gc_stress x cons_scan_off / cons_scan_off_force stay PASS with the arm live.
proggeramlug
added a commit
that referenced
this pull request
Jul 29, 2026
…rom the #6972 review, plus restore the dark gc-stress gate (#6975) * fix(gc): close four rooting gaps CodeRabbit found in the #6951 review All four are real; three are sites, one is a soundness hole in the gate. 1. `expr_may_trigger_gc` was unsound for coercing operators. It answered `false` for `Compare` / `Unary` / non-`Add` `Binary` whenever it could recurse into the operands without finding an allocation — but `o < x`, `-o` and `o * 2` run ToPrimitive / ToNumber on their operands, and a user-defined `Symbol.toPrimitive` / `valueOf` / `toString` is arbitrary JS: it allocates and it collects. `a < b` over two plain `LocalGet`s recursed straight to `false`. Since the predicate's whole contract is "false must mean provably no collection", that re-introduced the very bug in a narrower case. These operators are now GC-capable unless every operand is a proven inert primitive (`expr_is_inert_primitive`: literals, and locals the type analysis proved Number / Int32 / Boolean / Null / Void / Never with no reserved shadow slot). `i < n` and `x * 2` on numeric locals stay free — hot-loop rooting-call count is unchanged at 12. 2. `expr/binary.rs` BigInt dynamic helper, `!inline_bitwise` branch: a second copy of the two-`lower_expr` shape that the first pass missed (different indentation). Now uses `lower_operand_pair_rooted`. 3. `lower_canonical_str_self_append`: `s += rhs` must load `s` BEFORE evaluating `rhs` (a `rhs` that reassigns `s` must not be observed), so the pre-rhs value is carried across `rhs` and across `js_jsvalue_to_string` — both allocate. Re-reading the slot would take the wrong value, so the loaded box goes into a temp root. The coerced rhs handle is rooted too: the cold arm's `unbox_str_handle` materializes an SSO destination onto the heap, which is another allocation with the rhs handle live. 4. `lower_object_literal`'s `this_patches`: method-closure values are deferred and reused after every remaining property is lowered, so they sit in SSA registers across all of those allocations. Rooted when any initializer can collect, and refreshed before the patch loop. Re-verified: repro fixed; probe suite unchanged; 431-file gap sweep identical to the origin/main baseline; gc_repsel_matrix --arms all 361/361 byte-exact, FAIL=0, XFAIL=0; throw-through-argument-list byte-exact under both arms. * docs: changelog fragment for #6975 * test(gc): restore the gc-stress gate that #6925 left dark The GC x representation matrix has not run on main since #6925. That PR added test-files/test_gap_repsel_proven_this_frozen.ts without registering it in test-parity/gc_repsel_corpus.txt, and scripts/gc_repsel_matrix.sh enforces registration by exiting 3 BEFORE running anything. So `gc-stress` was failing on every PR with an UNREGISTERED message and the corpus was never exercised -- the gate was dark, not green. That is exactly the omission the manifest exists to catch (#6954, RFC 5.6); registering the file is one line. With the gate running again a genuine red cell appears, and it is Phase 5a's own escape hatch: repsel_ptr_shape_locals x rep_ptr_shape_off (PERRY_PTR_SHAPE_LOCALS=0 + PERRY_GC_FORCE_EVACUATE=1) dies partway with `TypeError: Cannot read properties of undefined (reading 'area')`, losing its last five output lines. Bisected: rc=0 at 8327ced (parent of #6925), rc=1 at 1a533a3 (#6925), rc=1 on current main. NOT #6972 -- a full --arms all run on that branch (based on 8327ced) was 361/361 byte-exact with FAIL=0, which included this cell. Filed as #6976 and triaged against it, with an explicit instruction to remove the entry when it is fixed: an arm that turns a representation OFF is supposed to be the safest cell in the matrix. Result: 380 cells, 379 byte-exact, PASS=29 UNVER=350 XFAIL=1 FAIL=0, and repsel_gc_stress x cons_scan_off / cons_scan_off_force stay PASS with the arm live. --------- Co-authored-by: Ralph Küpper <[email protected]>
This was referenced Jul 29, 2026
proggeramlug
added a commit
that referenced
this pull request
Jul 29, 2026
…6987) (#6989) * docs(gc): policy gate defaults said OFF while the code defaults ON gc_incremental_enabled's doc header claimed 'EXPERIMENTAL — default OFF' eight lines above a body comment reading 'DEFAULT ON (#6180 flip)'. gc_safepoint_moving_minor described its gate gc_moving_safepoint_enabled as '(default off)' when that accessor is !matches!(env, 0|off|false), i.e. default ON. Both now state the real default and name the kill switch. Also records why the incremental default is load-bearing beyond pause times: with the stepper on, budgeted cycles skip the conservative stack-scan subphase structurally, so a compiled program completes precise-roots-only cycles in its shipped config. Reasoning that assumes the automatic arms force a conservative scan is wrong by default — the mistake #6972 made while this comment still said the gate was off. gc_moving_loop_polls_enabled's 'opt-in, default OFF' is accurate and unchanged. Refs #6987 * changelog: fragment for #6989 --------- Co-authored-by: Ralph Küpper <[email protected]>
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.
Fixes #6951.
The bug
With the conservative native-stack scan disabled — precise/shadow-stack roots only — a collection landing during argument evaluation dropped
console.log's string-literal argument. No crash, no diagnostic.Reproduced on the issue's 15-line repro (
--profile perry-dev, macOS arm64, node 26.5.0), byte-identically with all six representation gates off (PERRY_CANONICAL_I32_LOCALS=0 PERRY_CANONICAL_STR_LOCALS=0 PERRY_PTR_SHAPE_LOCALS=0 PERRY_PTR_NUMARRAY_LOCALS=0 PERRY_SPECIALIZED_ABI=0 PERRY_INT_VALUED_LOCALS=0) — a runtime rooting gap, not a representation defect.PERRY_GC_TRACE=1confirms 16 completed cycles, all non-moving.Root cause
The shadow stack roots named locals: one slot per pointer-typed local, bound to that local's alloca. It has no slot for the values that exist only between two instructions, and an LLVM SSA register is not a GC root.
console.log("alpha", churn(420000))lowers to:%r9— the argument accumulator, holding the only reference to"alpha"— is live acrosschurnin nothing but an SSA register. The sweep freed it,churnreused the block, and the nextjs_array_push_f64wrote the number into a header whoselengthhad been reset to 0. Hence a one-element array and a vanished label with no leading space.Conservative stack scanning hid this:
gc_check_triggertakesManualGcScanGuard::force_full_scan()on both automatic arms, whilegc/roots.rs's nominal production default isAuto -> SkipDisabled. The scan was doing load-bearing correctness work, not acting as a safety net.Scope found (measured, not asserted)
A 20-shape probe suite, each shape repeated 6× at top level (the failure is a use-after-free — one collection is not enough, the block has to be recycled), run under
PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8against node 26.5.0. 13 of 20 shapes were broken before this PR; 4 of them SIGSEGV'd or aborted.console.log("lit", f())console.log("a", "b", f())console.log(fresh(), f())f()(control)fresh() + "/" + f()`${fresh()}#${f()}`console.log([fresh()], f())console.log(JSON.stringify(o), f())console.log(fresh(), fresh2(), f())[fresh(), f()](scalar-replaced){ a: fresh(), b: f() }(scalar-replaced)new C(fresh(), f())map.set(fresh(), f())join2(fresh(), f()),obj.m(…),arr.push(…),[...a, f()], nested calls, single-argFixed here:
console.log/info/warn/error/debug/trace/assert/timeLogand their*_spreadconsumers;lower_string_concat,lower_string_coerce_concat) and the n-way concat chain (lower_string_concat_chain— template literals, log lines), plus the intermediatejs_jsvalue_to_stringhandle in the both-non-string fallback;Not fixed, filed granularly with the probe that reproduces each:
[fresh(), f()]and{a: fresh(), b: f()}never allocate a container at all; the element slots are%rN = alloca doubleand are invisible to the collector. Different defect (escape analysis / representation), same symptom.new C(a, b)constructor arguments (lower_call/new.rs::lowered_args) are held acrossjs_object_alloc_class_inline_keysand the constructor call.map.set(a, b)) — aborts.s.concat(x)).The honest general statement: every pointer-valued SSA temporary live across a GC-capable call is unrooted.
expr/array_literal.rssaid so out loud before this PR ("Element expressions with their own allocations lower to SSA values pinned by conservative stack scanning"). This PR fixes the argument-temporary class the issue names plus the concat/literal families; a codegen-wide safepoint-liveness pass is what would close the rest, and that is a separate epic — the four issues above are its concrete, reproducible entry points.Mechanism
crates/perry-runtime/src/gc/roots/temp_roots.rs: a per-thread temp-root stack callable from generated code, registered ingc_initas a budgeted mutable root scanner — so slots are marked and rewritten, not pinned. Slots are visited withvisit_heap_word_u64_slot, the same decoder the shadow stack uses, so a slot may hold either word form thegc::root_wordscontract admits: a NaN-boxed value (whatlower_exprproduces) or a bare heap address (the rawi64array pointers threaded throughjs_array_alloc/js_array_push_f64). Immediates decode to nothing, so callers need not prove pointer-ness.Emission contract, in order:
Re-reading is mandatory, not defensive: the slot is a mutable root, so an evacuating cycle rewrites it and the register pushed beforehand is stale. That is exactly why this is preferable to widening conservative scanning — conservative roots have to pin, precise ones can move, which is the property #6950 needs.
js_array_push_f64_temp_rooted(idx, value)is the fused accumulator push (get + push + write-back), so an argument list pays the same number of calls per argument as before, plus three per list.Truncate is a stack cut, not a pop, so a missed release is bounded by the next one rather than leaking forever. Non-local exits are covered without touching
crate::exception:ShadowSavepointnow carries the temp-root depth, so thelongjmpunwind that already restores the shadow stack restores this stack with it.Cost
Emission is gated three ways, and any one of them suppresses it entirely:
expr_may_trigger_gc, one-sided — unrecognized means "assume it collects");expr_is_known_non_pointer_shadow_value);js_gc_register_global_root."user_" + i,[1, 2, 3],{a: i, b: total}and all-local argument lists emit byte-identical IR to before — asserted bynon_allocating_element_list_emits_no_rooting_calls.Measured on a hot loop doing a concat, an array literal, an object literal and a template literal per iteration: the three gates took emitted rooting calls from 32 → 12, and the remaining 12 are the template literal's one coerced heap operand (3 calls) plus the once-per-program
console.log. A protected site costs onejs_gc_temp_root_push+ onejs_gc_temp_root_get, each a TLS access and aVecop, plus onetruncateper group.The one place rooting is not gated is the console argument accumulator, which is always rooted:
console.logis not a hot path and the arguments are about to beutil.inspect-formatted, so a conditional there would buy nothing and could miss a case.Verification
PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8, withPERRY_GC_TRACE=1confirming real collections.test_gap_repsel_gc_stress.tsunderPERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8, 17 completed cycles in both arms:scripts/gc_repsel_matrix.sh --arms all, pinned node 26.5.0, pressure 8 MB: 361/361 cells byte-exact, FAIL=0, XFAIL=0.test_gap_repsel_gc_stress × cons_scan_offand× cons_scan_off_forcemoved from XFAIL to PASS with the arm measurably live (17 completed cycles) — the two entries intest-parity/gc_repsel_triage.txtare removed, socons_scan_off(a PR arm) is now a hard gate on this shape.test_gap_*.tsfiles, byte-exact vs node 26.5.0,origin/mainvs this branch — identical result sets (402 OK; the 7 DIFF / 9 RUNFAIL / 13 NODEFAIL are pre-existing and unchanged; the last two categories are this ad-hoc harness lacking the real runner's normalization and skip list).gc::tests::temp_roots, every one pinningConservativeStackScanMode::Disabled— with the scan on the bug is invisible, so an unpinned test proves nothing. The load-bearing one (temp_rooted_value_survives_a_real_collection) was verified to have teeth: dropping the scanner registration makes it fail withan object reachable only through a temp root must be marked, not swept (#6951).crates/perry-codegen/tests/temp_root_argument_temporaries.rspinning the emission contract (push / fused push / re-read / truncate, in that order) and the no-cost gate.origin/main, both unrelated to this change: a workspacecargo testbuild cannot link theperry-ext-http/perry-ext-http-serverlib-test binaries (undefinedjs_request_new/js_response_new— a feature-unification artifact of the workspace-wide selection; those packages build fine standalone), andcrates/perry-codegen/tests/native_proof_regressions.rshas 3 failures (mutable_string_key_rejects_static_write_pic,static_put_value_rejects_write_pic_when_rhs_can_allocate,nested_same_shape_object_writes_version_one_through_four_fields) — verified identical onorigin/main, unrelated to this PR.What this does not fix
moved_objectsis still 0 in every arm, and 333 of the 361 matrix cells are still UNVERIFIED. That is #6950 (gc_check_triggerforces a full conservative scan on both automatic arms, making copied-minor ineligible) plus the corpus members that perform zero collections. This PR removes the reason the precise-roots-only arm could not discriminate; making an evacuating precise-rooted minor reachable is the next step, and it is now unblocked.