diff --git a/changelog.d/7824-thread-local-policy-gate.md b/changelog.d/7824-thread-local-policy-gate.md new file mode 100644 index 0000000000..fbaa7651ad --- /dev/null +++ b/changelog.d/7824-thread-local-policy-gate.md @@ -0,0 +1,21 @@ +**The thread-local policy gate is green again, and now measures what it claims to.** +`scripts/check_thread_locals.py` was failing on `main`, which blocked +`tls-budget`'s documented promotion to a required context — a gate that runs, +reports failure, and cannot block anything. Two of the eight raw `thread_local!` +blocks were real, and they were not the ones reported: `array/indexing.rs`, +`map.rs`, `set.rs` and `registry_latch_probes.rs` are all `#[cfg(test)]` and do +not exist in a shipping build, while `gc/schedule.rs`'s `SAFEPOINT_COUNTER` +(every safepoint) and `SCHEDULE_NEXT_CANDIDATE_BYTES` (every poll) were live and +paying `_tlv_get_addr` on Darwin. Those two now use +`crate::perry_thread_local!`. The scan no longer counts `#[cfg(test)]` +declarations at all — recording one as "cold" records the wrong fact, since it +is not cold but absent — covering the attribute directly above a block, an +inline `#[cfg(test)] mod`, and a whole file declared `#[cfg(test)] mod ;` +(closed transitively, so `gc/tests/mod.rs` carries its subtree). Because an +over-broad exclusion would make the gate pass *by seeing less*, `--self-test` +now checks each shape in both directions — gated is skipped, and removing the +gate makes the same declaration fail again — proving six rejection directions +instead of four. The allowlist regeneration is provably one-way (101 → 91 +entries, 157 → 129 blocks, nothing added and no count increased), which also +retires three already-stale entries: `gc/zeal.rs` from #7741's removal of +`PERRY_GC_ZEAL`, plus `arena/quarantine.rs` and `gc/oldgen_defrag.rs`. (#7814) diff --git a/crates/perry-runtime/src/gc/schedule.rs b/crates/perry-runtime/src/gc/schedule.rs index e5addfde94..b645095f03 100644 --- a/crates/perry-runtime/src/gc/schedule.rs +++ b/crates/perry-runtime/src/gc/schedule.rs @@ -129,7 +129,7 @@ static SCHEDULE_FORCED: AtomicU64 = AtomicU64::new(0); /// other test's `before + 1` flaky. static SCHEDULE_SAFEPOINTS: AtomicU64 = AtomicU64::new(0); -thread_local! { +crate::perry_thread_local! { /// The monotonically increasing safepoint ordinal this thread's schedule is /// a function of. Thread-local on purpose — see the determinism note above. static SAFEPOINT_COUNTER: std::cell::Cell = const { std::cell::Cell::new(0) }; @@ -512,7 +512,7 @@ impl Drop for ScheduleStrideGuard { } } -thread_local! { +crate::perry_thread_local! { /// From-space high-water mark at or above which the next poll-arm candidate /// is due. Per-thread because the arena it measures is. /// diff --git a/scripts/check_thread_locals.py b/scripts/check_thread_locals.py index 6b801c6656..a5039ac990 100755 --- a/scripts/check_thread_locals.py +++ b/scripts/check_thread_locals.py @@ -32,6 +32,24 @@ * `--self-test` runs both directions against synthetic trees, so the checker itself cannot quietly stop being able to say no. +`#[cfg(test)]` DECLARATIONS ARE OUT OF SCOPE BY CONSTRUCTION +=========================================================== + +A `thread_local!` that only exists under `#[cfg(test)]` cannot cost +`_tlv_get_addr` in a shipping build, because it is not in one. Recording such a +declaration as "cold" would be recording the wrong fact — it is not cold, it is +*absent* — and it would spend the allowlist's credibility on entries no one can +ever act on. Three gated shapes are therefore not counted at all: + +* `#[cfg(test)]` immediately above the block, +* the block nested inside a `#[cfg(test)] mod … { … }` in the same file, +* the block's whole FILE being a test module, i.e. some `lib.rs`/`mod.rs` + declares it `#[cfg(test)] mod ;`. + +This is a *static* fact the scan can see, unlike hot-vs-cold. It is also the +direction that can go wrong quietly, so `--self-test` checks that removing the +gate makes the same declaration fail again. + WHAT IT DOES NOT DO =================== @@ -77,6 +95,17 @@ # macro without infinite regress. EXCLUDED = {"crates/perry-runtime/src/tls_hot.rs"} +# `#[cfg(test)] mod ;` — the whole file is a test module. +CFG_TEST_MOD_RE = re.compile( + r"(?m)^[ \t]*#\[cfg\(test\)\]\s*\n[ \t]*(?:pub(?:\([^)]*\))?\s+)?mod\s+([A-Za-z_0-9]+)\s*;" +) +# Any out-of-line `mod ;`, gated or not — the edges of the module tree. +ANY_MOD_RE = re.compile(r"(?m)^[ \t]*(?:pub(?:\([^)]*\))?\s+)?mod\s+([A-Za-z_0-9]+)\s*;") +# `#[cfg(test)] mod {` — an inline test module, whose body is skipped. +CFG_TEST_INLINE_MOD_RE = re.compile( + r"(?m)^[ \t]*#\[cfg\(test\)\]\s*\n[ \t]*(?:pub(?:\([^)]*\))?\s+)?mod\s+[A-Za-z_0-9]+\s*\{" +) + def block_bodies(src: str, pattern: re.Pattern[str]) -> list[str]: """Bodies of every macro block `pattern` starts, brace-matched.""" @@ -97,10 +126,86 @@ def block_bodies(src: str, pattern: re.Pattern[str]) -> list[str]: return bodies +def brace_span(src: str, start: int) -> tuple[int, int]: + """`(open, close)` offsets of the brace-matched block opening at/after `start`.""" + i = src.index("{", start) + depth = 0 + j = i + while j < len(src): + if src[j] == "{": + depth += 1 + elif src[j] == "}": + depth -= 1 + if depth == 0: + break + j += 1 + return i, j + + +def cfg_test_module_files(root: Path, crates: list[str]) -> set[str]: + """Files whose entire module cannot exist in a shipping build. + + Seeded from every `#[cfg(test)] mod ;`, then closed transitively: a + plain `mod y;` inside an already-test-only file is test-only too, which is + what makes `gc/tests/mod.rs` carry its whole subtree. A `mod x;` in `a/b.rs` + (or `a/b/mod.rs`) resolves to `a/b/x.rs` or `a/b/x/mod.rs`; both spellings + are recorded, and a miss is simply a file that stays in scope. + """ + declares: dict[str, list[tuple[str, bool]]] = {} + for crate in crates: + base = root / crate + for dirpath, _dirs, files in os.walk(base): + for name in sorted(files): + if not name.endswith(".rs"): + continue + path = Path(dirpath) / name + rel = str(path.relative_to(root)) + src = path.read_text() + parent = path.parent if name in ("lib.rs", "mod.rs") else path.with_suffix("") + gated = set(CFG_TEST_MOD_RE.findall(src)) + edges = [] + for stem in set(ANY_MOD_RE.findall(src)): + for candidate in (parent / f"{stem}.rs", parent / stem / "mod.rs"): + if candidate.exists(): + edges.append((str(candidate.relative_to(root)), stem in gated)) + declares[rel] = edges + + test_only = {child for edges in declares.values() for child, gated in edges if gated} + changed = True + while changed: + changed = False + for rel in test_only & declares.keys(): + for child, _gated in declares[rel]: + if child not in test_only: + test_only.add(child) + changed = True + return test_only + + +def shipping_raw_blocks(src: str) -> int: + """Raw `thread_local!` blocks that survive into a non-test build. + + Skips a block carrying `#[cfg(test)]` directly above it, and any block + inside an inline `#[cfg(test)] mod … { … }`. + """ + gated_spans = [brace_span(src, m.start()) for m in CFG_TEST_INLINE_MOD_RE.finditer(src)] + count = 0 + for m in RAW_RE.finditer(src): + if any(open_at < m.start() < close_at for open_at, close_at in gated_spans): + continue + preceding = src[: m.start()].rstrip() + line = preceding[preceding.rfind("\n") + 1 :].strip() + if line == "#[cfg(test)]": + continue + count += 1 + return count + + def scan(root: Path, crates: list[str]) -> tuple[dict[str, int], int]: """Raw `thread_local!` blocks per file, and total hot declarations.""" raw: dict[str, int] = {} hot_declarations = 0 + test_only_files = cfg_test_module_files(root, crates) for crate in crates: base = root / crate for dirpath, _dirs, files in os.walk(base): @@ -113,9 +218,9 @@ def scan(root: Path, crates: list[str]) -> tuple[dict[str, int], int]: hot_declarations += sum( len(DECL_RE.findall(body)) for body in block_bodies(src, HOT_RE) ) - if rel in EXCLUDED: + if rel in EXCLUDED or rel in test_only_files: continue - count = len(RAW_RE.findall(src)) + count = shipping_raw_blocks(src) if count: raw[rel] = count return raw, hot_declarations @@ -251,12 +356,48 @@ def self_test() -> int: ) if not any("HOT_SLOT_CAPACITY" in p for p in verify(root, CRATES, allowlist)): failures.append("exceeding HOT_SLOT_CAPACITY passed") + (src_dir / "hot.rs").write_text( + "crate::perry_thread_local! { static B: u8 = const { 0 }; }\n" + ) + + # 5. The three `#[cfg(test)]` shapes are out of scope — and, the half + # that can go wrong quietly, REMOVING the gate puts them back in it. + (src_dir / "cold.rs").write_text("thread_local! { static A: u8 = const { 0 }; }\n") + write_allowlist(root, CRATES, allowlist) + gated = { + "attribute": "#[cfg(test)]\nthread_local! { static G: u8 = const { 0 }; }\n", + "inline mod": ( + "#[cfg(test)]\nmod t {\n" + " thread_local! { static G: u8 = const { 0 }; }\n" + "}\n" + ), + } + for shape, body in gated.items(): + (src_dir / "gated.rs").write_text(body) + if verify(root, CRATES, allowlist): + failures.append(f"a `#[cfg(test)]` {shape} declaration was counted") + (src_dir / "gated.rs").write_text(body.replace("#[cfg(test)]\n", "")) + if not verify(root, CRATES, allowlist): + failures.append(f"an UNGATED {shape} declaration passed") + (src_dir / "gated.rs").unlink() + + # 6. A whole file declared `#[cfg(test)] mod ;` is out of scope, + # and drops back in when the parent stops gating it. + (src_dir / "probes.rs").write_text( + "thread_local! { static G: u8 = const { 0 }; }\n" + ) + (src_dir / "lib.rs").write_text("#[cfg(test)]\nmod probes;\n") + if verify(root, CRATES, allowlist): + failures.append("a `#[cfg(test)] mod ;` file was counted") + (src_dir / "lib.rs").write_text("mod probes;\n") + if not verify(root, CRATES, allowlist): + failures.append("an ungated `mod ;` file passed") for f in failures: print(f"SELF-TEST FAILED: {f}", file=sys.stderr) if failures: return 1 - print("self-test: the checker can fail in all four directions") + print("self-test: the checker can fail in all six directions") return 0 diff --git a/scripts/thread_local_cold_allowlist.json b/scripts/thread_local_cold_allowlist.json index afde09619b..de275ddcdd 100644 --- a/scripts/thread_local_cold_allowlist.json +++ b/scripts/thread_local_cold_allowlist.json @@ -1,11 +1,10 @@ { "_comment": "Files still declaring raw `thread_local!`. Every entry is a declaration that pays `_tlv_get_addr` on Darwin; the count is a ratchet, so adding one to an already-listed file fails too. New code should use `crate::perry_thread_local!` \u2014 see crates/perry-runtime/src/tls_hot.rs. Regenerate with scripts/check_thread_locals.py --update.", - "_hot_declarations": 157, + "_hot_declarations": 160, "files": { "crates/perry-runtime/src/agent.rs": 1, - "crates/perry-runtime/src/arena/block.rs": 3, + "crates/perry-runtime/src/arena/block.rs": 2, "crates/perry-runtime/src/arena/page_meta.rs": 2, - "crates/perry-runtime/src/arena/quarantine.rs": 1, "crates/perry-runtime/src/async_context.rs": 2, "crates/perry-runtime/src/async_hooks.rs": 3, "crates/perry-runtime/src/builtins/arithmetic.rs": 1, @@ -33,36 +32,29 @@ "crates/perry-runtime/src/fs/stream.rs": 1, "crates/perry-runtime/src/gc/barrier.rs": 2, "crates/perry-runtime/src/gc/barrier_arming.rs": 1, - "crates/perry-runtime/src/gc/cycle.rs": 2, + "crates/perry-runtime/src/gc/cycle.rs": 1, "crates/perry-runtime/src/gc/dirty_page_cache.rs": 1, "crates/perry-runtime/src/gc/fromspace_scan.rs": 1, - "crates/perry-runtime/src/gc/layout.rs": 2, + "crates/perry-runtime/src/gc/layout.rs": 1, "crates/perry-runtime/src/gc/layout_tables.rs": 1, "crates/perry-runtime/src/gc/malloc.rs": 2, - "crates/perry-runtime/src/gc/mod.rs": 3, + "crates/perry-runtime/src/gc/mod.rs": 2, "crates/perry-runtime/src/gc/old_free.rs": 1, - "crates/perry-runtime/src/gc/oldgen_defrag.rs": 1, - "crates/perry-runtime/src/gc/policy.rs": 10, - "crates/perry-runtime/src/gc/promote_in_place.rs": 2, + "crates/perry-runtime/src/gc/policy.rs": 6, + "crates/perry-runtime/src/gc/promote_in_place.rs": 1, "crates/perry-runtime/src/gc/roots/scan_mode.rs": 1, "crates/perry-runtime/src/gc/roots/shadow_stack.rs": 2, "crates/perry-runtime/src/gc/roots/temp_roots.rs": 1, "crates/perry-runtime/src/gc/scan_fallback.rs": 1, - "crates/perry-runtime/src/gc/shape_install.rs": 2, - "crates/perry-runtime/src/gc/telemetry.rs": 3, + "crates/perry-runtime/src/gc/shape_install.rs": 1, + "crates/perry-runtime/src/gc/telemetry.rs": 2, "crates/perry-runtime/src/gc/tenuring.rs": 1, - "crates/perry-runtime/src/gc/tests/copying.rs": 1, - "crates/perry-runtime/src/gc/tests/roots.rs": 1, - "crates/perry-runtime/src/gc/tests/runtime_roots.rs": 4, - "crates/perry-runtime/src/gc/tests/runtime_roots/transient_handles.rs": 1, - "crates/perry-runtime/src/gc/tests/support.rs": 1, "crates/perry-runtime/src/gc/trace.rs": 2, - "crates/perry-runtime/src/gc/zeal.rs": 3, "crates/perry-runtime/src/intl/number_format.rs": 1, "crates/perry-runtime/src/iter_result.rs": 1, "crates/perry-runtime/src/json/mod.rs": 1, "crates/perry-runtime/src/json/raw_json.rs": 1, - "crates/perry-runtime/src/json_tape.rs": 3, + "crates/perry-runtime/src/json_tape.rs": 2, "crates/perry-runtime/src/json_tape_store.rs": 1, "crates/perry-runtime/src/media_playback.rs": 1, "crates/perry-runtime/src/native_arena.rs": 1, @@ -78,13 +70,11 @@ "crates/perry-runtime/src/node_submodules/test.rs": 1, "crates/perry-runtime/src/node_submodules/test_once_unit_tests.rs": 1, "crates/perry-runtime/src/node_submodules/test_property.rs": 1, - "crates/perry-runtime/src/node_submodules/tests.rs": 1, "crates/perry-runtime/src/node_submodules/trace_events.rs": 1, "crates/perry-runtime/src/object/native_module/callable_exports.rs": 2, - "crates/perry-runtime/src/object/spill.rs": 2, + "crates/perry-runtime/src/object/spill.rs": 1, "crates/perry-runtime/src/os/os_process_emitter.rs": 1, "crates/perry-runtime/src/os_process_streams.rs": 1, - "crates/perry-runtime/src/per_test_global.rs": 1, "crates/perry-runtime/src/perf_hooks.rs": 3, "crates/perry-runtime/src/process.rs": 2, "crates/perry-runtime/src/process/env_misc.rs": 3,