fix(gc): parse PERRY_GC_DIAG by value — =0 no longer ENABLES diagnostics

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.

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={}",

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,

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<bool> = 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<bool> = 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()
     })
 }

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,

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");
         }
 

crates/perry-runtime/src/gc/layout.rs

@@ -159,11 +159,7 @@ fn shape_layout_keyed_enabled() -> bool {
     static E: OnceLock<bool> = 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,

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(

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)