fix(lint): clear the GC store-site and address-classification gates (#7258, #7259) - #7273
Conversation
Comment-only in the runtime/codegen, plus one rule-precision fix in the scanner. No generated code and no runtime behaviour changes. Triage of the 19: * 5 were PHANTOM. `classify_rust_store` searched the atomic-store regexes against `call_window` (6 lines forward) unanchored, so one `cache.store(...)` was re-reported from each of the five lines above it — including `continue;` and a bare `}`. Anchoring the match to the head line keeps split-call coverage and reports the site once. * 3 were genuinely new runtime sites needing a class: - array/alloc.rs, array/push_pop.rs (#7138): TAG_HOLE fill of freshly allocated, not-yet-published capacity -> INIT. - array/indexing.rs (#7071): the collector's own rewrite of a registered side-table root slot, mutator stopped -> ROOT. * 11 were codegen stores. Four already carried a POINTER_FREE marker that the #6915 `value_is_canonical_raw_f64` branch split pushed out of the +/-6-line window; the other seven (finite-check-guarded field stores, typed-array backing-store element stores) had prose rationale but not the canonical marker form. Self-tests: the new `proto_cache_scan` case fails on the pre-fix unanchored logic with exactly the five phantom lines, so the regression guard is live rather than merely non-throwing.
Three hand-rolled address floors landed on 2026-07-30 without going through `value::addr_class`, tripping the ratchet: child_process/value_util.rs `> 0x10000` / `<= 0x10000` (8fed172) fs/dirent.rs `< 0x1000` (7a4a866) Both are real, not bookkeeping. `js_get_string_pointer_unified` forwards a POINTER_TAG payload verbatim, and `JSValue::as_pointer` does the same — those payloads carry registry handle ids as well as heap pointers. A `0x1000` / `0x10000` floor sits an order of magnitude below `HANDLE_BAND_MAX`, so a fetch (0x40000..0xE0000), zlib (0xE0000..0xF0000) or proxy (0xF0000..0x100000) id sailed through into a `StringHeader`/`ObjectHeader` dereference — the Linux-only fault class of #1843/#4004/#6271 that macOS's high allocation base hides. Converted every handle-floor site in both files (including the two grandfathered ones, same defect, same fix) to `is_handle_band` / `is_above_handle_band`, which also subsumes the redundant null checks that followed. `cp_raw_slot_is_heap_ptr` keeps an explicit `raw > 0` so a negative i64 slot is not turned into a huge "above the band" value by `as usize`. Separately, the `O_SYMLINK` allowlist entry was keyed on ONE of the two spellings in constants.rs, so `("O_SYMLINK", (0x200000) as f64),` failed the gate while `"O_SYMLINK" => Some(0x200000),` was suppressed. Re-keyed on the constant name; a self-test now asserts BOTH spellings are covered and fails against the old entry. Ratchet baseline regenerated: 566 -> 544 sites, 249 entries. Zero increases and zero new keys (verified by diffing the key/count maps) — purely the slack that had accumulated while lint was red. 3 of the 22 are this change; 11 are `perry-stdlib/src/http.rs`, whose sites moved into `crates/perry-ext-http/` when #6826 merged it into the HTTP extension. `SCAN_ROOTS` covers only perry-runtime and perry-stdlib, so those sites are no longer audited at all — tracked separately, not silently absorbed here.
📝 WalkthroughWalkthroughThis change audits GC store sites, anchors multiline store detection, replaces hand-rolled address floors with handle-band predicates, updates the address-class allowlist and baseline, and adds regression tests. ChangesGC store audit
Handle-band validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
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
🤖 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/fs/dirent.rs`:
- Around line 132-140: Update the filesystem option-field access around
options_value and js_object_get_field_by_name to use
options_bool_field(options_value, b"withFileTypes") instead of constructing a
field key and dereferencing obj_ptr directly. Ensure the helper roots and
reloads options_value across any allocating operation, while preserving the
existing withFileTypes behavior.
🪄 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: ddfb04e3-664d-4256-b0d3-77c117b1520c
📒 Files selected for processing (15)
changelog.d/7273-lint-gc-store-and-addr-class-gates.mdcrates/perry-codegen/src/expr/index.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/expr/proven_view_access.rscrates/perry-runtime/src/array/alloc.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/child_process/value_util.rscrates/perry-runtime/src/fs/dirent.rsscripts/addr_class_allowlist.txtscripts/addr_class_inventory.pyscripts/addr_class_ratchet_baseline.txtscripts/gc_store_site_inventory.py
| // #7259: a POINTER_TAG payload can be a registry handle id rather than a | ||
| // heap address, and `< 0x1000` sits an order of magnitude below | ||
| // `HANDLE_BAND_MAX` — fetch/zlib/proxy ids passed it and were dereferenced | ||
| // as an ObjectHeader (the Linux-only fault class of #1843/#4004/#6271). | ||
| // `is_handle_band` also subsumes the null check that used to follow. | ||
| if crate::value::addr_class::is_handle_band(raw_ptr) { | ||
| return false; | ||
| } | ||
| let obj_ptr = raw_ptr as *const crate::object::ObjectHeader; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Root options_value before constructing the field key.
js_string_from_bytes at Line 141 can collect. obj_ptr can then be stale when Line 142 calls js_object_get_field_by_name.
Replace this duplicated read with options_bool_field(options_value, b"withFileTypes"). That helper roots and reloads options_value.
As per coding guidelines, GC-managed pointers must remain rooted across every operation that can collect. Based on learnings, options_field_value provides the required GC-safe filesystem option-field access.
🤖 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/fs/dirent.rs` around lines 132 - 140, Update the
filesystem option-field access around options_value and
js_object_get_field_by_name to use options_bool_field(options_value,
b"withFileTypes") instead of constructing a field key and dereferencing obj_ptr
directly. Ensure the helper roots and reloads options_value across any
allocating operation, while preserving the existing withFileTypes behavior.
Sources: Coding guidelines, Learnings
|
Good catch, and the underlying hazard is real — but it is pre-existing and out of scope here, so I have filed it as #7274 rather than folding it in. To be precise about what is wrong: the problem is not the direct field access versus The correct fix is the one the sibling Keeping it separate is deliberate. #7273 is a |
CI triage: both red checks are pre-existing on
|
1 participant
Clears steps 12 and 13 of the
lintjob. Both scripts exit 0 locally; both exit 1 onorigin/main.Fixes #7258. Fixes #7259.
lintruns its steps in order and stops at the first failure, and step 9 (public benchmark evidence freshness, #7257) fails first — so steps 12 and 13 have never actually executed in CI and neither failure appears in any Actions log. Both were reproduced by running the scripts directly.Triage
Each finding was placed in one of three buckets before being touched.
#7258 — GC store-site inventory (19 findings)
5 were phantom — a rule defect.
classify_rust_storesearched the atomic-store regexes againstcall_window(a 6-line forward window) without anchoring, so onecache.store(...)was re-reported from each of the five lines above it. The reported "store sites" includedcontinue;and a bare}.anchored_searchnow requires the match to begin inside the head line, which keeps the split-call coverage the window exists for (CACHE.store(…);) and reports each site exactly once, at the line that contains it.3 were legitimate new runtime sites needing an audit class. All arrived after the gate was written:
array/alloc.rs:55INITTAG_HOLEfill of a just-allocated array not yet reachable from any root; a non-pointer sentinel, so no old value to remember and no edge to recordarray/push_pop.rs:101INIT[old_capacity, new_capacity)slack — storage this allocation has never publishedarray/indexing.rs:111ROOTvisit_usize_slotreturns true only when it relocated the object, and the value written is the visitor's own to-space address11 were codegen stores. Four (
index.rs×3,index_set.rs) already carried aGC_STORE_AUDIT(POINTER_FREE)marker — #6915'svalue_is_canonical_raw_f64branch split pushed the else-branch store 9–10 lines below it, outside the ±6-line window. The other seven had prose rationale but not the canonical marker form:instance_misc1.rs,property_set.rs— reached only whenemit_plain_finite_number_checkproved the value's exponent is not all-ones. Every NaN-box tag (INT32/STRING/POINTER/BIGINT) has an all-ones exponent, so the stored value is a genuine unboxed double and cannot be a GC pointer.proven_view_access.rs×5 — all five arms store into the typed array's backing store (view.data_slot), whose elements are raw numeric bytes. This is the codegen-side counterpart of the runtime carve-out the script already makes for thetypedarray/typedarray_view/buffermodules (is_pointer_free_module).No generated code and no runtime behaviour changed — the runtime/codegen half of that commit is comments only.
#7259 — address-classification audit
2 ratchet regressions: real defects, fixed properly. Three hand-rolled floors landed on 2026-07-30 (
8fed172cfchild_process,7a4a86677fs) without going throughvalue::addr_class.js_get_string_pointer_unifiedforwards aPOINTER_TAGpayload verbatim andJSValue::as_pointerdoes the same — those payloads carry registry handle ids as well as heap pointers. A0x1000/0x10000floor sits an order of magnitude belowHANDLE_BAND_MAX, so a fetch (0x40000..0xE0000), zlib (0xE0000..0xF0000) or proxy (0xF0000..0x100000) id sailed straight into aStringHeader/ObjectHeaderdereference — the Linux-only fault class of #1843/#4004/#6271 that macOS's high allocation base hides.Every handle-floor site in both files was converted, including the two grandfathered ones (same defect, same one-line fix), so both baseline entries drop to zero rather than being re-pinned at a lower number.
is_handle_bandalso subsumes the redundantis_null()checks that followed.cp_raw_slot_is_heap_ptrkeeps an explicitraw > 0so a negativei64slot isn't turned into a huge "above the band" value byas usize.1 stale allowlist substring.
constants.rsspells the same POSIX flag two ways; the entry matched only one:Re-keyed on the constant name. The entry is still path-prefixed to
constants.rsand still requires theO_SYMLINKtoken, so any other band literal in that file still fails the gate. The pre-splitobject/native_module.rsentry was dropped — that file now contains zeroO_SYMLINKmentions and zero band literals.Ratchet baseline: 566 → 544 sites, 256 → 249 entries. Verified zero increases and zero new keys by diffing the
(rule, path) → countmaps, so this is purely slack that accumulated whilelintwas red. Only 3 of the 22 are this PR; the stale-entry warnings the gate printed after every pass are now gone.Gates assert their own subject
Per CLAUDE.md, both new regression tests were confirmed to fail without the fix, not merely to pass with it:
proto_cache_scancase fails on the pre-anchoring logic with exactly the five phantom lines;O_SYMLINKcase fails against the pre-fix allowlist with exactly the uncovered spelling.Finding-set diff vs
origin/mainResolved, not reshaped — every removal is a finding that no longer exists or is now classified, and nothing new was suppressed.
origin/mainnumbers come from a clean detached worktree.origin/maingc_store_site_inventory.pyaddr_class_inventory.pyThe anchoring change was measured in isolation first: it removes exactly the five phantom
array/indexing.rslines and nothing else, leaving the real store at line 111 to be classified on its merits.#7271 and #7265 landed mid-work; both gates' finding sets at
defa4d601are byte-identical to those ata25077491, so neither affects this.Left open, deliberately
addr_class_inventory.py'sSCAN_ROOTScovers onlyperry-runtimeandperry-stdlib, while its siblinggc_store_site_inventory.pyalso globscrates/perry-ext-*. When #6826 movedperry-stdlib/src/http.rsinto the HTTP extension, its 11handle-floorsites moved out of the gate's field of view rather than being fixed — the "gate runs but its subject never did" pattern. 18 sites across the ext crates are unaudited, 8 of themband-literal(not ratcheted, so they would hard-fail until each is individually justified). That per-site audit is its own piece of work and is not folded in here: tracked as #7272.This PR does not by itself turn
lintgreen — step 9 (#7257, blocked on #7264) still fails ahead of these two.Verification
python3 scripts/gc_store_site_inventory.py --self-test && python3 scripts/gc_store_site_inventory.py→ 0python3 scripts/addr_class_inventory.py --self-test && python3 scripts/addr_class_inventory.py→ 0cargo fmt --all -- --checkcleanscripts/check_file_size.shcleancargo build --profile perry-dev -p perry -p perry-runtime-static -p perry-stdlib-staticclean (the oneperry-runtimewarning,gc/mod.rs:70barrier_arming::*, is pre-existing onmainfrom perf(gc): stop maintaining a remembered set nothing has read yet (#7187 Phase A) #7250 and untouched here)Not verified: no gap/parity sweep was run. The #7258 half cannot affect behaviour (comments only). The #7259 half changes three runtime address guards in a strictly more rejecting direction — the gate's own rationale notes this conversion is safety-monotonic ("it can only reject MORE addresses, never dereference more") — but the
child_processspawn/fork option-slot path andfs.readdir(withFileTypes)were not exercised end-to-end, and the defect being fixed is Linux-only, so a macOS run would not have demonstrated it either way.Summary by CodeRabbit
Bug Fixes
Documentation
Tests