fix(path): #7621 — path.* arms read an SSO string's inline bytes as a StringHeader pointer - #7626
Conversation
📝 WalkthroughWalkthroughPath codegen now passes boxed operands to SSO-safe path helpers. The runtime materializes short strings, roots operands across allocation, and preserves heap-string and non-string behavior. Tests cover POSIX and Windows APIs, SSO boundaries, errors, and allocation churn. ChangesPath operand materialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PathCodegen
participant ValueArgs as value_args
participant PathRuntime
PathCodegen->>ValueArgs: Pass boxed path operands
ValueArgs->>ValueArgs: Materialize SSO strings
ValueArgs->>ValueArgs: Root and reread operands across allocation
ValueArgs->>PathRuntime: Invoke pointer-based path operation
PathRuntime-->>PathCodegen: Return path result
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: 2
🤖 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 `@changelog.d/7626-path-sso-operands.md`:
- Around line 50-53: Correct the test coverage statement in the changelog entry
to report the four unit tests defined in value_args.rs, unless a fifth runtime
test is actually added.
In `@crates/perry-runtime/src/path/value_args.rs`:
- Around line 101-110: Root both path operands before any materialization and
reload them after allocating operations in the runtime path flow around
value_args.rs:101-110. In instance_misc1.rs, wrap PathJoin (882-890),
PathWin32Join (902-905), Win32 BasenameExt/ResolveJoin/MatchesGlob (964-992),
and PathBasenameExt (1634-1645) lowering with rooting::with_operands_rooted so
raw operands remain valid across sequential lowering. Add the requested
GC-stress coverage for a heap first operand and allocating second operand in
test_gap_7621_path_sso_operands.ts:88-106.
🪄 Autofix
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: 8a662af9-4584-4db2-9030-812d8245abff
📒 Files selected for processing (9)
changelog.d/7626-path-sso-operands.mdcrates/perry-codegen/src/expr/array_methods.rscrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/misc_methods.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-runtime/src/path.rscrates/perry-runtime/src/path/value_args.rstest-files/test_gap_7621_path_sso_operands.ts
| Covered by `test-files/test_gap_7621_path_sso_operands.ts` (both sides of the | ||
| SSO boundary, computed and literal operands, absolute and relative bases, | ||
| multi-segment resolves, all twelve arms, the non-string throw) plus five | ||
| `perry-runtime` unit tests. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the runtime-test count.
crates/perry-runtime/src/path/value_args.rs defines four unit tests, not five. Change this count or add the missing test.
🤖 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 `@changelog.d/7626-path-sso-operands.md` around lines 50 - 53, Correct the test
coverage statement in the changelog entry to report the four unit tests defined
in value_args.rs, unless a fifth runtime test is actually added.
| let scope = RuntimeHandleScope::new(); | ||
| let a_ptr0 = path_arg_header(a); | ||
| let a_handle = JSValue::from_bits(a.to_bits()) | ||
| .is_any_string() | ||
| .then(|| scope.root_string_ptr(a_ptr0)); | ||
| let (b_ptr, a_ptr) = match &a_handle { | ||
| Some(h) => h.across_const::<StringHeader, _>(|| path_arg_header(b)), | ||
| None => (path_arg_header(b), a_ptr0), | ||
| }; | ||
| f(a_ptr, b_ptr) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Root both path operands across the complete two-operand flow.
The current flow has two GC windows. Codegen can invalidate the first operand while it lowers the second operand. The runtime can invalidate the second heap-string operand while it materializes the first SSO operand.
crates/perry-runtime/src/path/value_args.rs#L101-L110: establish valid roots for both string operands before either materialization, then reload after allocation.crates/perry-codegen/src/expr/instance_misc1.rs#L882-L890: lowerPathJoinwithrooting::with_operands_rooted.crates/perry-codegen/src/expr/instance_misc1.rs#L902-L905: lowerPathWin32Joinwithrooting::with_operands_rooted.crates/perry-codegen/src/expr/instance_misc1.rs#L964-L992: root the two operands for Win32BasenameExt,ResolveJoin, andMatchesGlobbefore sequential lowering can expose stale values.crates/perry-codegen/src/expr/instance_misc1.rs#L1634-L1645: lowerPathBasenameExtwithrooting::with_operands_rooted.test-files/test_gap_7621_path_sso_operands.ts#L88-L106: add a GC-stress case with a heap first operand and an allocating second operand.
Proposed lowering pattern
- Expr::PathJoin(a, b) => {
- let a_box = lower_expr(ctx, a)?;
- let b_box = lower_expr(ctx, b)?;
+ Expr::PathJoin(a, b) => rooting::with_operands_rooted(ctx, &[a, b], |ctx, vals| {
+ let a_box = vals[0].clone();
+ let b_box = vals[1].clone();
let blk = ctx.block();
let result = blk.call(
I64,
"js_path_join_value",
&[(DOUBLE, &a_box), (DOUBLE, &b_box)],
);
- Ok(nanbox_string_inline(blk, &result))
- }
+ Ok(nanbox_string_inline(blk, &result))
+ })As per coding guidelines, “A GC-managed value's root store must dominate every subsequent operation that can collect.” Based on learnings, raw NaN-boxed values must be rooted and reloaded across allocating operations.
📍 Affects 3 files
crates/perry-runtime/src/path/value_args.rs#L101-L110(this comment)crates/perry-codegen/src/expr/instance_misc1.rs#L882-L890crates/perry-codegen/src/expr/instance_misc1.rs#L902-L905crates/perry-codegen/src/expr/instance_misc1.rs#L964-L992crates/perry-codegen/src/expr/instance_misc1.rs#L1634-L1645test-files/test_gap_7621_path_sso_operands.ts#L88-L106
🤖 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/path/value_args.rs` around lines 101 - 110, Root
both path operands before any materialization and reload them after allocating
operations in the runtime path flow around value_args.rs:101-110. In
instance_misc1.rs, wrap PathJoin (882-890), PathWin32Join (902-905), Win32
BasenameExt/ResolveJoin/MatchesGlob (964-992), and PathBasenameExt (1634-1645)
lowering with rooting::with_operands_rooted so raw operands remain valid across
sequential lowering. Add the requested GC-stress coverage for a heap first
operand and allocating second operand in
test_gap_7621_path_sso_operands.ts:88-106.
Sources: Coding guidelines, Learnings
… header
`path.resolve("/root", computedShortString)` threw ERR_INVALID_ARG_TYPE where
node returns the path. Every `path.*` codegen arm unboxed its operand with
`unbox_to_i64` (`bitcast double -> i64; and POINTER_MASK`) and handed the low 48
bits to a runtime entry that dereferences them as `*const StringHeader`. That is
the header for a HEAP string and the CHARACTERS for a small-string-optimized one
(SHORT_STRING_TAG, <= SHORT_STRING_MAX_LEN = 5 inline bytes) — so a literal
worked (interned onto the heap) and a computed short string did not. Bisected by
length: 5 bytes throws, 6 works.
Twelve arms were affected, not the five the issue named: resolve, resolve's fold
step, join, win32.join, normalize, extname, dirname, basename, basename(p, ext),
isAbsolute, parse, matchesGlob, plus the win32 sub-namespace equivalents. `parse`
and `matchesGlob` were SILENTLY wrong rather than throwing.
Single-operand arms now call `js_path_arg_header`, which materializes only the
SSO case and reproduces the old mask bit for bit for heap strings AND every
non-string — so each entry point keeps its own established non-string behaviour
(throw, or `unwrap_or_default`). It is deliberately not
`js_get_string_pointer_unified`, which coerces numbers to strings and would turn
`path.isAbsolute(5)` from a throw into `false`.
Two-operand arms take both operands NaN-boxed and unbox inside one runtime call
(`js_path_*_value`). Codegen cannot close that window itself: materializing the
first operand allocates, and `rooting::with_operands_rooted` hands the lowering
registers rather than slots, so the second operand's register is stale with no
re-read to reach for. The runtime entry roots the first operand across the
second's materialization via `RuntimeHandle::across_const`.
Validated locally: the new gap test is byte-identical to node 26.5.1 and exits 0
(it threw at line 2 before the fix); byte-identical again under PERRY_GC_ZEAL=1 +
PERRY_GC_PROTECT_FROMSPACE=1 with 402k copying minors observed; test_parity_path,
test_gap_node_path, test_gap_6371_path_lexical_semantics, test_gap_node_fs and
test_cli_simulation unchanged.
34e59f6 to
1e8f843
Compare
Audit before merge — verified, merged as v0.5.1358Reproduced the bug on main and the fix on this branch, bisecting the SSO That silent-empty case is the reason this went unreported for so long — it The resolution of the #7627 collision is better than what I asked for. I And declining Gates re-run here: runtime 1,890/0, codegen 694/0, all six lint scripts + Also worth keeping: your post-rebase sweep initially showed pre-fix behaviour |
1 participant
Closes #7621.
Root cause
crates/perry-codegen/src/expr/arrays_finds.rs:604(and eleven siblings)unboxed a
path.*operand withunbox_to_i64—bitcast double -> i64; and POINTER_MASK— and handed the low 48 bits to a runtime entry that dereferencesthem as
*const StringHeader.That is the header for a heap string (
STRING_TAG= 0x7FFF, payload = thepointer) and the characters for a small-string-optimized one
(
SHORT_STRING_TAG= 0x7FF9, payload = length + up toSHORT_STRING_MAX_LEN= 5inline bytes,
crates/perry-runtime/src/value/tags.rs:96). A literal is internedonto the heap, so
path.resolve("/root", "s1")worked; a computed short stringtakes the inline form, so
path.resolve("/root", seg(1))threwERR_INVALID_ARG_TYPE. The #214 class.Bisected by length on a pre-fix build — the boundary is exactly
SHORT_STRING_MAX_LEN:Scope: twelve arms, not the five the issue named
Every arm below was confirmed broken on a pre-fix build with a computed 2-byte
operand. Two of them were silently wrong rather than throwing, which is why
they had never been noticed:
resolve(a, b),resolve(p),join,win32.join,normalize,extname,dirname,basename,basename(p, ext),isAbsoluteTHROW ERR_INVALID_ARG_TYPEparse(p).base"""se"matchesGlob(p, pat)""ppath.relative,path.formatandpath.toNamespacedPathwere already fine —they take NaN-boxed values (
js_path_relative_checked, #2995), which is theprecedent this PR generalises.
The fix
Single-operand arms call the new
js_path_arg_header(value) -> i64. Itmaterialises only the SSO case and otherwise reproduces the old mask bit for
bit — heap strings and every non-string. That asymmetry is the point: each
entry point keeps its own established non-string behaviour (
js_path_jointhrows,
js_path_matches_globdefaults to"") unexamined and unchanged, so theblast radius is provably the SSO case alone.
It is deliberately not
js_get_string_pointer_unified(whichunbox_str_handlewraps). That helper coerces non-strings — a number comes backas
"5"— which would silently turnpath.isAbsolute(5)from Node'sERR_INVALID_ARG_TYPEthrow intofalse.Two-operand arms hand both operands to the runtime NaN-boxed
(
js_path_*_value). Codegen cannot close that window itself, which is the"why it is not a one-line fix" the issue flagged: materialising the first operand
allocates, and
rooting::with_operands_rootedyields the lowering registers,not slots, so the second operand's register is stale the instant the first is
materialised and there is no re-read to reach for (
RootedSlothas noread, bydesign — see the note in
crates/perry-codegen/src/rooting.rs:334). Doing bothunboxes inside one runtime call puts the window somewhere it can be closed
properly:
crates/perry-runtime/src/path/value_args.rsroots the first operand ina
RuntimeHandleScopeand re-reads it throughRuntimeHandle::across_const,never binding the pre-collection address.
A non-string operand is a masked garbage address, so it is deliberately not
rooted — handing the collector something to mark that is not an object would be a
worse bug than the one being fixed.
Did the fix need rooting discipline? Yes — and here is its honest status
The SSO half is sabotage-proven in both directions. The rooting half is
defensive, not instrument-proven, and the module docs say so at the site:
Disabling the SSO branch (
if false && ...is_short_string()) turnssso_operand_resolves_to_a_real_headerred and crashesboth_operands_survive_the_materialisation_window; the gap test threw at line2 with exit 1 on the pre-fix binary.
Reverting
across_constto a pre-boundlet a_ptr = a_ptr0;produced zerofaults: byte-identical on the gap test under
PERRY_GC_ZEAL=1+PERRY_GC_PROTECT_FROMSPACE=1(402,214 copying minors observed viaPERRY_GC_DIAG), and 0/400,000 mismatches on an SSO-pairpath.joinhammerunder
PERRY_GC_FORCE_EVACUATE=1+ from-space protection.That is not a fluke, it is structural: the window is allocation-to-allocation,
and a collection reached from inside an allocation runs with
GC_FLAG_IN_ALLOCset, which makes the copying minor ineligible. Nothingmoves at an allocation point today, so no available instrument can turn that
window red. The ordering is written the correct way because it is the shape the
invariant asks for and costs nothing — not because a probe caught it.
Validation (local; CI backlog is deep, so this is the evidence)
test-files/test_gap_7621_path_sso_operands.ts— byte-identical tonode --experimental-strip-typeson the pinned 26.5.1, exit 0 both sides.Covers both sides of the SSO boundary (lengths 2..10), computed and literal
operands, absolute and relative bases (asserted cwd-independently),
multi-segment and absolute-reset resolves, all twelve arms, the non-string
throw, and a nursery-churn loop around the two-operand window.
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1; the instrument was asserted live(
[gc-fromspace-protect] retired_set=#N, 402,214 sets).test_parity_path,test_gap_node_path,test_gap_6371_path_lexical_semantics,test_gap_node_fs,test_cli_simulationall byte-identical to node.perry-runtimeunit tests;cargo test -p perry-runtime --no-fail-fast1890 passed / 0 failed,
cargo test -p perry-codegen690 passed / 0 failed.cargo fmt --all -- --check,check_file_size.sh,addr_class_inventory.py,raw_handle_debt.py(998, unchanged — the new codeuses
across_const, not a bareget_raw_*_ptr),class_id_collisions.py,check_test_registration.py,gc_store_site_inventory.py,workspace_architecture.pyall pass.One pre-existing failure is untouched by this PR:
perry-codegen'slarge_local_array_push_inbounds_store_emits_precise_slot_barrierfails identically on pristine
origin/main(A/B'd in this worktree).Rebased onto v0.5.1357 (#7627) — semantic conflict resolved
#7627 migrated
expr/instance_misc1.rsonto the Layer 1 rooting API while thiswas open. Rebasing gave 4 conflicts, all in that one file; every other file
applied cleanly. The migrated form is the base and this fix layers on top:
PathJoin,PathWin32Join,PathBasenameExt, and thewin32
BasenameExt/ResolveJoin/MatchesGlobmethods) keep theirwith_operands_rootedwrapper and now emit no unbox at all — both operandsgo to the runtime NaN-boxed. That is strictly stronger than the migrated base:
it deletes the window from codegen rather than protecting it, so there is no
raw operand register in the arm for a collection to invalidate.
js_path_arg_headerinside the rooted region,immediately followed by its consumer.
On
with_operands_rooted_across_call(added by #7627 for exactly thisshape): I read its doc and deliberately did not use it. Its own closing
paragraph draws the line — "Use it only when the emitted step can re-enter user
code or enumerate an arbitrary object's own properties. For a helper that merely
allocates, the project's position (#7198) is that it cannot initiate a moving
collection, so a root there would be pure cost."
js_path_arg_headeronlymaterialises SSO bytes; it dispatches nothing and enumerates nothing. That is
also the same fact I measured independently before the rebase (a collection
reached from inside an allocation runs with
GC_FLAG_IN_ALLOC, so the copyingminor is ineligible), which is why the rooting sabotage arm could not be made to
fault. Two independent routes to the same conclusion.
And after the resolution the question is moot for the two-operand arms anyway:
they emit zero
js_path_arg_headercalls, so there is no codegen-side window tostate.
Re-verified after the rebase, not before
SSO bisect re-confirmed against the new base: built
origin/main(v0.5.1357) in this worktree — lengths 2–5 throw, 6+ work, and
parse/matchesGlobare still silently wrong. refactor(codegen): migrate instance_misc1 + logical_collections + map_set onto the Layer 1 rooting API (#7615) #7627 did not change the bug (it isIR-identical by construction).
Gap test byte-identical to node 26.5.1, exit 0; byte-identical again under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1with 402,214 copying minorsasserted live via
PERRY_GC_DIAG.test_parity_path,test_gap_node_path,test_gap_6371_path_lexical_semantics,test_gap_node_fs,test_cli_simulationall byte-identical.Root-dominance corpus, both gated modes, on the rebased build — 129/129
sources compiled, 149
.ll:and 40/40 seeded violations caught (the checker asserted able to fail);
--unrooted-allocas: 7867 gc-capable allocas, 0 violations.Both above the CI liveness floors (
--min-files 90 --min-binds 1500 --min-funcs 1200), allowlist still empty.Ledger:
instance_misc1.rsis inMIGRATED_MODULESnow and nothing addedhere names
expr::temp_root—migrated_modules_do_not_reach_past_the_rooting_apigreen, alongside itsplanted-violation and clears-the-migrated-form siblings.
cargo test -p perry-codegen --lib694 passed / 0 failed;-p perry-runtime --no-fail-fast1890 passed / 0 failed.Full lint set from
.github/workflows/test.yml:cargo fmt --all -- --check,check_file_size.sh,addr_class_inventory.py(+--self-test),raw_handle_debt.py(+--self-test, 998, unchanged),class_id_collisions.py,check_test_registration.py(+--self-test),gc_store_site_inventory.py(+--self-test),workspace_architecture.py,gap_snapshot.py --self-test,gc_gate_wiring_check.py— all green.Conflict-free against #7631 (
git merge-treeof the two heads exits 0), and#7626 adds no
perry_hir::Classliteral, so #7631's struct widening cannot breakit in either merge order.