diff --git a/changelog.d/8000-gc-strategy-define-header-pin.md b/changelog.d/8000-gc-strategy-define-header-pin.md new file mode 100644 index 0000000000..a1528a5006 --- /dev/null +++ b/changelog.d/8000-gc-strategy-define-header-pin.md @@ -0,0 +1,30 @@ +### Codegen: pin the GC strategy onto the define line, in both root lowerings (#7982 follow-up) + +#7998 made `LlFunction::define_header` the single renderer of the `define … {` +line, after the in-process native path's private copy silently lost +`gc "statepoint-example"` — a natively-constructed module then got no RS4GC +pass and therefore **no precise roots at all**, while verifying, linking and +running correctly on any program that does not collect. + +The test that shipped with it does not actually pin that property, and the two +attempts to fix it failed the same way the original bug did. Recorded because +it is this change's own bug class occurring inside its own test: + +1. The `to_ir` == `define_header` agreement test **cannot** see a dropped + strategy: with one shared renderer both sides change identically. Sabotage + passed. +2. A dedicated strategy test that branched on `native_stack_roots_enabled()` + never ran its ON arm under `cargo test` — no module has called + `set_native_roots_for_target`, so the predicate is false in the test + process. Sabotage passed again. +3. Only pinning **both** lowerings with `NativeRootsPin::{native,shadow}`, and + asserting `stack_map_slot_count` in each arm first so neither can pass + vacuously, goes red on the sabotage. + +Native-roots must take the stack-map path AND name the strategy; shadow-stack +must not take it AND must not name the strategy. The tests live in +`function.rs`, which compiles WITHOUT the `llvm-inprocess` feature, so they run +in per-PR `cargo-test` rather than only in the feature job. + +Sabotage-verified with the fix committed first, then restored and REBUILT: 935 +`perry-codegen` lib tests green under `--features llvm-inprocess`. diff --git a/crates/perry-codegen/src/function.rs b/crates/perry-codegen/src/function.rs index 8fc68f0208..78888e4e26 100644 --- a/crates/perry-codegen/src/function.rs +++ b/crates/perry-codegen/src/function.rs @@ -1022,6 +1022,80 @@ mod define_header_tests { } } + /// The property that was actually lost, asserted directly (#7982) — in + /// **both** lowerings, neither of them dark. + /// + /// The header/`to_ir` agreement test above cannot see this: with one shared + /// renderer, dropping `gc "statepoint-example"` changes both sides + /// identically and that test stays green. Verified by doing exactly that. + /// + /// The first version of this test read `native_stack_roots_enabled()` and + /// branched on it. Under `cargo test` that predicate is false — no module + /// has called `set_native_roots_for_target` — so the ON arm, the one that + /// matters, never executed and the same sabotage passed a second time. That + /// is the failure mode this whole change is about, reproduced inside its own + /// test. `NativeRootsPin` exists for precisely this; both arms now run every + /// time, and each carries a liveness assertion that its lowering was + /// actually selected. + #[test] + fn the_gc_strategy_appears_exactly_when_the_function_asks_for_a_stack_map() { + use crate::codegen::helpers::NativeRootsPin; + const STRATEGY: &str = "gc \"statepoint-example\""; + + { + let _native = NativeRootsPin::native(); + let plain = probe(); + assert!( + !plain.define_header(false).contains(STRATEGY), + "a function that never reserved a stack-map slot must not claim \ + a GC strategy, even under the native-roots lowering" + ); + + let mut mapped = probe(); + mapped.enable_shadow_frame(0); + assert!( + mapped.reserve_shadow_slot().is_some(), + "a shadow-framed function must yield a root slot" + ); + assert_eq!( + mapped.stack_map_slot_count, 1, + "under the native-roots pin the reservation must take the \ + stack-map path — otherwise the assertion below is vacuous" + ); + assert!( + mapped.define_header(false).contains(STRATEGY), + "a stack-map function's define line MUST name the GC strategy. \ + Without it RS4GC never runs on the module, which verifies, \ + links and executes correctly on any program that does not \ + collect while having NO precise roots at all — #7332's shape, \ + and exactly what the in-process native backend shipped until \ + #7982" + ); + } + + { + let _shadow = NativeRootsPin::shadow(); + let mut mapped = probe(); + mapped.enable_shadow_frame(0); + assert!( + mapped.reserve_shadow_slot().is_some(), + "a shadow-framed function must yield a root slot in this \ + lowering too" + ); + assert_eq!( + mapped.stack_map_slot_count, 0, + "under the shadow-stack pin the reservation must NOT take the \ + stack-map path" + ); + assert!( + !mapped.define_header(false).contains(STRATEGY), + "the shadow-stack lowering must NOT name a GC strategy — RS4GC \ + is not the backend in that build, and claiming it would run the \ + pass over IR that has no addrspace(1) roots to rewrite" + ); + } + } + /// `force_external` drops only the linkage keyword. The codegen-unit path /// depends on that and on nothing else changing. #[test] diff --git a/gc-handoff/KNOBS-NOTES.md b/gc-handoff/KNOBS-NOTES.md index 6d05f28be1..c22817ad39 100644 --- a/gc-handoff/KNOBS-NOTES.md +++ b/gc-handoff/KNOBS-NOTES.md @@ -129,6 +129,17 @@ Six shapes, each exposed by fixing the previous one: Fixed structurally (one `LlFunction::define_header`), pinned in `function.rs` which compiles WITHOUT the feature, so per-PR CI sees it. + ★ The pin took THREE attempts, and the first two failed the same way the bug + did. (a) The `to_ir == define_header` agreement test cannot see a dropped + `gc "statepoint-example"` at all — one renderer means both sides change + identically; sabotage passed. (b) A dedicated strategy test that BRANCHED on + `native_stack_roots_enabled()` never ran its ON arm under `cargo test`: no + module has called `set_native_roots_for_target`, so the predicate is false in + the test process and the sabotage passed again. Only (c), pinning both + lowerings with `NativeRootsPin::{native,shadow}` and asserting + `stack_map_slot_count` in each arm so neither is vacuous, goes red on the + sabotage. Reproduced this PR's own bug class twice inside its own test. + ### Are the `.ll` corpora live or still frozen? — **LIVE** Refreshed to 497 / 1082 / 420 `addrspace(1)` sites (were 0/0/0). Two mechanisms