From 871aa3e4437140b67a1576f28be6254161df2161 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 12 Aug 2026 22:47:52 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(gc):=20parse=20PERRY=5FGC=5FDIAG=20by?= =?UTF-8?q?=20value=20=E2=80=94=20`=3D0`=20no=20longer=20ENABLES=20diagnos?= =?UTF-8?q?tics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gc_diag_enabled()` read the knob with `var_os(..).is_some()` — presence, not value — so `PERRY_GC_DIAG=0` turned diagnostics ON, and so did `off`, `false` and the empty string. That is a measurement-integrity bug: during #7803 triage it silently collapsed an A/B arm, because the "diagnostics off" control arm was instrumented exactly like the arm it controlled for. It fails toward a confident wrong answer, not a visible error. The audit that came with it found the same shape on 25 read sites across four GC knobs (DIAG x20, VERIFY_MARK x3, VERIFY_RS_NONFATAL x1, and VERIFY_EVACUATION, which was *split-brain*: value-parsed in gc/mod.rs and presence-parsed in the barrier's ever-dirty tracker, so `=0` switched the verifier off while leaving its side table populated on every barrier). There are now exactly two boolean vocabularies, both pure functions of the raw value so both directions are testable without touching the process environment: * `gc::env_flag_from_value` — default-OFF (#5093): `1`/`true`/`on`/`yes`; unset, the off-spellings, empty and anything UNRECOGNISED read OFF. * `gc::env_default_on_from_value` — default-ON kill switch: OFF only on an explicit `0`/`off`/`false`/`no`; unrecognised leaves the shipping default ON. They are deliberately not each other's negation — each fails toward its own documented default — and that asymmetry has its own assertion so a future tidy-up cannot collapse one into the other. Also unified onto them: GC_TRACE, GC_VERIFY_CLASSIFIER, GC_FORCE_EVACUATE, GEN_GC, WRITE_BARRIERS, GC_MOVING_SAFEPOINT, GC_INCREMENTAL, and GC_SAFEPOINT_ONLY's boolean arm. `PERRY_SHAPE_LAYOUT_KEYED` was `v != "0"`, so its documented off-state worked only for the literal `0` — `=off` and `=false` read as ON. Teeth, so this class cannot come back: * `gc/tests/env_knob_parse.rs` — both vocabularies over on/off/unrecognised spellings, plus the decisive arm: `gc_diag_enabled()` observed in a CHILD PROCESS under a real `PERRY_GC_DIAG=0`/`off`/``/`1`. The pure cases alone would stay green if that one line reverted to presence-parsing; only the live cached reader under a real environment closes it, and the ON arm is there so a fix that hard-wires `false` cannot pass either. * `scripts/check_gc_env_knobs.py` (already in `lint`) now rejects the presence-only shape outright for the GC family. The exemption list is empty and stale entries fail, so a fix must delete its own licence. Its `--self-test` sabotages the detector with the exact shape that shipped and requires it to be distinguished from the value-parsed replacement. Closes #7991. --- crates/perry-runtime/src/arena/quarantine.rs | 2 +- crates/perry-runtime/src/arena/reset.rs | 2 +- crates/perry-runtime/src/gc/barrier/mod.rs | 13 +- crates/perry-runtime/src/gc/copying.rs | 10 +- crates/perry-runtime/src/gc/cycle.rs | 2 +- crates/perry-runtime/src/gc/layout.rs | 6 +- crates/perry-runtime/src/gc/mod.rs | 72 ++++-- crates/perry-runtime/src/gc/oldgen.rs | 10 +- crates/perry-runtime/src/gc/policy.rs | 49 ++-- crates/perry-runtime/src/gc/scan_fallback.rs | 4 +- .../perry-runtime/src/gc/scanner_profile.rs | 2 +- crates/perry-runtime/src/gc/telemetry.rs | 21 +- crates/perry-runtime/src/gc/tenuring.rs | 8 +- .../src/gc/tests/env_knob_parse.rs | 237 ++++++++++++++++++ crates/perry-runtime/src/gc/tests/mod.rs | 1 + crates/perry-runtime/src/gc/tests/schedule.rs | 5 +- crates/perry-runtime/src/gc/trace.rs | 7 +- crates/perry-runtime/src/gc/verify.rs | 2 +- scripts/check_gc_env_knobs.py | 114 ++++++++- 19 files changed, 473 insertions(+), 94 deletions(-) create mode 100644 crates/perry-runtime/src/gc/tests/env_knob_parse.rs diff --git a/crates/perry-runtime/src/arena/quarantine.rs b/crates/perry-runtime/src/arena/quarantine.rs index f051308d91..dc92c0f091 100644 --- a/crates/perry-runtime/src/arena/quarantine.rs +++ b/crates/perry-runtime/src/arena/quarantine.rs @@ -532,7 +532,7 @@ pub(crate) fn copying_quarantine_from_spaces_and_flip() -> ArenaResetStats { let recycled = push_set_and_evict(retired); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { let stats = quarantine_stats(); eprintln!( "[gc-fromspace-protect] mode={:?} retired_set=#{} blocks={} sets_held={}/{} bytes_protected={} bytes_poisoned={} blocks_recycled={}", diff --git a/crates/perry-runtime/src/arena/reset.rs b/crates/perry-runtime/src/arena/reset.rs index ac61f63b81..92e6cd5551 100644 --- a/crates/perry-runtime/src/arena/reset.rs +++ b/crates/perry-runtime/src/arena/reset.rs @@ -363,7 +363,7 @@ pub fn arena_reset_empty_blocks(block_has_live: &[bool]) -> ArenaResetStats { crate::gc::ARENA_FREE_LIST_NONEMPTY.with(|c| c.set(false)); } }); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-block-release] removed {} blocks ({} bytes): pooled={} bytes, deallocated={} bytes", stats.removed_blocks, diff --git a/crates/perry-runtime/src/gc/barrier/mod.rs b/crates/perry-runtime/src/gc/barrier/mod.rs index fac1ccae41..7a01820065 100644 --- a/crates/perry-runtime/src/gc/barrier/mod.rs +++ b/crates/perry-runtime/src/gc/barrier/mod.rs @@ -1049,12 +1049,7 @@ pub(super) fn generated_write_barriers_emitted() -> bool { pub(crate) fn write_barriers_enabled() -> bool { use std::sync::OnceLock; static CACHED: OnceLock = OnceLock::new(); - *CACHED.get_or_init(|| { - !matches!( - std::env::var("PERRY_WRITE_BARRIERS").as_deref(), - Ok("0") | Ok("off") | Ok("false") - ) - }) + *CACHED.get_or_init(|| super::env_default_on_enabled("PERRY_WRITE_BARRIERS")) } #[inline] @@ -1739,7 +1734,11 @@ fn ever_dirty_tracking_enabled() -> bool { use std::sync::OnceLock; static CACHED: OnceLock = OnceLock::new(); *CACHED.get_or_init(|| { - std::env::var_os("PERRY_GC_VERIFY_EVACUATION").is_some() + // #7991: value-parsed, not presence-parsed. This site read the same + // knob as `gc::gc_verify_evacuation_enabled()` but with the opposite + // convention, so `PERRY_GC_VERIFY_EVACUATION=0` switched the verifier + // off while leaving its side table being populated on every barrier. + super::env_flag_enabled("PERRY_GC_VERIFY_EVACUATION") || super::fromspace_scan::fromspace_scan_enabled() }) } diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index b9a708ac52..0d1d0df5cf 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -710,7 +710,7 @@ impl CopyingNurseryCollector { // refuses to move would silently stay in from-space across a copying // minor. `pointer_bearing_large_object_threshold_is_movable` pins that. if total < GC_HEADER_SIZE || total > MAX_YOUNG_MOVE_BYTES { - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-move-guard] refusing wild young move user={:#x} obj_type={} size={}", old_user as usize, @@ -932,7 +932,7 @@ fn untraced_promotion_instrument_veto() -> Option<&'static str> { if super::fromspace_scan::fromspace_scan_enabled() { return Some("fromspace_scan"); } - if std::env::var_os("PERRY_GC_VERIFY_MARK").is_some() { + if crate::gc::gc_verify_mark_enabled() { return Some("verify_mark"); } if super::barrier::incremental_mark_in_progress_on_this_thread() { @@ -1326,7 +1326,7 @@ pub(super) fn run_copied_minor_attempt( trace.root_sources.native_stack_fallback.scanned = matches!(decision, ConservativeStackScanDecision::Scan); } - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { let reason = match eligibility.fallback_reason { CopiedMinorFallbackReason::None => "none", CopiedMinorFallbackReason::NotAttempted => "not_attempted", @@ -1690,7 +1690,7 @@ pub(super) fn run_copied_minor_attempt( // young objects, check that no MARKED (survived) object references an // UNMARKED (about-to-be-freed) child — i.e. a live parent whose child is // being swept. Non-fatal; logs parent/child obj_types. - if std::env::var_os("PERRY_GC_VERIFY_MARK").is_some() { + if crate::gc::gc_verify_mark_enabled() { super::verify::verify_marked_heap_report_nonfatal("copying-minor"); } @@ -1923,7 +1923,7 @@ pub(super) fn run_copied_minor_attempt( collector.stats.copied_bytes, collector.stats.survivor_live_bytes, ); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-copy-minor] ran in_place={} untraced={} untraced_cycles={} untraced_objects={} in_place_blocks={} in_place_dead_bytes={} sparse_blocks={} survival_permille={} copied_objects={} copied_bytes={} promoted_objects={} promoted_bytes={} freed_bytes={} tenuring_survivals={} eden_live_bytes={} trigger={:?} declared_safepoint={}", collector.stats.in_place_promotion, diff --git a/crates/perry-runtime/src/gc/cycle.rs b/crates/perry-runtime/src/gc/cycle.rs index 4bc22252fa..582e35150f 100644 --- a/crates/perry-runtime/src/gc/cycle.rs +++ b/crates/perry-runtime/src/gc/cycle.rs @@ -1583,7 +1583,7 @@ impl GcCycleState { // Diagnostic (PERRY_GC_VERIFY_MARK): marks are final for this minor and // sweep has not yet run — report any OLD parent whose young/malloc child // is UNMARKED (about to be swept live = dropped remembered-set edge). - if std::env::var_os("PERRY_GC_VERIFY_MARK").is_some() { + if crate::gc::gc_verify_mark_enabled() { super::verify::verify_minor_unmarked_young_children_report("minor-prelude"); } diff --git a/crates/perry-runtime/src/gc/layout.rs b/crates/perry-runtime/src/gc/layout.rs index 99582b8ca9..4a094e9f7d 100644 --- a/crates/perry-runtime/src/gc/layout.rs +++ b/crates/perry-runtime/src/gc/layout.rs @@ -159,11 +159,7 @@ fn shape_layout_keyed_enabled() -> bool { static E: OnceLock = OnceLock::new(); // Default ON; `PERRY_SHAPE_LAYOUT_KEYED=0` restores the per-object maps // (A/B validation). - *E.get_or_init(|| { - std::env::var("PERRY_SHAPE_LAYOUT_KEYED") - .map(|v| v != "0") - .unwrap_or(true) - }) + *E.get_or_init(|| super::env_default_on_enabled("PERRY_SHAPE_LAYOUT_KEYED")) } /// keys_array only exists on genuine shaped objects (`ObjectFields`). Arrays, diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 028f25e5af..b54d5be003 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -378,10 +378,7 @@ pub fn gen_gc_enabled() -> bool { if !write_barriers_enabled() { return false; } - !matches!( - std::env::var("PERRY_GEN_GC").as_deref(), - Ok("0") | Ok("off") | Ok("false") - ) + env_default_on_enabled("PERRY_GEN_GC") }) } @@ -437,11 +434,7 @@ fn gc_force_evacuate_enabled() -> bool { // the mode — without this it would be a knob whose name promises relocation // stress and whose effect is sweep pressure. Unconditional, per #7611's // deletion note above. - schedule::gc_schedule_enabled() - || matches!( - std::env::var("PERRY_GC_FORCE_EVACUATE").as_deref(), - Ok("1") | Ok("on") | Ok("true") - ) + schedule::gc_schedule_enabled() || env_flag_enabled("PERRY_GC_FORCE_EVACUATE") } fn gc_verify_evacuation_enabled() -> bool { @@ -450,10 +443,7 @@ fn gc_verify_evacuation_enabled() -> bool { { return forced; } - matches!( - std::env::var("PERRY_GC_VERIFY_EVACUATION").as_deref(), - Ok("1") | Ok("on") | Ok("true") - ) + env_flag_enabled("PERRY_GC_VERIFY_EVACUATION") } /// Per-thread test overrides for the two collector knobs the unit suite needs @@ -1271,19 +1261,61 @@ fn emit_schedule_liveness_verdict() { } } -/// #5093: parse a boolean-ish env var by value (not mere presence): true for -/// `1`/`true`/`on`/`yes` (case-insensitive), false for unset / `0`/`false`/`off` -/// / `no` / empty / anything else. -fn env_flag_enabled(name: &str) -> bool { - match std::env::var(name) { - Ok(v) => matches!( +/// #5093 semantics as a **pure** function of the raw value, so both directions +/// can be pinned by a test without touching the process environment (the live +/// readers cache in a `OnceLock`; a test that called `set_var` would be at the +/// mercy of which test ran first, and `set_var` is process-wide — see the +/// `knob_overrides` note above for what that cost us once already). +/// +/// True for `1`/`true`/`on`/`yes` (case-insensitive, surrounding whitespace +/// ignored). False for unset, `0`/`false`/`off`/`no`, the empty string, **and +/// anything unrecognised** — a typo must not silently arm an instrument. +/// +/// #7991: this is the single definition of "boolean-ish GC knob". Every GC knob +/// that is a boolean must route through it. `scripts/check_gc_env_knobs.py` +/// enforces that by rejecting presence-only reads (`var_os(..).is_some()`) of +/// GC-family names in production code. +pub(crate) fn env_flag_from_value(raw: Option<&str>) -> bool { + match raw { + Some(v) => matches!( v.trim().to_ascii_lowercase().as_str(), "1" | "true" | "on" | "yes" ), - Err(_) => false, + None => false, } } +/// #5093: parse a boolean-ish env var by value (not mere presence). +/// See [`env_flag_from_value`] for the exact contract. +pub(crate) fn env_flag_enabled(name: &str) -> bool { + env_flag_from_value(std::env::var(name).ok().as_deref()) +} + +/// The mirror of [`env_flag_from_value`] for a **default-ON kill switch**: +/// the feature is ON for unset, for the empty string, and for anything +/// unrecognised; OFF only for an explicit `0`/`off`/`false`/`no` +/// (case-insensitive, surrounding whitespace ignored). +/// +/// This is deliberately **not** `!env_flag_from_value(..)`. Both helpers fail +/// toward their knob's documented default, which is the opposite direction in +/// each case: a typo must neither arm an instrument that is off by default nor +/// disable a collector feature that ships on. +pub(crate) fn env_default_on_from_value(raw: Option<&str>) -> bool { + match raw { + Some(v) => !matches!( + v.trim().to_ascii_lowercase().as_str(), + "0" | "off" | "false" | "no" + ), + None => true, + } +} + +/// Read a default-ON kill switch from the environment. +/// See [`env_default_on_from_value`] for the exact contract. +pub(crate) fn env_default_on_enabled(name: &str) -> bool { + env_default_on_from_value(std::env::var(name).ok().as_deref()) +} + /// FFI: get GC stats #[no_mangle] pub extern "C" fn js_gc_stats( diff --git a/crates/perry-runtime/src/gc/oldgen.rs b/crates/perry-runtime/src/gc/oldgen.rs index 40c3d1ca6b..af77f511aa 100644 --- a/crates/perry-runtime/src/gc/oldgen.rs +++ b/crates/perry-runtime/src/gc/oldgen.rs @@ -425,7 +425,7 @@ pub(super) fn maybe_print_evacuation_policy_diag( decision: EvacuationPolicyDecision, evacuation: EvacuationTraceStats, ) { - if std::env::var_os("PERRY_GC_DIAG").is_none() { + if !crate::gc::gc_diag_enabled() { return; } if !decision.considered && decision.reason != "barriers_inactive" { @@ -992,7 +992,7 @@ fn legacy_sweep_with_age_bump_and_old_reclaim_targets( // Reset every block that ended up with zero live objects. // Diagnostic: PERRY_GC_DIAG=1 reports block-level liveness. - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { let live_general = (0..resettable_general_n) .filter(|&i| block_has_live[i]) .count(); @@ -1029,7 +1029,7 @@ fn legacy_sweep_with_age_bump_and_old_reclaim_targets( // better than hole-by-hole reuse. if reclaim_dead_old_blocks { old_free_rebuild_from_live_old_blocks(&block_has_live, old_block_start); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!("[gc-old-free] reusable_bytes={}", old_free_bytes()); } } @@ -1386,7 +1386,7 @@ impl ArenaSweepObjectsState { &self.block_has_live, self.old_block_start, ); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!("[gc-old-free] reusable_bytes={}", super::old_free_bytes()); } } @@ -1413,7 +1413,7 @@ impl ArenaSweepObjectsState { } fn maybe_print_diag(&self) { - if std::env::var_os("PERRY_GC_DIAG").is_none() { + if !crate::gc::gc_diag_enabled() { return; } let live_general = (0..self.resettable_general_n) diff --git a/crates/perry-runtime/src/gc/policy.rs b/crates/perry-runtime/src/gc/policy.rs index 62a57d2ed3..c3456bef2b 100644 --- a/crates/perry-runtime/src/gc/policy.rs +++ b/crates/perry-runtime/src/gc/policy.rs @@ -475,12 +475,7 @@ pub(crate) fn gc_note_external_side_free(bytes: usize) { pub(crate) fn gc_moving_safepoint_enabled() -> bool { static CACHED: OnceLock = OnceLock::new(); // Default ON; the kill switch is an explicit `=0`/`off`/`false`. - *CACHED.get_or_init(|| { - !matches!( - std::env::var("PERRY_GC_MOVING_SAFEPOINT").as_deref(), - Ok("0") | Ok("off") | Ok("false") - ) - }) + *CACHED.get_or_init(|| super::env_default_on_enabled("PERRY_GC_MOVING_SAFEPOINT")) } /// Phase 4 of the moving-GC project: gate the INCREMENTAL old-gen collector (the @@ -515,10 +510,7 @@ pub(crate) fn gc_incremental_enabled() -> bool { // default; the synchronous collector remains for manual gc(), // emergency reclaim, and as the PERRY_GC_INCREMENTAL=0 escape hatch // (bisection / max-throughput batch workloads). - !matches!( - std::env::var("PERRY_GC_INCREMENTAL").as_deref(), - Ok("0") | Ok("off") | Ok("false") - ) + super::env_default_on_enabled("PERRY_GC_INCREMENTAL") }) } @@ -744,12 +736,7 @@ pub(super) fn gc_trace_enabled() -> bool { } static CACHED: OnceLock = OnceLock::new(); - *CACHED.get_or_init(|| { - matches!( - std::env::var("PERRY_GC_TRACE").as_deref(), - Ok("1") | Ok("on") | Ok("true") - ) - }) + *CACHED.get_or_init(|| super::env_flag_enabled("PERRY_GC_TRACE")) } #[cfg(test)] @@ -1058,13 +1045,25 @@ pub(super) enum SafepointOnlyContract { pub(super) fn gc_safepoint_only_contract() -> SafepointOnlyContract { use std::sync::OnceLock; static CACHED: OnceLock = OnceLock::new(); - *CACHED.get_or_init( - || match std::env::var("PERRY_GC_SAFEPOINT_ONLY").as_deref() { - Ok("1") | Ok("on") | Ok("true") => SafepointOnlyContract::Heal, - Ok("strict") => SafepointOnlyContract::Strict, - _ => SafepointOnlyContract::Off, - }, - ) + *CACHED.get_or_init(|| { + safepoint_only_contract_from_value(std::env::var("PERRY_GC_SAFEPOINT_ONLY").ok().as_deref()) + }) +} + +/// Pure value→contract mapping (#7991), so both directions are testable without +/// touching the process environment. The boolean arm shares the one GC +/// boolean-ish vocabulary; `strict` is this knob's own third state. +pub(super) fn safepoint_only_contract_from_value(raw: Option<&str>) -> SafepointOnlyContract { + if matches!( + raw.map(|v| v.trim().to_ascii_lowercase()).as_deref(), + Some("strict") + ) { + return SafepointOnlyContract::Strict; + } + if super::env_flag_from_value(raw) { + return SafepointOnlyContract::Heal; + } + SafepointOnlyContract::Off } /// Contract enforcement chokepoint, called once at every synchronous @@ -2152,7 +2151,7 @@ fn gc_finish_arena_trigger_collection(pre_in_use: usize, outcome: GcCollectOutco } // 10-25% freed → keep step unchanged (marginal churn). GC_STEP_BYTES.with(|c| c.set(step)); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-step] pre_in_use={} post_in_use={} sweep_freed={} block_reclaim={} pct={}% step={}→{}", pre_in_use, post_in_use, sweep_freed_bytes, block_reclaim, pct_freed, old_step, step @@ -3362,7 +3361,7 @@ pub(super) fn gc_drain_active_budgeted_cycle() { break; } } - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!("[gc-drain] WARNING: parked budgeted cycle could not be drained before synchronous collection"); } } diff --git a/crates/perry-runtime/src/gc/scan_fallback.rs b/crates/perry-runtime/src/gc/scan_fallback.rs index 43b54ec9af..c3915ca637 100644 --- a/crates/perry-runtime/src/gc/scan_fallback.rs +++ b/crates/perry-runtime/src/gc/scan_fallback.rs @@ -213,7 +213,7 @@ pub(crate) fn record_scan_fallback(site: ConservativeScanSite) { c.set(counts); counts[site.index()] }); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-scan-fallback] site={} automatic={} count={}", site.as_str(), @@ -233,7 +233,7 @@ pub(crate) fn record_safepoint_drain(kind: SafepointDrainKind) { c.set(counts); counts[kind.index()] }); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-safepoint-drain] kind={} count={}", kind.as_str(), diff --git a/crates/perry-runtime/src/gc/scanner_profile.rs b/crates/perry-runtime/src/gc/scanner_profile.rs index 7144e877b4..954767b3f9 100644 --- a/crates/perry-runtime/src/gc/scanner_profile.rs +++ b/crates/perry-runtime/src/gc/scanner_profile.rs @@ -37,7 +37,7 @@ pub(super) fn scanner_profile_enabled() -> bool { if cached != 0 { return cached == 2; } - let on = std::env::var_os("PERRY_GC_DIAG").is_some(); + let on = crate::gc::gc_diag_enabled(); cell.set(if on { 2 } else { 1 }); on }) diff --git a/crates/perry-runtime/src/gc/telemetry.rs b/crates/perry-runtime/src/gc/telemetry.rs index e8e5eda3d6..ce610ce5da 100644 --- a/crates/perry-runtime/src/gc/telemetry.rs +++ b/crates/perry-runtime/src/gc/telemetry.rs @@ -3,12 +3,29 @@ use super::*; /// Number of most-recent pause samples retained per thread (#6187). pub const GC_RECENT_PAUSE_WINDOW: usize = 32; -/// Is `PERRY_GC_DIAG` set? Read once and cached, so a diagnostic call site can +/// Is `PERRY_GC_DIAG` ON? Read once and cached, so a diagnostic call site can /// sit on a path that runs before/around `main` without paying a `getenv` each /// time. Diagnostic-only: nothing may branch on this for behaviour. +/// +/// #7991: this used to be `var_os(..).is_some()` — *presence*, not value — so +/// `PERRY_GC_DIAG=0` turned diagnostics ON. That is not cosmetic: it silently +/// collapsed an A/B arm during #7803 triage, because the investigator's "clean" +/// control arm got the same diagnostics as the instrumented one. A knob that +/// fails toward a confident wrong answer is worse than one that fails loudly. +/// The value semantics are #5093's, shared with every other GC knob via +/// [`super::env_flag_from_value`]. pub fn gc_diag_enabled() -> bool { static ENABLED: std::sync::OnceLock = std::sync::OnceLock::new(); - *ENABLED.get_or_init(|| std::env::var_os("PERRY_GC_DIAG").is_some()) + *ENABLED.get_or_init(|| env_flag_enabled("PERRY_GC_DIAG")) +} + +/// Is `PERRY_GC_VERIFY_MARK` ON? Cached for the same reason as +/// [`gc_diag_enabled`], and value-parsed for the same reason (#7991): the three +/// mark-verifier call sites were presence-only, so `=0` armed a verifier that +/// walks the whole heap. +pub(crate) fn gc_verify_mark_enabled() -> bool { + static ENABLED: std::sync::OnceLock = std::sync::OnceLock::new(); + *ENABLED.get_or_init(|| env_flag_enabled("PERRY_GC_VERIFY_MARK")) } pub struct GcStats { diff --git a/crates/perry-runtime/src/gc/tenuring.rs b/crates/perry-runtime/src/gc/tenuring.rs index b1965709d5..b9435641d9 100644 --- a/crates/perry-runtime/src/gc/tenuring.rs +++ b/crates/perry-runtime/src/gc/tenuring.rs @@ -307,7 +307,7 @@ pub(super) fn note_surviving_object_census(moved_bytes: usize, moved_objects: us return; } let previous = MEAN_SURVIVING_OBJECT_BYTES.replace(mean); - if previous != mean && std::env::var_os("PERRY_GC_DIAG").is_some() { + if previous != mean && crate::gc::gc_diag_enabled() { eprintln!( "[gc-tenuring] nursery cap object denomination: mean_surviving_object_bytes {} -> {} \ (scale {} permille, band {} B)", @@ -523,7 +523,7 @@ pub(super) fn seed_promote_lock_from_sweep(eden_live_bytes: usize, eden_dead_byt // mark-sweep, including refusals. A policy that silently declines is // indistinguishable from one that never ran (#7024/#7025), and the // refusal reason is the number a future tuning decision needs. - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { let classified = eden_live_bytes.saturating_add(eden_dead_bytes); let pct = if classified == 0 { 0 @@ -547,7 +547,7 @@ pub(super) fn seed_promote_lock_from_sweep(eden_live_bytes: usize, eden_dead_byt } fn diag_cap_scale(from: u8, to: u8, eden_live_bytes: usize) { - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-tenuring] nursery cap scale {from}x -> {to}x (eden_live_bytes={eden_live_bytes})" ); @@ -559,7 +559,7 @@ fn set_survivals(current: u8, next: u8, eden_live_bytes: usize, why: &str) { return; } TENURING_SURVIVALS.with(|s| s.set(next)); - if std::env::var_os("PERRY_GC_DIAG").is_some() { + if crate::gc::gc_diag_enabled() { eprintln!( "[gc-tenuring] survivals {} -> {} ({why}, eden_live_bytes={} desired={})", current, diff --git a/crates/perry-runtime/src/gc/tests/env_knob_parse.rs b/crates/perry-runtime/src/gc/tests/env_knob_parse.rs new file mode 100644 index 0000000000..7f24d9ebcd --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/env_knob_parse.rs @@ -0,0 +1,237 @@ +//! The GC environment-knob parse contract, pinned in **both** directions +//! (#7991). +//! +//! The bug this closes: `gc_diag_enabled()` read `PERRY_GC_DIAG` with +//! `var_os(..).is_some()` — *presence*, not value — so `PERRY_GC_DIAG=0` +//! turned diagnostics ON, as did `off`, `false` and the empty string. That is +//! a measurement-integrity bug, not a cosmetic one: during #7803 triage it +//! silently collapsed an A/B arm, because the investigator's "diagnostics off" +//! control arm got exactly the same diagnostics as the instrumented arm. The +//! failure direction is the dangerous one — a confident wrong answer rather +//! than a visible error. +//! +//! It was also *inconsistent with its immediate neighbours*: +//! `PERRY_GC_PROTECT_FROMSPACE` two modules over has parsed its value properly +//! all along, so `=0` really was off there. Two adjacent GC knobs with +//! opposite conventions is the drift CLAUDE.md's knob kill-policy exists to +//! prevent, so this file asserts the *shared vocabulary* rather than one +//! knob's behaviour — a future knob that hand-rolls its own `matches!` is the +//! regression, and `scripts/check_gc_env_knobs.py` rejects the presence-only +//! shape outright. +//! +//! Everything here is a pure function of the raw value. That is deliberate: +//! the live readers cache in a `OnceLock`, and `std::env::set_var` is +//! process-wide, so a test that set the real variable would be at the mercy of +//! which libtest thread ran first (`knob_overrides` in `gc/mod.rs` records +//! what that cost us — 5 failures in 100 runs across three unrelated cases). + +use super::super::policy::{safepoint_only_contract_from_value, SafepointOnlyContract}; +use super::super::{env_default_on_from_value, env_flag_from_value}; + +/// Every spelling a human might reasonably use to mean "off", plus the two +/// that actually bit: `Some("0")` and `Some("")`. +const OFF_SPELLINGS: &[Option<&str>] = &[ + None, + Some("0"), + Some("off"), + Some("false"), + Some("no"), + Some(""), + Some(" "), + Some("OFF"), + Some("False"), + Some(" 0 "), +]; + +const ON_SPELLINGS: &[&str] = &["1", "true", "on", "yes", "TRUE", "On", " 1 ", "YES"]; + +/// The unrecognised case gets its own name because it is the one an +/// unthinking `!is_off()` implementation gets wrong: a typo must leave a +/// default-OFF instrument OFF, not arm it. +const UNRECOGNISED: &[&str] = &["banana", "2", "-1", "onn", "ye", "enabled", "0x1"]; + +#[test] +fn default_off_knobs_are_parsed_by_value_not_presence() { + for raw in OFF_SPELLINGS { + assert!( + !env_flag_from_value(*raw), + "{raw:?} must read as OFF — presence is not consent. `PERRY_GC_DIAG=0` \ + enabling diagnostics is exactly how #7803's control arm was lost." + ); + } + for raw in ON_SPELLINGS { + assert!(env_flag_from_value(Some(raw)), "{raw:?} must read as ON"); + } + for raw in UNRECOGNISED { + assert!( + !env_flag_from_value(Some(raw)), + "{raw:?} is unrecognised and must leave a default-OFF knob OFF, not \ + silently arm it" + ); + } +} + +/// The mirror contract. A default-ON kill switch must fail toward its own +/// default, so it is **not** the negation of the default-OFF parser: an +/// unrecognised value leaves the shipping feature ON. +#[test] +fn default_on_kill_switches_only_fire_on_an_explicit_off() { + assert!( + env_default_on_from_value(None), + "unset must leave a default-ON feature ON" + ); + for raw in ["0", "off", "false", "no", "OFF", "False", " 0 "] { + assert!( + !env_default_on_from_value(Some(raw)), + "{raw:?} must disable a default-ON kill switch" + ); + } + for raw in ["1", "true", "on", "yes", ""] { + assert!( + env_default_on_from_value(Some(raw)), + "{raw:?} must leave a default-ON feature ON" + ); + } + for raw in UNRECOGNISED { + assert!( + env_default_on_from_value(Some(raw)), + "{raw:?} is unrecognised; a typo must not silently disable a shipping \ + collector default" + ); + } +} + +/// The two parsers are not each other's negation, and that asymmetry is the +/// point rather than an oversight — so it gets an assertion of its own, or a +/// future tidy-up will "simplify" one into the other. +#[test] +fn the_two_vocabularies_disagree_only_on_the_unrecognised_case() { + for raw in UNRECOGNISED { + assert!(!env_flag_from_value(Some(raw))); + assert!(env_default_on_from_value(Some(raw))); + } + // ...and agree everywhere a value is recognised. + for raw in ["1", "true", "on", "yes"] { + assert!(env_flag_from_value(Some(raw)) && env_default_on_from_value(Some(raw))); + } + for raw in ["0", "off", "false", "no"] { + assert!(!env_flag_from_value(Some(raw)) && !env_default_on_from_value(Some(raw))); + } +} + +/// `PERRY_GC_SAFEPOINT_ONLY` is three-state. Its boolean arm must share the one +/// vocabulary; only `strict` is its own. +#[test] +fn safepoint_only_is_three_state_over_the_shared_vocabulary() { + for raw in OFF_SPELLINGS { + assert_eq!( + safepoint_only_contract_from_value(*raw), + SafepointOnlyContract::Off, + "{raw:?} must leave the safepoint-only contract Off" + ); + } + for raw in ON_SPELLINGS { + assert_eq!( + safepoint_only_contract_from_value(Some(raw)), + SafepointOnlyContract::Heal, + "{raw:?} must select Heal" + ); + } + for raw in ["strict", "STRICT", " strict "] { + assert_eq!( + safepoint_only_contract_from_value(Some(raw)), + SafepointOnlyContract::Strict, + "{raw:?} must select Strict" + ); + } + for raw in UNRECOGNISED { + assert_eq!( + safepoint_only_contract_from_value(Some(raw)), + SafepointOnlyContract::Off, + "{raw:?} is unrecognised and must not arm a contract enforcer" + ); + } +} + +/// The decisive arm: the **live cached reader**, initialised in a child +/// process under a real `PERRY_GC_DIAG=0`. +/// +/// The pure cases above pin the vocabulary; they do not by themselves prove +/// `gc_diag_enabled()` uses it — reverting that one line to +/// `var_os(..).is_some()` leaves every one of them green. Nothing short of +/// observing the shipping reader under the shipping environment closes that, +/// and it cannot be done in-process: the reader caches in a `OnceLock` and +/// `set_var` is visible to every other libtest thread, so an in-process ON arm +/// would be both racy and order-dependent. A child process is the isolation. +/// +/// The child re-runs *this* test with the marker set, so the assertion and the +/// environment that produces it stay in one place. +#[test] +fn perry_gc_diag_zero_really_disables_diagnostics() { + // Deliberately NOT a `PERRY_GC_*` name: this is test-harness plumbing, and + // the GC knob family is audited (`scripts/check_gc_env_knobs.py`). + const CHILD_ENV: &str = "PERRY_TEST_GC_DIAG_PARSE_CHILD"; + if let Some(expected) = std::env::var_os(CHILD_ENV) { + let want = expected == *"on"; + assert_eq!( + super::super::gc_diag_enabled(), + want, + "PERRY_GC_DIAG={:?} must read as {}", + std::env::var_os("PERRY_GC_DIAG"), + if want { "ON" } else { "OFF" } + ); + return; + } + + // (raw value, expected verdict). `0` is the case that shipped broken; the + // ON arm is here so a fix that hard-wires `false` cannot pass either — a + // liveness counter satisfiable by two paths is a presence check, not a + // proof. + for (raw, want_on) in [("0", false), ("off", false), ("", false), ("1", true)] { + let status = + std::process::Command::new(std::env::current_exe().expect("current test binary")) + .arg("gc::tests::env_knob_parse::perry_gc_diag_zero_really_disables_diagnostics") + .arg("--exact") + .arg("--nocapture") + .env("PERRY_GC_DIAG", raw) + .env(CHILD_ENV, if want_on { "on" } else { "off" }) + .status() + .expect("launch isolated PERRY_GC_DIAG witness"); + assert!( + status.success(), + "PERRY_GC_DIAG={raw:?} did not read as {}; presence is not consent", + if want_on { "ON" } else { "OFF" } + ); + } +} + +/// The remaining boolean GC knobs, asserted OFF when unset — the state every CI +/// job and every developer shell is in, and what makes an un-instrumented run +/// trustworthy. +#[test] +fn the_live_readers_are_off_when_their_knobs_are_unset() { + // If any of these were still presence-parsed they would still be off here + // (the vars are unset), so this is a *default* assertion, not a proof of + // the parse — that is what the pure cases above are for. It is worth + // having anyway: the default-off contract for a diagnostic knob is what + // makes an un-instrumented run trustworthy. + for name in [ + "PERRY_GC_DIAG", + "PERRY_GC_TRACE", + "PERRY_GC_VERIFY_MARK", + "PERRY_GC_VERIFY_EVACUATION", + "PERRY_GC_VERIFY_RS_NONFATAL", + "PERRY_GC_VERIFY_CLASSIFIER", + "PERRY_GC_FORCE_EVACUATE", + ] { + if std::env::var_os(name).is_some() { + // An operator running the suite under a knob is not a test + // failure; skip rather than assert something untrue. + continue; + } + assert!( + !super::super::env_flag_enabled(name), + "{name} is unset and must read as OFF" + ); + } +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 56c254c0a6..43041e9d95 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -14,6 +14,7 @@ mod cycle_state; mod dead_owner_side_tables; mod debt_pacer; mod dirty_page_cache; +mod env_knob_parse; mod error_side_tables; mod evacuation; mod fromspace_protect; diff --git a/crates/perry-runtime/src/gc/tests/schedule.rs b/crates/perry-runtime/src/gc/tests/schedule.rs index 3ae837f780..9ebcdf2bab 100644 --- a/crates/perry-runtime/src/gc/tests/schedule.rs +++ b/crates/perry-runtime/src/gc/tests/schedule.rs @@ -320,10 +320,7 @@ fn the_schedule_implies_forced_evacuation() { #[test] fn unset_is_inert_for_evacuation_policy() { let _off = ScheduleGuard::off(); - let baseline = matches!( - std::env::var("PERRY_GC_FORCE_EVACUATE").as_deref(), - Ok("1") | Ok("on") | Ok("true") - ); + let baseline = super::super::env_flag_enabled("PERRY_GC_FORCE_EVACUATE"); assert_eq!( gc_force_evacuate_enabled(), baseline, diff --git a/crates/perry-runtime/src/gc/trace.rs b/crates/perry-runtime/src/gc/trace.rs index 13ae67f2a6..886ff6361e 100644 --- a/crates/perry-runtime/src/gc/trace.rs +++ b/crates/perry-runtime/src/gc/trace.rs @@ -37,12 +37,7 @@ pub(super) fn classifier_verify_enabled() -> bool { return false; } static CACHED: std::sync::OnceLock = std::sync::OnceLock::new(); - *CACHED.get_or_init(|| { - matches!( - std::env::var("PERRY_GC_VERIFY_CLASSIFIER").as_deref(), - Ok("1") | Ok("on") | Ok("true") - ) - }) + *CACHED.get_or_init(|| super::env_flag_enabled("PERRY_GC_VERIFY_CLASSIFIER")) } thread_local! { diff --git a/crates/perry-runtime/src/gc/verify.rs b/crates/perry-runtime/src/gc/verify.rs index 2b98a272b5..423fb2aed1 100644 --- a/crates/perry-runtime/src/gc/verify.rs +++ b/crates/perry-runtime/src/gc/verify.rs @@ -625,7 +625,7 @@ pub(super) fn verify_old_to_young_edges_covered() -> OldYoungEdgeVerifyStats { // in the same cycle) can be reached. use std::sync::OnceLock; static NONFATAL: OnceLock = OnceLock::new(); - if *NONFATAL.get_or_init(|| std::env::var_os("PERRY_GC_VERIFY_RS_NONFATAL").is_some()) { + if *NONFATAL.get_or_init(|| super::env_flag_enabled("PERRY_GC_VERIFY_RS_NONFATAL")) { eprintln!( "[gc-verify] old-young-edge-verifier (non-fatal): missing_edges={}", stats.missing_edges diff --git a/scripts/check_gc_env_knobs.py b/scripts/check_gc_env_knobs.py index 8f38576c92..0ce62cbbb8 100755 --- a/scripts/check_gc_env_knobs.py +++ b/scripts/check_gc_env_knobs.py @@ -27,8 +27,29 @@ rf"{NEVER_SHIPPED_NURSERY}|PERRY_WRITE_BARRIERS|PERRY_SHADOW_STACK|" rf"PERRY_RS4GC|PERRY_STACKMAP_WALKER|PERRY_CONSERVATIVE_STACK_SCAN)\b" ) +# A knob is "owned" either by a direct environment read or by one of the two +# shared boolean-ish readers (#7991) — the latter ARE environment reads, one +# indirection away, and a knob routed through them must not read as unparsed. PARSER_RE = re.compile( - r'(?:std::env::var(?:_os)?|env_var)\(\s*"(PERRY_[A-Z0-9_]+)"\s*\)' + r"(?:std::env::var(?:_os)?|env_var|env_flag_enabled|env_default_on_enabled)" + r'\(\s*"(PERRY_[A-Z0-9_]+)"\s*\)' +) + +# #7991: a GC knob read for mere PRESENCE is a bug, not a style choice. +# `var_os("PERRY_GC_DIAG").is_some()` made `PERRY_GC_DIAG=0` turn diagnostics +# ON — and `off`, `false` and the empty string with it — which silently +# collapsed an A/B arm during #7803 triage: the investigator's "clean" control +# arm was instrumented exactly like the arm it was controlling for. A knob +# that fails toward a confident wrong answer is worse than one that fails +# loudly, so the shape is rejected outright rather than reviewed case by case. +# +# Boolean GC knobs go through `gc::env_flag_enabled` (default-OFF, #5093 +# vocabulary) or `gc::env_default_on_enabled` (default-ON kill switch). Both +# are pure functions of the raw value underneath, so both directions are +# testable without touching the process environment +# (`gc/tests/env_knob_parse.rs`). +PRESENCE_ONLY_RE = re.compile( + r'std::env::var_os\(\s*"(PERRY_[A-Z0-9_]+)"\s*\)\s*\.\s*is_(?:some|none)\s*\(\s*\)' ) PARSER_ROOTS = ( @@ -60,6 +81,12 @@ "PERRY_GC_EVIDENCE_DIR": "scripts/run_memory_stability_tests.sh", } +# Presence-only reads that are knowingly kept, name -> written reason. It is +# EMPTY and should stay that way: every new hit is a red build. An entry that +# matches nothing also fails, so a fix must delete its own exemption rather +# than leave a stale licence behind. +PRESENCE_ONLY_ALLOWED: dict[str, str] = {} + def strip_rust_comments(source: str) -> str: """Remove comments so a deleted parser cannot survive as coverage.""" @@ -86,6 +113,35 @@ def parsed_knobs(root: Path) -> dict[str, set[str]]: return found +def presence_only_reads(root: Path) -> dict[str, set[str]]: + """GC-family knobs read for presence rather than value (#7991).""" + found: dict[str, set[str]] = defaultdict(set) + for path in production_rust_files(root): + rel = path.relative_to(root).as_posix() + source = strip_rust_comments(path.read_text(encoding="utf-8")) + for name in PRESENCE_ONLY_RE.findall(source): + if KNOB_RE.fullmatch(name): + found[name].add(rel) + return found + + +def presence_only_problems(found: dict[str, set[str]]) -> list[str]: + problems = [] + for name in sorted(set(found) - set(PRESENCE_ONLY_ALLOWED)): + paths = ", ".join(sorted(found[name])) + problems.append( + f"{name}: read for presence (var_os(..).is_some()/.is_none()) in {paths}; " + f"'{name}=0' would ENABLE it. Use gc::env_flag_enabled (default-OFF) or " + f"gc::env_default_on_enabled (default-ON kill switch)." + ) + for name in sorted(set(PRESENCE_ONLY_ALLOWED) - set(found)): + problems.append( + f"{name}: presence-only exemption is stale — no such read remains, " + f"so delete its PRESENCE_ONLY_ALLOWED entry" + ) + return problems + + def claim_files(root: Path): yield root / "CLAUDE.md" for base in CLAIM_ROOTS: @@ -123,13 +179,17 @@ def problems_for( def self_test() -> int: live = "PERRY_GC_" + "LIVE" deleted = "PERRY_GEN_GC_" + "DELETED" + indirect = "PERRY_GC_" + "INDIRECT" + killswitch = "PERRY_GC_" + "KILLSWITCH" source = ( f'let _ = std::env::var("{live}");\n' f'// let _ = std::env::var("{deleted}");\n' + f'let _ = super::env_flag_enabled("{indirect}");\n' + f'let _ = env_default_on_enabled("{killswitch}");\n' ) extracted = set(PARSER_RE.findall(strip_rust_comments(source))) failures = [] - if extracted != {live}: + if extracted != {live, indirect, killswitch}: failures.append(f"comment stripping admitted the wrong parser set: {sorted(extracted)}") claims = {live: {"docs/current.md"}, deleted: {"scripts/live.sh"}} @@ -143,11 +203,54 @@ def self_test() -> int: if historical not in HISTORICAL_DOCS: failures.append("the historical generational plan lost its exact exemption") + # #7991: the presence-only detector must be able to say no. Sabotage it + # with the exact shape that shipped, and with the value-parsed shape that + # replaced it, and require it to distinguish them. + bug = "PERRY_GC_" + "SABOTAGE" + presence_source = f'if std::env::var_os("{bug}").is_some() {{}}' + negated_source = f'if !std::env::var_os("{bug}").is_none() {{}}' + fixed_source = f'if env_flag_enabled("{bug}") {{}}' + commented_source = f'// if std::env::var_os("{bug}").is_some() {{}}' + for label, source, expect_hit in ( + ("presence is_some", presence_source, True), + ("presence is_none", negated_source, True), + ("value-parsed", fixed_source, False), + ("commented out", commented_source, False), + ): + hit = bool(PRESENCE_ONLY_RE.findall(strip_rust_comments(source))) + if hit != expect_hit: + failures.append( + f"presence-only detector misread the {label} shape " + f"(saw hit={hit}, wanted {expect_hit})" + ) + if not presence_only_problems({bug: {"runtime.rs"}}): + failures.append("an unexempted presence-only GC read passed") + PRESENCE_ONLY_ALLOWED[bug] = "self-test" + try: + if presence_only_problems({bug: {"runtime.rs"}}): + failures.append("an exempted presence-only read was still rejected") + if not any("stale" in p for p in presence_only_problems({})): + failures.append("a stale presence-only exemption passed") + finally: + del PRESENCE_ONLY_ALLOWED[bug] + if PRESENCE_ONLY_ALLOWED: + failures.append( + "PRESENCE_ONLY_ALLOWED is no longer empty; every GC knob must be " + "value-parsed" + ) + # A non-GC name must NOT be flagged: this checker owns the GC family only. + non_gc = "PERRY_" + "DEBUG" + if KNOB_RE.fullmatch(non_gc): + failures.append("KNOB_RE claimed a non-GC knob; the family boundary moved") + for failure in failures: print(f"GC env-knob self-test FAILED: {failure}", file=sys.stderr) if failures: return 1 - print("GC env-knob self-test: OK (dead and commented parsers are rejected)") + print( + "GC env-knob self-test: OK (dead and commented parsers are rejected; the " + "presence-only detector distinguishes a sabotaged read from a fixed one)" + ) return 0 @@ -160,7 +263,9 @@ def main() -> int: parsers = parsed_knobs(REPO) claims = claimed_knobs(REPO) + presence = presence_only_reads(REPO) problems = problems_for(claims, parsers, REPO) + problems.extend(presence_only_problems(presence)) if problems: print("GC environment-knob drift check FAILED:", file=sys.stderr) for problem in problems: @@ -173,7 +278,8 @@ def main() -> int: return 1 print( f"GC environment-knob drift check OK: {len(claims)} claimed knobs, " - f"{len(parsers)} live env parsers, {len(HISTORICAL_DOCS)} historical documents exempt" + f"{len(parsers)} live env parsers, {len(HISTORICAL_DOCS)} historical documents " + f"exempt, 0 presence-only GC reads" ) return 0 From ca90c50cdef7cb13074883fe311ed389593740bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 12 Aug 2026 22:52:52 +0200 Subject: [PATCH 2/2] docs(changelog): fragment for #7993 (GC env-knob value parsing) --- changelog.d/7993-gc-diag-knob-value-parse.md | 62 ++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 changelog.d/7993-gc-diag-knob-value-parse.md diff --git a/changelog.d/7993-gc-diag-knob-value-parse.md b/changelog.d/7993-gc-diag-knob-value-parse.md new file mode 100644 index 0000000000..5b09298fc6 --- /dev/null +++ b/changelog.d/7993-gc-diag-knob-value-parse.md @@ -0,0 +1,62 @@ +### GC env knobs: `PERRY_GC_DIAG=0` no longer ENABLES diagnostics (#7991) + +`gc_diag_enabled()` read its knob with `var_os(..).is_some()` — **presence, +not value** — so `PERRY_GC_DIAG=0` turned diagnostics on, and so did `off`, +`false` and the empty string. + +That is a measurement-integrity bug rather than a cosmetic one. During #7803 +triage it silently collapsed an A/B arm: the investigator disabled +diagnostics for the clean arm and got them in *both*, so the arms were no +longer different in the way intended. It fails toward a **confident wrong +answer**, not a visible error. It was also inconsistent with its immediate +neighbour — `PERRY_GC_PROTECT_FROMSPACE` has parsed its value properly all +along, so `=0` really was off there. + +**The audit found the same shape on 25 read sites across four knobs.** +`PERRY_GC_DIAG` (20 sites), `PERRY_GC_VERIFY_MARK` (3 — `=0` armed a +whole-heap verifier), `PERRY_GC_VERIFY_RS_NONFATAL` (1), and +`PERRY_GC_VERIFY_EVACUATION`, which was *split-brain*: value-parsed in +`gc/mod.rs`, presence-parsed in the barrier's ever-dirty tracker, so `=0` +switched the verifier off while leaving its side table populated on every +barrier. One adjacent find outside the GC family: `PERRY_SHAPE_LAYOUT_KEYED` +was `v != "0"`, so its documented off-state worked only for the literal `0` +— `=off` and `=false` read as ON. + +**There are now exactly two boolean vocabularies**, both pure functions of +the raw value so both directions are testable without touching the process +environment: + +* `gc::env_flag_from_value` — default-OFF (#5093): `1`/`true`/`on`/`yes`; + unset, the off-spellings, empty and anything **unrecognised** read OFF. +* `gc::env_default_on_from_value` — default-ON kill switch: OFF only on an + explicit `0`/`off`/`false`/`no`; unrecognised leaves the shipping default + ON. + +They are deliberately **not** each other's negation — each fails toward its +own documented default — and that asymmetry has its own assertion so a future +tidy-up cannot collapse one into the other. Also unified onto them: +`PERRY_GC_TRACE`, `PERRY_GC_VERIFY_CLASSIFIER`, `PERRY_GC_FORCE_EVACUATE`, +`PERRY_GEN_GC`, `PERRY_WRITE_BARRIERS`, `PERRY_GC_MOVING_SAFEPOINT`, +`PERRY_GC_INCREMENTAL`, `PERRY_SHAPE_LAYOUT_KEYED`, and +`PERRY_GC_SAFEPOINT_ONLY`'s boolean arm (`strict` stays its own third state). + +**Teeth, because this is precisely the class where a doc comment is not a +change.** `gc/tests/env_knob_parse.rs` pins both vocabularies over on / off / +unrecognised spellings — but those pure cases would *all stay green* if that +one line reverted to presence-parsing, so the decisive case observes the +**live cached reader in a child process** under a real `PERRY_GC_DIAG=0` / +`off` / `` / `1`. The ON arm is there so a fix that hard-wires `false` cannot +pass either. Separately, `scripts/check_gc_env_knobs.py` (already in `lint`) +now rejects the presence-only shape outright for the GC family; its +exemption list is empty, a **stale** entry also fails so a fix must delete +its own licence, and its `--self-test` sabotages the detector with the exact +shape that shipped and requires it to be told apart from the replacement. + +Sabotage-verified: with the fix committed, `telemetry.rs` was reverted in +place and both teeth fired (`PERRY_GC_DIAG=Some("0") must read as OFF`; and +the lint gate naming the file), then restored **and rebuilt** to re-confirm +green. + +Diagnostic-only by contract, so no program semantics change; every in-repo +use of these knobs is `=1`. The damage was to investigations — any prior A/B +that used `PERRY_GC_DIAG=0` as its control arm was not controlled.