fix(gc): refuse the knob combination that emits a rootless binary (#7326) - #7332
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe codegen now rejects ChangesRootless backend validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 3, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* fix(gc): split the precise-root analysis from its lowering One knob answered two questions. "Which locals hold GC pointers, and where must each stay live" is the analysis and is backend-independent. "Is that answer represented as a heap-backed shadow frame or a native stack map" is the lowering, and LlFunction already chose it independently -- enable_shadow_frame_inner and reserve_shadow_slot both take the native path first. But the eight sites that build the slot map all gated on shadow_stack_enabled(), so PERRY_SHADOW_STACK=0 switched the ANALYSIS off and left the statepoint lowering with nothing to lower. The result was a binary with no precise frame roots at all: no __perry_gcmap section, same size as a plain shadow-off build, correct output. Nothing distinguished it from a good build until a collection freed a live object. #7332 made the pair a hard error as a stopgap. Route those eight sites through precise_root_analysis_enabled() instead and the pair becomes expressible, which is what the stopgap was standing in for. Measured on 01_nursery_churn: PERRY_STATEPOINTS=1 with and without PERRY_SHADOW_STACK=0 now emit an identical 885-byte root map and an identical __text. The knob keeps its own meaning on its own -- no gcmap, and still observable against the default build. A mode nobody can select is a mode nobody can measure, so this is the prerequisite for the shadow-stack lowering ever being removed rather than merely being switched off in one configuration nobody tests. * docs: changelog fragment for #7340 * docs(gc): record the full-suite RS4GC result and correct the x86-64 mechanism Two corrections and one measurement. The gap suite re-run against RS4GC in-process, two arms per test (shadow-stack control + RS4GC), 479/479: 447 pass->pass, 19 pre-existing diffs unchanged, 13 node_fail, ZERO new regressions, ZERO refusals, ZERO compile failures. Zero refusals is the load-bearing number -- 128 of the 479 tests contain `try {}` and the bridge cannot compile any of them. The earlier soak's "13 regressions, do not flip" was measured against the bridge, before #7329/#7330, on a backend that structurally cannot compile a quarter of the suite. It should not be carried forward. And the x86-64 mechanism was wrong. The workflow comment claimed _Unwind_GetGR(ctx, 7) "does not reliably return the stack pointer". Measured on x86-64 Linux (glibc 2.39, gcc 13.3.0): it SEGFAULTS. RBX, RBP and RIP return correctly; RAX and RSP both SIGSEGV, because libgcc tracks only the columns CFI restores and RSP is derived from the CFA rather than tracked. The fault is in the call itself, so no address validation after it can help -- the previous wording pointed at the wrong fix. Details and a reproducer in #7333. * docs(gc): measure the statepoint binary-size axis — it is root density, not metadata The plan asserted 'closing that axis needs fewer roots, not a tighter encoding' on the strength of one app measurement. Measured directly with two 2000-function programs: root-free functions +0 bytes (no map emitted, text identical) root-dense functions +4,330,592 B (97% __text, 21% gcmap) So statepoints carry NO fixed cost -- a function with nothing live across a safepoint pays nothing -- and the growth is the per-root relocation sequence, not the map. #7314's compact map fully answered the metadata objection, but metadata was never the dominant term at scale. Runtime on the same probes, quiet host, median of 5: statepoints 1-2% faster, every probe neutral or faster. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 12, 2026
…nfreeze the dialect corpora (#7998) * fix(codegen): teach the in-process native backend today's RS4GC IR, refresh the frozen corpora `PERRY_LLVM_INPROCESS=native` could not build any function containing an RS4GC root slot — which is effectively every function. Every `main` execution of `llvm-inprocess`'s native-backend job failed at in line: %r5 = alloca ptr addrspace(1) The issue warned this was not a one-liner, and it was not: fixing each shape only exposed the next. The family, in the order it surfaced: 1. `basic_type` had no `addrspace(N)` arm — the reported failure. 2. `ty_and_val` split `ptr addrspace(1) null` into type `ptr` and value `addrspace(1) null`. The qualifier belongs to the type but sits after a space, so OPERAND position needed the same treatment as type position. 3. `constant` returned an addrspace(0) `null` regardless of the operand type, which the verifier rejects when stored into an `alloca ptr addrspace(1)`. 4. Define lines carry `"frame-pointer"="non-leaf"` (a string attribute) and `gc "statepoint-example"` (which contains a space, so the whitespace-split attribute loop saw two junk tokens). 5. Callsites carry `"gc-leaf-function"`. 6. Landing pads are now `landingpad token cleanup`. `token` is not an inkwell `BasicType`, so that pad is built through llvm-sys. The `{ptr, i32} catch` shape still occurs and keeps its branch. Two further defects were hiding behind the reader failure, both worse than it: **The native path returned assembly and called it an object.** The statepoint backends compact the stack map at assembly time, so the plan carries `-S`. The textual in-process path has always run the rewrite-and-assemble step afterwards; the native and diff paths returned the bytes straight to the object cache, and the link died with `ld: unknown file type`. Factored into `linker::finish_native_emission` and wired into all four native/diff emit sites. **A natively-constructed module had NO GC strategy, so RS4GC never ran on it.** `native_emit::synth_define_header` was a second, independent copy of `LlFunction::to_ir`'s header renderer, written before `"frame-pointer"="non-leaf"` and `gc "statepoint-example"` existed and never updated. The result verifies, links and executes correctly on any program that does not collect, while having no precise roots at all — #7332's shape, and invisible to a behaviour-parity smoke arm by construction. The two callers now share `LlFunction::define_header`, so the next attribute reaches both; `function.rs` (compiled without the `llvm-inprocess` feature, i.e. visible to per-PR CI) pins `to_ir`'s first line against it. **The corpora, which is the half worth more than the fix.** All three tracked `.ll` files froze on 2026-08-03, 151 codegen commits earlier, and contained zero `addrspace(1)`. `corpus_spike ... ok` proved the tests ran, not that they test today's IR — CLAUDE.md's fourth way a gate cannot fail, inside the liveness assert written to prevent it. The same thing had happened nine days earlier (#7310, stale setjmp calls), which is the argument against fixing it by hand again: * `scripts/refresh_llvm_inprocess_corpora.sh` regenerates all three from the built compiler (`--check` diffs instead of writing). * `scripts/check_llvm_corpus_currency.py` (added to `lint`) asserts every IR form the reader carries a dedicated branch for is PRESENT in the corpora, so a form that disappears fails and must be either refreshed or deleted per the kill policy. Sabotage-verified against the corpora this commit replaces: it names all 10 forms they lacked. * `llvm-inprocess.yml`'s existing age diagnostic was itself vacuous — `git log` on a depth-1 checkout printed `0` commits behind however stale the files were. Now `fetch-depth: 0`, and it fails loudly rather than printing a reassuring zero if history is ever missing again. Corpora refreshed: 497 / 1082 / 420 `addrspace(1)` sites, EH corpus keeps 84 invoke edges and its personality clause. `dialect/mod.rs` crossed the 2000-line cap, so type/constant parsing moved to `dialect/types.rs`. Validated locally against LLVM 22.1.4: 934 `perry-codegen` lib tests green (all three corpus round-trips included); `PERRY_LLVM_INPROCESS=native` compiles and runs the spike and the try/catch program with output byte-identical to the textual backend. Known remaining, NOT closed here and separate defects: `=diff` still reports a byte mismatch on the spike (149,105 text vs 163,902 native — it was 50,995 before the GC-strategy fix, i.e. the gap narrowed from "RS4GC never ran" to a real but much smaller divergence), and the unit-split diff arm fails with `call to undeclared @js_shadow_slot_set`. Neither was ever reached in CI, because the native arm failed first. Refs #7982. * docs(changelog): fragment for #7998 (native backend RS4GC IR + corpus currency) * docs(gc-handoff): KNOBS-NOTES for the #7991/#7982/#7737 batch The knob-parse audit inventory (every GC-family env knob, how it parsed before and after), the corpus-currency findings, and the #7737 triage — including two things worth more than either fix: * `llvm-inprocess.yml`'s corpus-age diagnostic was itself vacuous. It asks `git log` how many IR-affecting commits landed since the corpora changed, on a depth-1 checkout — one commit to answer from, so it printed a confident 0 however stale the files were. * #7737 item 4's stated prerequisite has gone stale in the opposite direction: `gc-ratchet` has zero successes in its last 30 `main` runs and `gc-stress` failed the last three, so promoting either to required today would block every open PR. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 12, 2026
… lowerings (#8000) * test(codegen): assert the GC strategy reaches the define line, in BOTH lowerings The header/`to_ir` agreement test added alongside `LlFunction::define_header` cannot see the defect it was written for: with one shared renderer, dropping `gc "statepoint-example"` changes both sides identically and the test stays green. Verified by doing exactly that. The first replacement branched on `native_stack_roots_enabled()`. Under `cargo test` that predicate is false — no module has called `set_native_roots_for_target` — so the ON arm never executed and the same sabotage passed a second time. That is this change's own bug class, reproduced inside its own test, twice. `NativeRootsPin` exists for this. Both lowerings now run every time, and each arm asserts `stack_map_slot_count` first so neither can pass vacuously: native-roots must take the stack-map path AND name the strategy; shadow-stack must not take it AND must not name the strategy. Sabotage-verified with the fix committed first: dropping the strategy from the one renderer fails this test with the message naming #7332's shape, and the other two stay green. Restored and REBUILT to re-confirm; 935 perry-codegen lib tests pass under `--features llvm-inprocess`, and the three `define_header_tests` also pass WITHOUT the feature, i.e. in per-PR CI. Refs #7982. * docs(changelog): fragment for #8000 (GC-strategy define-header pin) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7326.
PERRY_SHADOW_STACK=0+PERRY_STATEPOINTS=1produced a binary with no precise frame roots at all — no__perry_gcmapsection, same size as a plain shadow-off build, and it ran and printed the correct answer. Nothing distinguished it from a correct build until a collection moved something live.The cause is structural, not a missing check
The statepoint backends are an alternative lowering of the shadow stack's root-set analysis, not an independent mechanism.
reserve_shadow_slot()is the single entry point that, undernative_stack_roots_enabled(), allocates a stack-map slot instead — and the caller returns empty maps outright when the shadow stack is off. Switching one off switches the other off with it.The consequence for the adoption plan
Because they share this analysis, "delete the shadow stack and keep statepoints" is not currently expressible. Any plan treating them as two interchangeable mechanisms — including the one in
docs/engine-plan.md— needs that premise corrected before the swap can happen. That is a decoupling job, not a flag flip.Verified
cargo fmtclean.Summary by CodeRabbit
Bug Fixes
Documentation