From 4ffa079a38408938a9c77c4dd6c6a5b85d5cf46f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 5 Sep 2026 06:06:07 +0200 Subject: [PATCH] fix(gc): convert gc/census.rs to hot TLS and classify its holders (#9740) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `crates/perry-runtime/src/gc/census.rs` was the last file keeping `tls-budget` / `self-test-checkers` red on `main`. It declared two shipping raw `thread_local!` blocks, which `scripts/check_thread_locals.py` rejects, so every open PR inherited a red X that hid real failures. Converting them to `crate::perry_thread_local!` makes `gc_runtime_root_holders.py` see the declarations for the first time — it enumerates `perry_thread_local!`, so a raw block is audited by NEITHER gate. That is the wrong way round, and #9740's first finding: the declarations that skipped the convention are exactly the ones whose GC contract nobody checked. The file's `#[cfg(test)]` block is folded into the macro as well (the checker does not count it, but the inventory should see it), so no declaration in `census.rs` is now outside the inventory. `ARMED`, `SEQ`, `LABEL` and `TEST_PATH_OVERRIDE` are a flag, a counter, a `&'static str` and `#[cfg(test)]` storage — `not_a_gc_pointer` and `test_only`. `PASS1_MARKED` is what #9740 was filed for, and it needed a new verdict. It holds real GC header addresses, so `not_a_gc_pointer` — defined as an id, a counter, an epoch, a code address, .rodata or Rust-owned state — would be a false statement about it. `covered_elsewhere` names a scanner, and no root scan can observe this holder at all: it is empty outside the window. `open_gap` and `unverified` assert a defect or an open question and both fail the gate. The holder is correct precisely BECAUSE it is untraced — tracing the marked set would make the census a participant in the reachability it exists to report. `untraced_in_nonmoving_window` states that contract: a GC address, untraced, written and consumed inside one window in which nothing moves, nothing is freed and the mutator never runs, used as a key rather than dereferenced. Verified against the source rather than taken from the issue: both call sites in `gc/cycle.rs` are guarded by the identical predicate (`self.minor.is_none() && !progress_kind.is_budgeted()`, since `full_trace = self.minor.is_none()`), so pass 1 running implies pass 2 consuming it; not-minor means no evacuation and no `rewrite_forwarded_references` in that cycle; not-budgeted means `run_to_completion` drives every phase on an unbounded budget, so no mutator window exists between them; and the addresses are only `binary_search` keys at census.rs:389. The window is the entire safety argument, so the window is what the gate pins. An entry must name it with `window_opens` / `window_closes`, and `window_problems` checks against the holder's own source — the same shallow same-file method the script already uses for coverage — that both functions still exist and both still name the holder. Renaming a boundary, deleting one, or moving the write or the take out of it turns the gate red. Proven by sabotage: renaming `census_take_if_armed_at_full_sweep_start` and replacing the `take()` each produce the corresponding failure, and neutering `window_problems` makes `--self-test` object. `--self-test` drives all three rejections against a planted tree. #9740's other finding — that a raw `thread_local!` is invisible to the holder inventory for the ~120 files still on the allowlist — is a separate campaign and is left open on the issue. Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2 --- .../9740-census-tls-and-window-verdict.md | 28 ++++ crates/perry-runtime/src/gc/census.rs | 28 +++- scripts/gc_runtime_root_holders.json | 42 ++++++ scripts/gc_runtime_root_holders.py | 132 +++++++++++++++++- 4 files changed, 223 insertions(+), 7 deletions(-) create mode 100644 changelog.d/9740-census-tls-and-window-verdict.md diff --git a/changelog.d/9740-census-tls-and-window-verdict.md b/changelog.d/9740-census-tls-and-window-verdict.md new file mode 100644 index 0000000000..a293b936c2 --- /dev/null +++ b/changelog.d/9740-census-tls-and-window-verdict.md @@ -0,0 +1,28 @@ +**Convert `gc/census.rs` to hot TLS, and give the root-holder inventory a +verdict that fits its pass-1 snapshot.** `census.rs` was the last file keeping +`tls-budget` / `self-test-checkers` red on `main`: it declared two shipping +`thread_local!` blocks that `check_thread_locals.py` rejects, and every open PR +inherited the red X. + +Converting them made `gc_runtime_root_holders.py` see four declarations for the +first time — it enumerates `perry_thread_local!`, so a raw block escapes both +gates, which is backwards: the declarations that skipped the convention are the +ones whose GC contract nobody audited. `ARMED`, `SEQ` and `LABEL` are a flag, a +counter and a `&'static str`, classified `not_a_gc_pointer`; the file's +`#[cfg(test)]` block is folded into the macro too (`test_only`), so no +declaration in `census.rs` is left outside the inventory. + +`PASS1_MARKED` is the one #9740 was filed for. It holds real GC header +addresses, so `not_a_gc_pointer` — defined as an id, a counter, a code address, +.rodata or Rust-owned state — would have been a false statement about it, and +`covered_elsewhere` / `open_gap` / `unverified` fit no better. It is correct +because it is *untraced*: written at the end of mark propagation and consumed at +sweep entry of the same synchronous full cycle, under call-site guards that are +the identical predicate, in a window with no evacuation and no mutator resume, +and used only as `binary_search` keys. The new verdict +`untraced_in_nonmoving_window` says exactly that, and — because the window is +the whole safety argument — an entry must name the two functions that bound it +in `window_opens` / `window_closes`. The gate checks against the holder's own +source that both still exist and both still name the holder, so renaming a +boundary, deleting one, or moving the write or the take out of it fails the +gate. `--self-test` drives all three rejections. diff --git a/crates/perry-runtime/src/gc/census.rs b/crates/perry-runtime/src/gc/census.rs index 891035533f..32bb093ef4 100644 --- a/crates/perry-runtime/src/gc/census.rs +++ b/crates/perry-runtime/src/gc/census.rs @@ -43,17 +43,17 @@ static SIGNAL_PENDING: AtomicBool = AtomicBool::new(false); static SIGNAL_INSTALLED: AtomicBool = AtomicBool::new(false); static MAIN_THREAD: OnceLock = OnceLock::new(); -thread_local! { +crate::perry_thread_local! { static ARMED: Cell = const { Cell::new(false) }; static SEQ: Cell = const { Cell::new(0) }; static LABEL: RefCell<&'static str> = const { RefCell::new("manual") }; } -#[cfg(test)] -thread_local! { +crate::perry_thread_local! { /// Test-only per-thread override of the output path, so a unit test can /// enable the census without touching the process env (the env read is a /// process-wide OnceLock that any earlier collection would latch). + #[cfg(test)] static TEST_PATH_OVERRIDE: RefCell> = const { RefCell::new(None) }; } @@ -177,9 +177,29 @@ fn census_service_signal() { super::js_gc_collect(); } -thread_local! { +crate::perry_thread_local! { /// Pass-1 snapshot: sorted header addresses that were marked when mark /// propagation finished (see the module docs). + /// + /// These are real GC header addresses and they are deliberately NOT traced: + /// the marked set is what the census observes, so tracing it would make the + /// observer a participant in the reachability it reports. That is sound + /// only inside one window — written by `census_pass1_if_armed` at the end + /// of mark propagation, `take()`n by + /// `census_take_if_armed_at_full_sweep_start` at sweep entry of the SAME + /// synchronous full cycle. Both call sites in `gc/cycle.rs` are guarded by + /// the identical predicate (`self.minor.is_none() && !is_budgeted()`), so + /// pass 1 running implies pass 2 consuming it; not-minor means no + /// evacuation in that cycle and not-budgeted means no mutator window, so + /// nothing moves and nothing is freed while the vector is alive. The + /// addresses are only ever `binary_search` keys — compared, never + /// dereferenced. + /// + /// KEEP THE WINDOW SHUT. If the write or the take ever leaves those two + /// functions the addresses can go stale, and the census silently + /// misclassifies live objects. `scripts/gc_runtime_root_holders.json` + /// records this as `untraced_in_nonmoving_window` and pins both boundary + /// names, so widening the window turns that gate red (#9740). static PASS1_MARKED: RefCell>> = const { RefCell::new(None) }; } diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index 42a17ec138..bf5d4131dc 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -16,6 +16,16 @@ " not_a_gc_pointer - the stored value is an id, a counter, an epoch, a code address, a", " .rodata object, or Rust-owned state. Nothing for the collector.", " test_only - #[cfg(test)] storage; never live in a shipped binary.", + " untraced_in_nonmoving_window", + " - the stored value IS a GC heap address and is correct BECAUSE it is", + " untraced: written and consumed inside ONE window in which nothing", + " moves, nothing is freed and the mutator never runs, and used as a", + " key rather than dereferenced. `window_opens` / `window_closes` name", + " the two functions that bound it; the gate checks that both still", + " exist in the holder's file and both still name the holder, so", + " widening the window fails. Do NOT reach for this to avoid arguing", + " a holder is not a pointer \u2014 if the addresses outlive a collection,", + " the honest verdict is open_gap.", " open_gap - a real unrooted GC pointer. `issue` says where it is tracked; this", " verdict FAILS while old-page relocation ships enabled.", " unverified - enumerated, verdict NOT established. This also FAILS: an unknown", @@ -237,6 +247,38 @@ "verdict": "not_a_gc_pointer", "why": "Monotonic id counter for READ_LINES_REGISTRY keys. The registry's own heap values are visited by scan_filehandle_roots_mut (fs/filehandle.rs:65), reached from scan_fs_handle_roots_mut." }, + { + "file": "crates/perry-runtime/src/gc/census.rs", + "name": "ARMED", + "verdict": "not_a_gc_pointer", + "why": "#9740: `Cell` saying whether a PERRY_GC_CENSUS census is armed on this thread. Set by census_arm, cleared by census_take_if_armed_at_full_sweep_start; false unless PERRY_GC_CENSUS is set. A flag, never an address." + }, + { + "file": "crates/perry-runtime/src/gc/census.rs", + "name": "SEQ", + "verdict": "not_a_gc_pointer", + "why": "#9740: `Cell` monotonic census sequence number, incremented once per emitted census and written to the JSON line as `seq`. A counter, exactly what this verdict names." + }, + { + "file": "crates/perry-runtime/src/gc/census.rs", + "name": "LABEL", + "verdict": "not_a_gc_pointer", + "why": "#9740: `RefCell<&'static str>` holding what triggered the armed census \u2014 \"manual\" (explicit gc()), \"signal\" (SIGUSR2), or a test literal. `&'static str` from string literals is .rodata, which this verdict names; the census never constructs one from the JS heap." + }, + { + "file": "crates/perry-runtime/src/gc/census.rs", + "name": "PASS1_MARKED", + "verdict": "untraced_in_nonmoving_window", + "window_opens": "census_pass1_if_armed", + "window_closes": "census_take_if_armed_at_full_sweep_start", + "why": "#9740: sorted GC header addresses of the objects marked when mark propagation finished. These ARE heap addresses, so `not_a_gc_pointer` (an id, a counter, a code address, .rodata or Rust-owned state) would be a false statement about them; they are correct because they are untraced, not because they are not pointers. Written at the end of mark propagation and `take()`n at sweep entry of the SAME cycle \u2014 both call sites are guarded by the identical predicate `self.minor.is_none() && !self.progress_kind.is_budgeted()` (gc/cycle.rs:1182 and :1670, where `full_trace = self.minor.is_none()`), so pass 1 running implies pass 2 consuming it. Not minor means no evacuation and no rewrite_forwarded_references in that cycle; not budgeted means run_to_completion drives every phase on an unbounded budget, so no mutator window exists between the two. Nothing moves, nothing is freed, and the vector is used only as `binary_search` keys at census.rs:389 \u2014 compared, never dereferenced. Deliberately untraced: the marked set is the census's observation, and tracing it would make the observer a participant in the reachability it reports. Distinct from map.rs's MAP_COMPACTION_LOG, which is also address-keyed but LONG-LIVED and stays valid across moves via map_header_moved_for_gc; this holder has no such maintenance and needs none, because it cannot survive a move. The window is the whole safety argument, so the window is what is pinned." + }, + { + "file": "crates/perry-runtime/src/gc/census.rs", + "name": "TEST_PATH_OVERRIDE", + "verdict": "test_only", + "why": "#9740: `#[cfg(test)]` per-thread override of the PERRY_GC_CENSUS output path, so a unit test can enable a census without latching the process-wide OnceLock that reads the env var. Holds `Option<&'static str>` \u2014 a leaked path string \u2014 and is absent from shipped binaries." + }, { "file": "crates/perry-runtime/src/gc/trace.rs", "name": "FORWARDED_STUB_MEMBERSHIP_RECOVERIES", diff --git a/scripts/gc_runtime_root_holders.py b/scripts/gc_runtime_root_holders.py index cf4236e58c..25fd959990 100755 --- a/scripts/gc_runtime_root_holders.py +++ b/scripts/gc_runtime_root_holders.py @@ -83,6 +83,16 @@ (a stale exemption is how these gates rot — same rule as `scripts/gc_root_dominance_allowlist.json`). + One verdict carries a machine-checked side condition rather than only prose. + `untraced_in_nonmoving_window` (#9740) covers the holder whose stored value + really is a GC heap address and is correct BECAUSE it is untraced — written + and consumed inside one window in which nothing moves, nothing is freed and + the mutator never runs, and used as a key rather than dereferenced. Such an + entry must name the window with `window_opens` / `window_closes`, and both + functions must still exist in the holder's own file and still name the + holder. The safety argument is entirely "the window is shut", so the window + is what the gate pins; see `VERDICTS` for why no older verdict fit. + The identity-pinned frontier ---------------------------- @@ -125,6 +135,9 @@ * an inventory entry that no longer matches a declaration -> exit 1 * an `open_gap` or `unverified` verdict -> exit 1; old-page relocation ships enabled, so a known or unevaluated movable-address holder cannot be exempted +* an `untraced_in_nonmoving_window` verdict whose named window no longer bounds + the holder — a renamed or deleted boundary, or a boundary that no longer + touches the holder -> exit 1 * a frontier/rule-T holder not in the pinned `frontier` list -> exit 1 (ratchet up) * a `frontier` entry matching no holder -> exit 1 (ratchet down / stale) * fewer than MIN_HOLDERS declarations matched -> exit 2, because a regex that @@ -1037,10 +1050,35 @@ def reachable_text(call_pattern: re.Pattern) -> dict[Path, str]: "covered_elsewhere", # a registered scanner in ANOTHER file visits it "not_a_gc_pointer", # id, counter, epoch, code address, .rodata, Rust-owned "test_only", # #[cfg(test)] storage + "untraced_in_nonmoving_window", # GC addresses, untraced, bounded by a named window "open_gap", # a real unrooted GC pointer, with an issue "unverified", # enumerated, verdict not established — a dated TODO } +# `untraced_in_nonmoving_window` (#9740) is for the holder whose stored value IS +# a GC heap address and which is nonetheless correct BECAUSE it is untraced. +# +# `gc/census.rs`'s `PASS1_MARKED` is the case that forced it: a sorted vector of +# header addresses snapshotted at the end of mark propagation and consumed at +# sweep entry OF THE SAME synchronous full cycle, used only as `binary_search` +# keys — compared, never dereferenced, and deliberately not traced (tracing the +# marked set would make the census a participant in the reachability it exists +# to observe). Every other verdict would have been a false statement about it: +# `not_a_gc_pointer` is defined as an id/counter/code address/Rust-owned state +# and a heap address is none of those; `covered_elsewhere` names a scanner, and +# no root scan can even observe this holder, which is empty outside the window; +# `open_gap` and `unverified` assert a defect or an unanswered question, and +# both fail the gate. +# +# The verdict is only worth more than an exemption if the WINDOW is the thing +# pinned, so `window_opens` / `window_closes` name the two functions that bound +# it and `window_problems` checks, against the holder's own source, that both +# still exist and both still name the holder. Renaming a boundary, deleting one, +# or moving the write or the take out of it turns this gate red — which is the +# regression the verdict exists to catch, since a holder of stale heap addresses +# is safe only for as long as its window stays shut. +WINDOW_FIELDS = ("window_opens", "window_closes") + # `unverified` is the one verdict that classifies nothing. It exists so a hole # the gate CAN see is named rather than silent, and it is capped so the list # cannot quietly become the whole inventory — at which point the gate would be a @@ -1106,7 +1144,43 @@ def apply_frontier( return unpinned, stale -def inventory_problems(inventory: list[dict]) -> list[str]: +def window_problems(entry: dict, root: Path, label: str) -> list[str]: + """Check a window verdict against the holder's own source. + + The claim `untraced_in_nonmoving_window` makes is not "this value is + harmless" but "this value never outlives the window between these two + functions". That is checkable in the same shallow, same-file way the rest of + this script computes coverage: both boundaries must still be functions in + the holder's file, and both must still name the holder. A `why` string + cannot notice that the take moved; this can. + """ + problems: list[str] = [] + path = root / entry["file"] + if not path.exists(): + return [ + f"{label}: {entry['file']} does not exist, so the window this verdict " + f"names cannot be checked" + ] + bodies = function_bodies(path.read_text(encoding="utf-8", errors="replace")) + name = entry["name"] + for field in WINDOW_FIELDS: + fn = entry[field].strip() + body = bodies.get(fn) + if body is None: + problems.append( + f"{label}: {field} names `{fn}`, which is not a function in " + f"{entry['file']} — the window's boundary was renamed or removed, so " + f"this verdict no longer describes the holder" + ) + elif not re.search(rf"\b{re.escape(name)}\b", body): + problems.append( + f"{label}: `{fn}` ({field}) no longer mentions `{name}` — the holder " + f"outlives the window the verdict was granted for" + ) + return problems + + +def inventory_problems(inventory: list[dict], root: Path | None = None) -> list[str]: """Structural checks on the inventory itself. Without these, `apply_inventory` accepts any object carrying a matching @@ -1140,6 +1214,16 @@ def inventory_problems(inventory: list[dict]) -> list[str]: ) if verdict == "open_gap" and not (entry.get("issue") or "").strip(): problems.append(f"{label}: open_gap must cite an `issue`") + if verdict == "untraced_in_nonmoving_window": + missing = [f for f in WINDOW_FIELDS if not (entry.get(f) or "").strip()] + if missing: + problems.append( + f"{label}: untraced_in_nonmoving_window must name the window that " + f"bounds the holder — missing {', '.join(missing)}. Without both " + f"boundaries this verdict is an exemption, not a contract" + ) + elif root is not None: + problems.extend(window_problems(entry, root, label)) if verdict in {"open_gap", "unverified"}: problems.append( f"{label}: `{verdict}` is not a shippable old-page relocation verdict. " @@ -1221,7 +1305,7 @@ def report(root: Path, quiet: bool = False) -> int: inventory = load_inventory(INVENTORY_PATH) unclassified, stale = apply_inventory(holders, inventory) - malformed = inventory_problems(inventory) + malformed = inventory_problems(inventory, root) frontier = load_frontier(INVENTORY_PATH) frontier_new, frontier_stale = apply_frontier( holders, frontier, registered_scanners, inventory @@ -1893,7 +1977,7 @@ def expect_absent(rel: str, name: str, why: str) -> None: ", ".join(f"{e['file']}:{e['name']}" for e in frontier_stale[:5]), ) ) - failures.extend(inventory_problems(inventory)) + failures.extend(inventory_problems(inventory, REPO_ROOT)) # …and the structural checker must itself be able to fail. long_why = "x" * 30 for bad, expect in ( @@ -1906,6 +1990,48 @@ def expect_absent(rel: str, name: str, why: str) -> None: failures.append( f"inventory_problems did not reject the malformed {expect!r} entry: {bad}" ) + # The window verdict's boundaries are checked against real source, so its + # rejections need a real tree. Planting one is what keeps the check from + # degrading into "the field is non-empty". + with tempfile.TemporaryDirectory() as tmp: + window_root = Path(tmp) + (window_root / "src").mkdir() + (window_root / "src" / "w.rs").write_text( + "crate::perry_thread_local! {\n" + " static SNAP: RefCell>> = const { RefCell::new(None) };\n" + "}\n" + "fn opens() {\n SNAP.with(|p| *p.borrow_mut() = Some(Vec::new()));\n}\n" + "fn closes() {\n SNAP.with(|p| p.borrow_mut().take());\n}\n" + "fn unrelated() {\n let _ = 1;\n}\n", + encoding="utf-8", + ) + bounded = { + "file": "src/w.rs", + "name": "SNAP", + "verdict": "untraced_in_nonmoving_window", + "why": long_why, + "window_opens": "opens", + "window_closes": "closes", + } + if inventory_problems([bounded], window_root): + failures.append( + "the window verdict rejected a holder its own named window bounds" + ) + for field, value, expect in ( + ("window_opens", "", "window_opens"), + ("window_closes", "renamed", "not a function"), + ("window_closes", "unrelated", "no longer mentions"), + ): + broken = dict(bounded, **{field: value}) + if not any( + expect in problem + for problem in inventory_problems([broken], window_root) + ): + failures.append( + f"the window verdict accepted {field}={value!r}, which widens the " + f"window it claims to be bounded by" + ) + over_cap = [ {"file": f"f{i}", "name": "N", "verdict": "unverified", "why": long_why} for i in range(MAX_UNVERIFIED + 1)