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. 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