Statepoint prerequisites: split the two oversize files, arm every GC knob, and find why the gate was never green - #7322
Conversation
📝 WalkthroughWalkthroughThe PR extracts precise-root lowering into a dedicated module, adds Statepoint and RS4GC paths, updates report configuration, expands AArch64 and x86-64 CI validation, moves linker tests, and documents the resulting platform and gate status. ChangesNative precise-root pipeline
Linker test extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NativeRootsWorkflow
participant PerryCompiler
participant StatepointReportAssert
participant GCWalkerTraceAssert
participant NativeRootsComplete
NativeRootsWorkflow->>PerryCompiler: compile probes and collect reports
PerryCompiler->>StatepointReportAssert: validate backend and totals
PerryCompiler->>GCWalkerTraceAssert: validate walker counters
StatepointReportAssert->>NativeRootsComplete: report arm result
GCWalkerTraceAssert->>NativeRootsComplete: report arm result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
8167ac0 to
a8557b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
crates/perry-codegen/src/function/precise_roots.rs (1)
1073-1098: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises both backends, but it runs only the statepoint backend.
for output in [lower_statepoints(input, 1)]iterates a single-element array. The RS4GC backend is never exercised here, andlower_roots_for_rs4gchas no unit test in this module. Add the RS4GC arm, or rename the test to state its actual scope.💚 Proposed test change
- for output in [lower_statepoints(input, 1)] { + let rs4gc = lower_precise_roots_to_native_stack( + input, + "probe", + 1, + PreciseRootBackend::Rs4gc, + ); + for output in [lower_statepoints(input, 1), rs4gc] {Note that the RS4GC output asserts differ: that backend marks audited callees
"gc-leaf-function"instead of emitting statepoints, so the arm needs its own expectations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/function/precise_roots.rs` around lines 1073 - 1098, Update audited_non_collecting_helpers_are_not_safepoints_in_either_backend to exercise both lower_statepoints and lower_roots_for_rs4gc. Give each backend its own assertions, expecting statepoint output only at the explicit collection boundary for lower_statepoints and the RS4GC-specific "gc-leaf-function" annotations without statepoints for lower_roots_for_rs4gc..github/workflows/gc-native-roots.yml (1)
216-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
__llvm_stackmapsabsence check for parity.The statepoint arm (Lines 108-111) and the RS4GC arm (Lines 340-343) assert both facts:
__perry_gcmappresent and__llvm_stackmapsgone. This arm asserts only the first, so it stays green if compaction stops running under the safepoint-only contract.♻️ Proposed change
otool -l "/tmp/so-$name" | grep -q "sectname __perry_gcmap" \ || { echo "::error::$name has no __perry_gcmap section — statepoint mode was not live"; exit 1; } + otool -l "/tmp/so-$name" | grep -q "sectname __llvm_stackmaps" \ + && { echo "::error::$name still carries __llvm_stackmaps — the compact rewrite did not run"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/gc-native-roots.yml around lines 216 - 219, Update the statepoint verification command near the existing __perry_gcmap assertion to also fail when otool reports an __llvm_stackmaps section. Match the checks used by the other statepoint and RS4GC arms: require __perry_gcmap to be present and __llvm_stackmaps to be absent for the generated /tmp/so-$name artifact.scripts/gc_walker_trace_assert.py (1)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the two walker flags mutually exclusive and required.
Both flags can be passed together today, which makes the assertion unsatisfiable and always fails. If neither flag is passed, the script only checks
walks > 0, so a misspelled flag name in a workflow arm would still exit 0 and assert nothing about the walker mode. That is the failure mode the module docstring exists to prevent.♻️ Proposed change
ap = argparse.ArgumentParser() ap.add_argument("trace") - ap.add_argument("--require-fp-walks", action="store_true") - ap.add_argument("--forbid-fp-walks", action="store_true") + mode = ap.add_mutually_exclusive_group(required=True) + mode.add_argument("--require-fp-walks", action="store_true") + mode.add_argument("--forbid-fp-walks", action="store_true") args = ap.parse_args()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gc_walker_trace_assert.py` around lines 62 - 66, Update the argument parser around argparse.ArgumentParser and the --require-fp-walks/--forbid-fp-walks definitions so the two flags form a mutually exclusive group that is required. Preserve the existing trace positional argument and ensure parsing rejects both flags together or neither flag, preventing an unrecognized walker-mode assertion from succeeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/gc-native-roots.yml:
- Around line 405-412: Tighten the grep expression in the x86-64 refusal check
to match only the distinctive compact-map refusal text, removing the broad
“stack map” alternative and any other generic wording that can occur in
unrelated diagnostics. Keep the existing failure handling and success message
unchanged, ensuring the gate passes only when the intended refusal behavior is
observed.
In `@crates/perry-codegen/src/function/precise_roots.rs`:
- Around line 1-7: Update the module-level documentation for the precise-roots
entry points lower_precise_roots_to_native_stack and
retype_landing_pads_for_statepoints to identify LlFunction::to_ir as their
caller instead of LlFunction::serialize; leave the rest of the documentation
unchanged.
- Around line 787-816: Update retype_landing_pads_for_statepoints so unused
Itanium landing pads are rewritten to landingpad token catch ptr null instead of
landingpad token cleanup. Preserve the existing register-use detection and leave
referenced landing pads unchanged, ensuring the catch-all handler remains
available during phase-1 unwinding.
In `@crates/perry-codegen/src/statepoint_report.rs`:
- Around line 8-12: Update statepoint reporting around enabled() so it no longer
reads PERRY_STATEPOINT_REPORT or any user-controlled environment value; require
the internal driver-to-worker setting established by --statepoint-report
instead. Add a regression test proving that setting the environment variable
alone does not activate reporting.
In `@crates/perry/src/commands/compile/run_pipeline.rs`:
- Around line 230-237: Before each compile in the pipeline, clear the
process-global statepoint report setting and drain existing records via the
relevant report-state API, including when no report format is selected. Then
update the logic around statepoint_report_format to set the environment variable
only when args.statepoint_report is Some, ensuring prior CLI or user-provided
values cannot affect subsequent builds.
In `@docs/engine-plan.md`:
- Around line 93-95: Update the documentation around PERRY_STATEPOINT_REPORT to
clarify that the driver may still set it internally for propagating report
configuration to Rayon workers, but it is not a user-facing configuration path;
state that --statepoint-report is the sole user-facing entry point.
- Around line 101-105: Update the changelog sentence in the entry describing
`#7319` to accurately state that 39 of 45 emitted IR files were byte-identical and
the remaining six differed only in the same-binary control comparison, rather
than claiming all 45 matched exactly.
- Around line 91-100: Update the CI-coverage summary in the documented list to
include the surviving PERRY_STATEPOINTS control and its assertion. Revise the
PERRY_STACKMAP_WALKER description so the unwind case requires both fp_walks == 0
and walks > 0, while preserving the existing verify assertion.
In `@scripts/statepoint_report_assert.py`:
- Around line 37-48: Update load to read the file through a context manager and
attempt JSON decoding at each “{” candidate in the stream, continuing past
invalid candidates until a valid JSON object is found. Preserve the existing
statepoint-report validation for the decoded object, and only emit the decode
error after no candidate succeeds.
---
Nitpick comments:
In @.github/workflows/gc-native-roots.yml:
- Around line 216-219: Update the statepoint verification command near the
existing __perry_gcmap assertion to also fail when otool reports an
__llvm_stackmaps section. Match the checks used by the other statepoint and
RS4GC arms: require __perry_gcmap to be present and __llvm_stackmaps to be
absent for the generated /tmp/so-$name artifact.
In `@crates/perry-codegen/src/function/precise_roots.rs`:
- Around line 1073-1098: Update
audited_non_collecting_helpers_are_not_safepoints_in_either_backend to exercise
both lower_statepoints and lower_roots_for_rs4gc. Give each backend its own
assertions, expecting statepoint output only at the explicit collection boundary
for lower_statepoints and the RS4GC-specific "gc-leaf-function" annotations
without statepoints for lower_roots_for_rs4gc.
In `@scripts/gc_walker_trace_assert.py`:
- Around line 62-66: Update the argument parser around argparse.ArgumentParser
and the --require-fp-walks/--forbid-fp-walks definitions so the two flags form a
mutually exclusive group that is required. Preserve the existing trace
positional argument and ensure parsing rejects both flags together or neither
flag, preventing an unrecognized walker-mode assertion from succeeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d783f0e1-e31f-4b29-8d0c-56b8368c567d
📒 Files selected for processing (13)
.github/workflows/gc-native-roots.ymlchangelog.d/7322-statepoint-prerequisites.mdcrates/perry-codegen/src/function.rscrates/perry-codegen/src/function/precise_roots.rscrates/perry-codegen/src/linker.rscrates/perry-codegen/src/linker_tests.rscrates/perry-codegen/src/statepoint_report.rscrates/perry/src/commands/compile/run_pipeline.rsdocs/engine-plan.mddocs/src/cli/flags.mdscripts/gc_gate_wiring_check.pyscripts/gc_walker_trace_assert.pyscripts/statepoint_report_assert.py
| # Non-zero for the RIGHT reason. Any old failure (missing clang, a | ||
| # broken checkout) would also be non-zero, and a job green on an | ||
| # unrelated error is the hazard this whole workflow is about. | ||
| if ! grep -qiE "stack map|compact-map|gc roots would be invisible" /tmp/x86.out /tmp/x86.err; then | ||
| echo "::error::statepoint compilation failed on x86-64, but not with the compact-map refusal this job asserts. Read the output above: either the refusal message changed, or something unrelated is broken." | ||
| exit 1 | ||
| fi | ||
| echo "x86-64: statepoint compilation refuses, as expected, with the compact-map message." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the refusal-message match.
The pattern stack map|compact-map|gc roots would be invisible is broader than the refusal it pins. The substring stack map can appear in an unrelated diagnostic, for example a panic from lower_precise_roots_to_native_stack that mentions "a plain stack map". The job would then pass on the wrong failure, which is the hazard the comment above describes.
Match the distinctive part of the refusal text only.
🔒️ Proposed fix
- if ! grep -qiE "stack map|compact-map|gc roots would be invisible" /tmp/x86.out /tmp/x86.err; then
+ if ! grep -qiE "compact-map rewriter could not parse|gc roots would be invisible to the collector" /tmp/x86.out /tmp/x86.err; thenAs per coding guidelines: a CI gate "must assert that the behavior it measures actually executed".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Non-zero for the RIGHT reason. Any old failure (missing clang, a | |
| # broken checkout) would also be non-zero, and a job green on an | |
| # unrelated error is the hazard this whole workflow is about. | |
| if ! grep -qiE "stack map|compact-map|gc roots would be invisible" /tmp/x86.out /tmp/x86.err; then | |
| echo "::error::statepoint compilation failed on x86-64, but not with the compact-map refusal this job asserts. Read the output above: either the refusal message changed, or something unrelated is broken." | |
| exit 1 | |
| fi | |
| echo "x86-64: statepoint compilation refuses, as expected, with the compact-map message." | |
| # Non-zero for the RIGHT reason. Any old failure (missing clang, a | |
| # broken checkout) would also be non-zero, and a job green on an | |
| # unrelated error is the hazard this whole workflow is about. | |
| if ! grep -qiE "compact-map rewriter could not parse|gc roots would be invisible to the collector" /tmp/x86.out /tmp/x86.err; then | |
| echo "::error::statepoint compilation failed on x86-64, but not with the compact-map refusal this job asserts. Read the output above: either the refusal message changed, or something unrelated is broken." | |
| exit 1 | |
| fi | |
| echo "x86-64: statepoint compilation refuses, as expected, with the compact-map message." |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/gc-native-roots.yml around lines 405 - 412, Tighten the
grep expression in the x86-64 refusal check to match only the distinctive
compact-map refusal text, removing the broad “stack map” alternative and any
other generic wording that can occur in unrelated diagnostics. Keep the existing
failure handling and success message unchanged, ensuring the gate passes only
when the intended refusal behavior is observed.
Source: Coding guidelines
| //! Precise GC roots lowered onto the native frame (#7173 / #7174). | ||
| //! | ||
| //! Split out of `function.rs` only because of the 2,000-line cap; this is the | ||
| //! statepoint/RS4GC half of the module and nothing else moved with it. The | ||
| //! entry points are [`lower_precise_roots_to_native_stack`] and | ||
| //! [`retype_landing_pads_for_statepoints`], both called from | ||
| //! `LlFunction::serialize`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the caller name in the module docs.
The entry points are called from LlFunction::to_ir in crates/perry-codegen/src/function.rs (Lines 743 and 764), not from LlFunction::serialize.
📝 Proposed doc fix
-//! entry points are [`lower_precise_roots_to_native_stack`] and
-//! [`retype_landing_pads_for_statepoints`], both called from
-//! `LlFunction::serialize`.
+//! entry points are [`lower_precise_roots_to_native_stack`] and
+//! [`retype_landing_pads_for_statepoints`], both called from
+//! `LlFunction::to_ir`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //! Precise GC roots lowered onto the native frame (#7173 / #7174). | |
| //! | |
| //! Split out of `function.rs` only because of the 2,000-line cap; this is the | |
| //! statepoint/RS4GC half of the module and nothing else moved with it. The | |
| //! entry points are [`lower_precise_roots_to_native_stack`] and | |
| //! [`retype_landing_pads_for_statepoints`], both called from | |
| //! `LlFunction::serialize`. | |
| //! Precise GC roots lowered onto the native frame (`#7173` / `#7174`). | |
| //! | |
| //! Split out of `function.rs` only because of the 2,000-line cap; this is the | |
| //! statepoint/RS4GC half of the module and nothing else moved with it. The | |
| //! entry points are [`lower_precise_roots_to_native_stack`] and | |
| //! [`retype_landing_pads_for_statepoints`], both called from | |
| //! `LlFunction::to_ir`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/function/precise_roots.rs` around lines 1 - 7,
Update the module-level documentation for the precise-roots entry points
lower_precise_roots_to_native_stack and retype_landing_pads_for_statepoints to
identify LlFunction::to_ir as their caller instead of LlFunction::serialize;
leave the rest of the documentation unchanged.
| pub(super) fn retype_landing_pads_for_statepoints(ir: &str) -> String { | ||
| const ITANIUM: &str = "landingpad { ptr, i32 } catch ptr null"; | ||
| if !ir.contains(ITANIUM) { | ||
| return ir.to_string(); | ||
| } | ||
| let mut out = String::with_capacity(ir.len()); | ||
| for line in ir.lines() { | ||
| let rewritten = match line.split_once(" = ") { | ||
| Some((reg, rest)) if rest.trim() == ITANIUM => { | ||
| let reg = reg.trim(); | ||
| // Referenced anywhere else? Then its payload is live. | ||
| let used = ir.lines().any(|other| { | ||
| !std::ptr::eq(other.as_ptr(), line.as_ptr()) && mentions_register(other, reg) | ||
| }); | ||
| if used { | ||
| None | ||
| } else { | ||
| Some(format!("{} = landingpad token cleanup", reg)) | ||
| } | ||
| } | ||
| _ => None, | ||
| }; | ||
| match rewritten { | ||
| Some(r) => out.push_str(&r), | ||
| None => out.push_str(line), | ||
| } | ||
| out.push('\n'); | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the landing-pad emission and the exception hand-off in try lowering.
rg -nP --type=rust -C8 'landingpad' crates/perry-codegen/src | head -120Repository: PerryTS/perry
Length of output: 10121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== precise_roots outline =="
ast-grep outline crates/perry-codegen/src/function/precise_roots.rs --view expanded | sed -n '1,220p'
echo "== precise_roots relevant section =="
sed -n '720,830p' crates/perry-codegen/src/function/precise_roots.rs
echo "== try_stmt relevant section =="
sed -n '1,220p' crates/perry-codegen/src/stmt/try_stmt.rs
echo "== eh landingpad/exception personality relevant sections =="
sed -n '90,180p' crates/perry-codegen/src/dialect/eh.rs
rg -nC5 'catch.*null|perry_eh_personality|get_personality_function|ExceptionObject|exception|payload|throw' crates/perry-codegen/src crates/perry --glob '*.rs' | sed -n '1,260p'Repository: PerryTS/perry
Length of output: 44282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== references to retype_landing_pads_for_statepoints =="
rg -nC6 'retype_landing_pads_for_statepoints|lower_precise_roots_to_native_stack' crates/perry-codegen/src crates/perry --glob '*.rs'
echo "== precise_roots tests around rewrite =="
sed -n '834,878p' crates/perry-codegen/src/function/precise_roots.rs
echo "== function.rs precision lowering flow around relevant lines =="
sed -n '700,760p' crates/perry-codegen/src/function.rs
echo "== search for landingpad token occurrences =="
rg -n 'landingpad token|cleanup|catch ptr null|LandingPadClause|Catch' crates crates/perry —glob '*.rs' crates/** 2>/dev/null | sed -n '1,220p'
echo "== LLVM language docs from local installed docs? =="
rg -n 'landingpad token cleanup|landingpad token catch|LandingPadClause|catch ptr null|cleanup landing pad' /usr/local/include /usr/include /usr/share/doc 2>/dev/null | sed -n '1,80p' || trueRepository: PerryTS/perry
Length of output: 40598
🌐 Web query:
LLVM IR landingpad token cleanup clause catch ptr null cleanup personality catch phase 1
💡 Result:
In LLVM IR, the landingpad instruction is used to define a landing pad, which serves as the unwind destination for invoke instructions [1][2]. The components mentioned—cleanup, catch clauses, and the null pointer—play specific roles in the exception handling mechanism [1][3]. Cleanup Clause The cleanup clause indicates that the landing pad must be executed whenever the stack is unwound through that scope, regardless of whether a catch clause matches the exception [1][4]. A landingpad instruction should include a cleanup clause if it contains code that must run as part of unwinding (e.g., C++ destructors) [1][3]. Catch Clauses and Null Pointers Catch clauses are used to specify exception types that the landing pad is prepared to handle [1][2]. If a catch clause specifies a null pointer (catch ptr null), it acts as a catch-all block, matching any exception thrown [1][3]. This is the standard way to represent C++ catch-all blocks like catch (...) [1][3]. Phase 1 Unwinding Exception unwinding typically occurs in two phases: 1. Phase 1 (Search Phase): The runtime unwinder searches the stack for a handler that can catch the exception [5]. During this phase, the unwinder skips landing pads that contain only cleanup clauses [5][6]. Only landing pads with matching catch or filter clauses will stop the search in this phase [5]. 2. Phase 2 (Cleanup/Handler Phase): The runtime unwinder returns to the frames identified in Phase 1, executing cleanup code and finally the catch handler [1][3]. Because cleanup landing pads are skipped during Phase 1, they are essentially invisible to the search phase [5][6]. If a function contains only cleanup landing pads, the runtime continues searching past these frames until it finds a suitable handler or terminates [5][6]. This behavior is critical for tools like FunctionAttrs, which must distinguish between landing pads that can actually catch an exception and those that only perform cleanups, in order to correctly infer whether a function may throw or unwind [5][6]. Personality Function The personality function, specified in the parent function’s attribute list, defines the language-specific and ABI-specific behavior of the exception handling runtime (e.g., gxx_personality_v0 for C++) [1][7]. It determines how the runtime interprets the landingpad clauses and how it manages the transition between Phase 1 and Phase 2 [1][7].
Citations:
- 1: https://llvm.org/docs/ExceptionHandling.html
- 2: https://blog.llvm.org/2011/11/llvm-30-exception-handling-redesign.html
- 3: https://releases.llvm.org/21.1.2/docs/ExceptionHandling.html
- 4: https://releases.llvm.org/20.1.0/docs/ExceptionHandling.html
- 5: https://reviews.llvm.org/D147694
- 6: llvm/llvm-project@9fe78db
- 7: https://clang.llvm.org/docs/LLVMExceptionHandlingCodeGen.html
Preserve the catch-all clause when retyping RS4GC landing pads.
retype_landing_pads_for_statepoints changes landingpad { ptr, i32 } catch ptr null into landingpad token cleanup. Cleanup-only landing pads are skipped during Itanium phase-1 scanning, so this can drop the only handler for blocks that currently unwind to it. Use landingpad token catch ptr null to keep RS4GC’s token requirement while preserving catch-all selection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/function/precise_roots.rs` around lines 787 - 816,
Update retype_landing_pads_for_statepoints so unused Itanium landing pads are
rewritten to landingpad token catch ptr null instead of landingpad token
cleanup. Preserve the existing register-use detection and leave referenced
landing pads unchanged, ensuring the catch-all handler remains available during
phase-1 unwinding.
| //! `PERRY_STATEPOINT_REPORT` is how the driver carries that flag across to the | ||
| //! rayon module workers — the driver sets it, nothing else should. It is not a | ||
| //! user-facing knob: accepting it from the environment made it a fifth GC env | ||
| //! knob with no CI arm, so that spelling was deleted under CLAUDE.md's GC knob | ||
| //! kill policy. `gc-native-roots.yml` exercises the report through the flag. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Remove the remaining user-controlled environment switch.
enabled() still reads PERRY_STATEPOINT_REPORT on Lines 89-96. Users can therefore enable reporting without --statepoint-report. This contradicts the documented removal of direct environment configuration and the PR objective.
Pass the report setting through an internal driver-to-worker mechanism that does not read arbitrary process environment in codegen. Add a regression test for environment-only activation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/statepoint_report.rs` around lines 8 - 12, Update
statepoint reporting around enabled() so it no longer reads
PERRY_STATEPOINT_REPORT or any user-controlled environment value; require the
internal driver-to-worker setting established by --statepoint-report instead.
Add a regression test proving that setting the environment variable alone does
not activate reporting.
| // | ||
| // `PERRY_STATEPOINT_REPORT` is written here and read by the rayon module | ||
| // workers; it is NOT a user-facing knob. It used to be accepted from the | ||
| // environment as a second spelling of `--statepoint-report`, which made it | ||
| // a fifth GC env knob with no CI arm — deleted under CLAUDE.md's kill | ||
| // policy (#7314 review item), leaving the flag as the single entry point. | ||
| // `--opt-report` keeps its env spelling because that one is not a GC knob. | ||
| let statepoint_report_format = args.statepoint_report; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear stale report state before each compile.
PERRY_STATEPOINT_REPORT is process-global. This code sets it only when args.statepoint_report is Some and never clears it. Because perry dev reuses the process, a build with --statepoint-report leaves reporting enabled for later builds without the flag. A user-provided environment value also remains effective because Rayon workers read this variable directly.
The no-report path also skips take_records(), so records can accumulate across repeated builds. Clear the internal variable and drain old records before each build, then set the variable only for the current CLI selection.
Proposed fix
let statepoint_report_format = args.statepoint_report;
+std::env::remove_var("PERRY_STATEPOINT_REPORT");
+let _ = perry_codegen::statepoint_report::take_records();
if let Some(fmt) = statepoint_report_format {
std::env::set_var(
"PERRY_STATEPOINT_REPORT",
@@
);
std::env::set_var("PERRY_NO_CACHE", "1");
- // `perry dev` reuses the process; discard records from its previous
- // build before starting this one.
- let _ = perry_codegen::statepoint_report::take_records();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry/src/commands/compile/run_pipeline.rs` around lines 230 - 237,
Before each compile in the pipeline, clear the process-global statepoint report
setting and drain existing records via the relevant report-state API, including
when no report format is selected. Then update the logic around
statepoint_report_format to set the environment variable only when
args.statepoint_report is Some, ensuring prior CLI or user-provided values
cannot affect subsequent builds.
| 1. ~~**Four of five new knobs have no CI arm.**~~ **Closed by #7319.** Every | ||
| surviving knob now has an arm that asserts its own subject was live, and the | ||
| fifth was deleted: `PERRY_STATEPOINT_REPORT` was a second spelling of | ||
| `--statepoint-report`, so the env spelling is gone and the flag is the only | ||
| entry point. `PERRY_RS4GC` asserts every function record carries | ||
| `backend: rs4gc` (it bails per function to the explicit bridge, so a green | ||
| 9/9 matrix proves nothing on its own); `PERRY_GC_SAFEPOINT_ONLY` asserts a | ||
| codegen differential (statepoints strictly down, skipped calls strictly up); | ||
| `PERRY_STACKMAP_WALKER` asserts `fp_walks > 0` under `verify` and | ||
| `fp_walks == 0` under `unwind`, from the GC trace. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make the CI-coverage summary complete.
PERRY_STATEPOINTS is a surviving control but is not named in this list. Also, the unwind assertion requires both fp_walks == 0 and walks > 0; fp_walks == 0 alone does not prove that the walker executed.
Proposed wording
+ `PERRY_STATEPOINTS` asserts statepoint backend records.
`PERRY_RS4GC` asserts every function record carries
`backend: rs4gc` ...
`PERRY_STACKMAP_WALKER` asserts `fp_walks > 0` under `verify` and
- `fp_walks == 0` under `unwind`, from the GC trace.
+ `fp_walks == 0` with `walks > 0` under `unwind`, from the GC trace.Based on the supplied scripts/statepoint_report_assert.py contract and the PR objectives.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. ~~**Four of five new knobs have no CI arm.**~~ **Closed by #7319.** Every | |
| surviving knob now has an arm that asserts its own subject was live, and the | |
| fifth was deleted: `PERRY_STATEPOINT_REPORT` was a second spelling of | |
| `--statepoint-report`, so the env spelling is gone and the flag is the only | |
| entry point. `PERRY_RS4GC` asserts every function record carries | |
| `backend: rs4gc` (it bails per function to the explicit bridge, so a green | |
| 9/9 matrix proves nothing on its own); `PERRY_GC_SAFEPOINT_ONLY` asserts a | |
| codegen differential (statepoints strictly down, skipped calls strictly up); | |
| `PERRY_STACKMAP_WALKER` asserts `fp_walks > 0` under `verify` and | |
| `fp_walks == 0` under `unwind`, from the GC trace. | |
| 1. ~~**Four of five new knobs have no CI arm.**~~ **Closed by `#7319`.** Every | |
| surviving knob now has an arm that asserts its own subject was live, and the | |
| fifth was deleted: `PERRY_STATEPOINT_REPORT` was a second spelling of | |
| `--statepoint-report`, so the env spelling is gone and the flag is the only | |
| entry point. `PERRY_STATEPOINTS` asserts statepoint backend records. | |
| `PERRY_RS4GC` asserts every function record carries | |
| `backend: rs4gc` (it bails per function to the explicit bridge, so a green | |
| 9/9 matrix proves nothing on its own); `PERRY_GC_SAFEPOINT_ONLY` asserts a | |
| codegen differential (statepoints strictly down, skipped calls strictly up); | |
| `PERRY_STACKMAP_WALKER` asserts `fp_walks > 0` under `verify` and | |
| `fp_walks == 0` with `walks > 0` under `unwind`, from the GC trace. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engine-plan.md` around lines 91 - 100, Update the CI-coverage summary in
the documented list to include the surviving PERRY_STATEPOINTS control and its
assertion. Revise the PERRY_STACKMAP_WALKER description so the unwind case
requires both fp_walks == 0 and walks > 0, while preserving the existing verify
assertion.
| fifth was deleted: `PERRY_STATEPOINT_REPORT` was a second spelling of | ||
| `--statepoint-report`, so the env spelling is gone and the flag is the only | ||
| entry point. `PERRY_RS4GC` asserts every function record carries |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify the remaining internal use of PERRY_STATEPOINT_REPORT.
The driver still sets this variable to propagate report configuration to Rayon workers. State that it is no longer a user-facing configuration path and that --statepoint-report is the only user-facing entry point.
Proposed wording
- so the env spelling is gone and the flag is the only entry point.
+ so the env spelling is no longer user-facing; the driver still propagates it
+ internally to Rayon workers, and the flag is the only user-facing entry point.Based on the PR objectives and the supplied configuration context.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fifth was deleted: `PERRY_STATEPOINT_REPORT` was a second spelling of | |
| `--statepoint-report`, so the env spelling is gone and the flag is the only | |
| entry point. `PERRY_RS4GC` asserts every function record carries | |
| fifth was deleted: `PERRY_STATEPOINT_REPORT` was a second spelling of | |
| `--statepoint-report`, so the env spelling is no longer user-facing; the driver still propagates it | |
| internally to Rayon workers, and the flag is the only user-facing entry point. `PERRY_RS4GC` asserts every function record carries |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engine-plan.md` around lines 93 - 95, Update the documentation around
PERRY_STATEPOINT_REPORT to clarify that the driver may still set it internally
for propagating report configuration to Rayon workers, but it is not a
user-facing configuration path; state that --statepoint-report is the sole
user-facing entry point.
| 2. ~~**#7314 broke the file-size gate.**~~ **Closed by #7319** — `function.rs` | ||
| 2036 → 952 (statepoint/RS4GC lowering into `function/precise_roots.rs`) and | ||
| `linker.rs` 2082 → 1618 (unit tests into `linker_tests.rs`, the pattern that | ||
| file already used for `linker_temp_lifecycle_tests.rs`). Verified by | ||
| byte-identical emitted IR over 45 modules × 3 modes. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not overstate the IR comparison result.
The changelog records 39 of 45 .ll files as byte-identical. The remaining six differ only in the same-binary control comparison. This sentence currently implies that all 45 comparisons were byte-identical.
Proposed wording
- Verified by byte-identical emitted IR over 45 modules × 3 modes.
+ Verified no refactor-induced IR differences over 45 modules × 3 modes:
+ 39/45 `.ll` files were byte-identical, and the remaining six matched the
+ same-binary control differences.Based on the comparison details in changelog.d/7322-statepoint-prerequisites.md.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 2. ~~**#7314 broke the file-size gate.**~~ **Closed by #7319** — `function.rs` | |
| 2036 → 952 (statepoint/RS4GC lowering into `function/precise_roots.rs`) and | |
| `linker.rs` 2082 → 1618 (unit tests into `linker_tests.rs`, the pattern that | |
| file already used for `linker_temp_lifecycle_tests.rs`). Verified by | |
| byte-identical emitted IR over 45 modules × 3 modes. | |
| 2. ~~**`#7314` broke the file-size gate.**~~ **Closed by `#7319`** — `function.rs` | |
| 2036 → 952 (statepoint/RS4GC lowering into `function/precise_roots.rs`) and | |
| `linker.rs` 2082 → 1618 (unit tests into `linker_tests.rs`, the pattern that | |
| file already used for `linker_temp_lifecycle_tests.rs`). Verified no | |
| refactor-induced IR differences over 45 modules × 3 modes: 39/45 `.ll` files | |
| were byte-identical, and the remaining six matched the same-binary control | |
| differences. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engine-plan.md` around lines 101 - 105, Update the changelog sentence in
the entry describing `#7319` to accurately state that 39 of 45 emitted IR files
were byte-identical and the remaining six differed only in the same-binary
control comparison, rather than claiming all 45 matched exactly.
| def load(path: str) -> dict: | ||
| text = open(path, encoding="utf-8", errors="replace").read() | ||
| start = text.find("{") | ||
| if start < 0: | ||
| sys.exit(f"::error::{path} contains no JSON report — was --statepoint-report=json passed?") | ||
| try: | ||
| report, _ = json.JSONDecoder().raw_decode(text[start:]) | ||
| except json.JSONDecodeError as exc: | ||
| sys.exit(f"::error::{path} does not decode as a statepoint report: {exc}") | ||
| if "totals" not in report or "functions" not in report: | ||
| sys.exit(f"::error::{path} is JSON but not a statepoint report (no totals/functions)") | ||
| return report |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Decode the first valid JSON object, not the first { character.
The docstring states that the stream is interleaved with linker warnings and driver chatter. Any earlier line that contains { — a linker warning, a Rust panic message, or a Debug-formatted value — makes raw_decode fail at that offset, and the whole arm fails with "does not decode as a statepoint report" while a valid report is present later in the file. Retry at each { candidate instead. Also use a context manager for the read.
🛡️ Proposed fix
def load(path: str) -> dict:
- text = open(path, encoding="utf-8", errors="replace").read()
- start = text.find("{")
- if start < 0:
+ with open(path, encoding="utf-8", errors="replace") as handle:
+ text = handle.read()
+ if "{" not in text:
sys.exit(f"::error::{path} contains no JSON report — was --statepoint-report=json passed?")
- try:
- report, _ = json.JSONDecoder().raw_decode(text[start:])
- except json.JSONDecodeError as exc:
- sys.exit(f"::error::{path} does not decode as a statepoint report: {exc}")
- if "totals" not in report or "functions" not in report:
- sys.exit(f"::error::{path} is JSON but not a statepoint report (no totals/functions)")
- return report
+ decoder = json.JSONDecoder()
+ last_error = None
+ start = text.find("{")
+ while start >= 0:
+ try:
+ report, _ = decoder.raw_decode(text[start:])
+ except json.JSONDecodeError as exc:
+ last_error = exc
+ else:
+ if isinstance(report, dict) and "totals" in report and "functions" in report:
+ return report
+ start = text.find("{", start + 1)
+ sys.exit(
+ f"::error::{path} carries no statepoint report with totals/functions "
+ f"(last decode error: {last_error})"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def load(path: str) -> dict: | |
| text = open(path, encoding="utf-8", errors="replace").read() | |
| start = text.find("{") | |
| if start < 0: | |
| sys.exit(f"::error::{path} contains no JSON report — was --statepoint-report=json passed?") | |
| try: | |
| report, _ = json.JSONDecoder().raw_decode(text[start:]) | |
| except json.JSONDecodeError as exc: | |
| sys.exit(f"::error::{path} does not decode as a statepoint report: {exc}") | |
| if "totals" not in report or "functions" not in report: | |
| sys.exit(f"::error::{path} is JSON but not a statepoint report (no totals/functions)") | |
| return report | |
| def load(path: str) -> dict: | |
| with open(path, encoding="utf-8", errors="replace") as handle: | |
| text = handle.read() | |
| if "{" not in text: | |
| sys.exit(f"::error::{path} contains no JSON report — was --statepoint-report=json passed?") | |
| decoder = json.JSONDecoder() | |
| last_error = None | |
| start = text.find("{") | |
| while start >= 0: | |
| try: | |
| report, _ = decoder.raw_decode(text[start:]) | |
| except json.JSONDecodeError as exc: | |
| last_error = exc | |
| else: | |
| if isinstance(report, dict) and "totals" in report and "functions" in report: | |
| return report | |
| start = text.find("{", start + 1) | |
| sys.exit( | |
| f"::error::{path} carries no statepoint report with totals/functions " | |
| f"(last decode error: {last_error})" | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 37-37: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, encoding="utf-8", errors="replace")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/statepoint_report_assert.py` around lines 37 - 48, Update load to
read the file through a context manager and attempt JSON decoding at each “{”
candidate in the stream, continuing past invalid candidates until a valid JSON
object is found. Preserve the existing statepoint-report validation for the
decoded object, and only emit the decode error after no candidate succeeds.
|
Second, independent check on the "pure move" claim — text-level, reproducible against All four are empty apart from the three widenings. That is the whole semantic delta of the split; the byte-identical-IR corpus in the description is the behavioural half of the same claim. |
1 participant
Clears the two mechanical prerequisites between statepoints (#7314, opt-in) and
becoming the default root mechanism, and prepares the third. It also found two
things that change what "adopt statepoints" means — one of them large.
1.
lint's file-size gate#7314 pushed two files over the 2,000-line cap. Both split along the seam that
already existed in them; no rename, no signature change, no behaviour change.
perry-codegen/src/function.rsfunction/precise_roots.rs(1099)perry-codegen/src/linker.rslinker_tests.rs(473)linker.rsalready carried#[path] mod linker_temp_lifecycle_tests;with thecomment "a sibling file only because of the 2,000-line cap" — the new module is
the same device applied to the block above it.
function.rsneeded a productionsplit because its test module is only 265 lines; the block that moved is exactly
what #7314 added, and three items widened from private to
pub(super). That isthe entire diff of intent.
Evidence: byte-identical emitted IR
Not "it compiles". Both arms built with the identical package set
(
-p perry -p perry-runtime-static -p perry-stdlib-static), binaries hashed andconfirmed different, then each emitted
--trace llvmover 15 modules × 3 modes(default,
PERRY_STATEPOINTS=1,PERRY_RS4GC=1) — 45.llfiles per arm. Thestatepoint and RS4GC modes are in there because the moved code runs in no other
mode; a default-only corpus would have been vacuous for the block that moved.
39/45 byte-identical. The other 6 differ under a same-binary control too.
Running arm A twice produces the same 6 files with the same per-file line counts
(4/2/4/2/4/2) as A-vs-B: Perry's tagged-template site id and its
__perry_cap_<hex>capture suffix are not deterministic across runs of onebinary. So the refactor's contribution to the diff is empty.
2. The GC knob kill-policy
Four knobs had no arm anywhere. All four are resolved, and each arm asserts its
own subject was live — because
PERRY_GC_FORCE_EVACUATEpassed for monthswhile being inert (#6942/#6946).
PERRY_STATEPOINT_REPORT--statepoint-report. The env read inrun_pipeline.rsis gone; the driver still sets the variable to reach the rayon workers, which is now its only role. The flag keeps an arm, so the mode is exercised while the knob is not a knob.PERRY_RS4GC--only-backend rs4gc: every function record must carrybackend: rs4gc. RS4GC bails per function to the explicit bridge on any unrecognised root-alloca shape, so a 9/9 green matrix is compatible with RS4GC having rewritten nothing. Measured 9/9 functions on the try/catch probe.PERRY_GC_SAFEPOINT_ONLYstrict)strictrun that never panics proves enforcement was armed, not that the contract did anything — and individual probes show a zero delta (09_try_catch_rootsis one), so the assert is aggregate by construction.PERRY_STACKMAP_WALKERverify+unwind)PERRY_GC_TRACE=1stream:verifyrequiresfp_walks > 0,unwindrequiresfp_walks == 0withwalks > 0. Every mode produces identical program output, so output alone can never say which walker ran.Two small assert helpers carry these, both with negative cases exercised locally:
scripts/statepoint_report_assert.pyandscripts/gc_walker_trace_assert.py.3. ★ Statepoints do not work on x86-64 (#7321)
gc-native-rootshas never been green, and not for a flaky reason. Onubuntu-latestthe compact-map rewriter refuses the first probe:That is the fail-closed path doing its job, so there is no correctness exposure —
what it changes is scope. The native-root mechanism is aarch64-only today, and
#7314's headline evidence (drizzle, 23,301 statepoints) is aarch64 evidence.
gc_map.rsdescribes its base registers in aarch64 terms throughout(
DWARF_REG_{FP,SP}_AARCH64, "x19 on aarch64"), which is consistent, though thisPR does not prove the cause.
The matrix moves to
macos-14, where the mechanism actually runs. The gap isasserted rather than dropped:
statepoints-refuse-x86compiles one probe onx86-64, requires a non-zero exit for the compact-map reason specifically, and
goes red the day x86-64 starts working — which is the prompt to widen the matrix
and close #7321.
A second latent defect in the same workflow
It set
RUSTFLAGS="-Cforce-frame-pointers=yes". Cargo takes rustflags fromexactly one source, so that replaced
.cargo/config.toml's[build] rustflagsand silently dropped
-C force-unwind-tables=yes— a trap that config filedocuments in a comment. A/B'd on one tree, runtime rebuilt each way:
09_try_catch_rootsaborts — "unwind tables are missing from thisruntime build (0 frame(s) visible to the unwinder)" — and 4 of 9 probes fail
PERRY_STACKMAP_WALKER=verifywith the unwinder visiting zero frames;The consequence is not cosmetic. On any host where the x29 chain walk is
unavailable, the unwinder is the walker — so it would find no roots at all,
while forced-evacuation verification stayed quiet, because it enumerates roots
through that same walker. Fixed here.
What ran, and where
Every arm was executed locally on aarch64 macOS — the same OS and architecture as
macos-14— by extracting therun:blocks from the workflow and executing themverbatim against this branch's build:
RS4GC additionally needs
optandclangfrom the same LLVM install — amismatched pair fails with
unterminated attribute grouponnocreateundeforpoison, which is the pairing Perry's independent discovery picksby default on a Mac. The arm pins both.
gc-native-roots-completeis a fan-in job mirroringconformance-smoke-complete,so branch protection needs one context rather than one per arm, and it is
registered in
scripts/gc_gate_wiring_check.pysolintnow asserts thisworkflow's own wiring.
Not verified here
statepoints-refuse-x86is the one jobnever executed; its assertion is derived from CI run 30823009708's log.
macos-14runner itself — arms were run on a local M1, not a GH runner —and
brew list llvm || brew install llvmon that image.path, and the refactor's IR identity is shown above.
cargo check -D warningsis already red onmain(variant NeverReturns is never constructed, perry-codegen) andcargo test -p perry-codegen --test loop_safepoint_purityalready fails 6/7 there. Both reproduce identically atorigin/mainwith this branch's files reverted. Not touched.Blocker 3 — the branch-protection edit (admin only)
Do not promote yet: the job has never been green in any shape, and per
CLAUDE.md a never-green required context blocks every open PR the day it lands.
Order:
push: branches: [main]trigger firesgc-native-roots.gc-native-roots-completeis green onmain— not on the PR.main→ Require status checks to pass → addgc-native-roots-complete(the fan-in only; adding the arms individuallyis what makes every future arm a protection edit).
gh api -X PATCH repos/PerryTS/perry/branches/main/protection/required_status_checks -f 'contexts[]=…'works too, but it replaces the list — send all eight:lint, cargo-test, parity, compile-smoke, api-docs-drift, security-audit, conformance-smoke-complete, gc-native-roots-complete.Until step 3,
gc-native-rootsstill reports without blocking — hazard 2, the onethat let #6925 survive three merges.
Refs #7314, #7173, #7174. Closes nothing on its own; #7321 tracks the x86-64 gap.
Summary by CodeRabbit
New Features
Documentation
--statepoint-report[=text|json]and removed the deprecated environment-based activation option.Tests