-
-
Notifications
You must be signed in to change notification settings - Fork 157
test(gc): enable the knob loop_safepoint_purity asserts on (6 tests red on main) #7320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| ### Fix `loop_safepoint_purity`, red on `main` since the moving-GC work | ||
|
|
||
| Six of the seven tests in `crates/perry-codegen/tests/loop_safepoint_purity.rs` | ||
| were failing on `main`. Every assertion in that file is about *which* loops keep | ||
| their back-edge `js_gc_loop_safepoint()` poll, but the poll is gated behind | ||
| `PERRY_GC_MOVING_LOOP_POLLS`, which defaults off — so the emitter returned | ||
| before the purity analysis ran and the six "sabotage" cases (a call in the body, | ||
| an object literal, string concatenation, a module-global accumulator, coercible | ||
| bound/accumulator) saw no poll for a reason unrelated to what they test. The one | ||
| test asserting a *pure* numeric loop drops its poll passed for the same wrong | ||
| reason. | ||
|
|
||
| The suite now enables the knob it asserts on, before the first codegen call — | ||
| `moving_safepoint_polls_enabled` caches in a `OnceLock`. | ||
|
|
||
| Worth noting how it stayed red: integration suites under `crates/*/tests/` do | ||
| not run per-PR (nightly/tag only), which CLAUDE.md already calls out as a way a | ||
| regression "can land green and sit red for days". | ||
|
|
||
| This makes the suite exercise the poll-ON path. The **default** path — polls | ||
| off, which is what ships — remains untested, and `PERRY_GC_MOVING_LOOP_POLLS` | ||
| is a GC knob with no CI arm covering either state. That is a kill-policy | ||
| question for the owner, not something this change decides. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,14 +125,32 @@ fn module_with_init(name: &str, init: Vec<Stmt>) -> Module { | |
| } | ||
| } | ||
|
|
||
| /// The back-edge poll is gated behind `PERRY_GC_MOVING_LOOP_POLLS`, which | ||
| /// defaults OFF. Every assertion in this file is about WHICH loops keep the | ||
| /// poll, so with the knob unset the emitter returns before the purity analysis | ||
| /// runs and six of these tests fail for a reason that has nothing to do with | ||
| /// what they test. | ||
| /// | ||
| /// That is how they went red: the knob arrived with the moving-GC work and | ||
| /// nothing here turned it on, while integration suites under `crates/*/tests/` | ||
| /// do not run per-PR, so nobody saw it. `moving_safepoint_polls_enabled` | ||
| /// caches in a `OnceLock`, so this must run before the first codegen call — | ||
| /// every helper that produces IR goes through here. | ||
| fn enable_back_edge_polls() { | ||
| // Safe on edition 2021, and every test in this binary wants the same value. | ||
| std::env::set_var("PERRY_GC_MOVING_LOOP_POLLS", "1"); | ||
| } | ||
|
Comment on lines
+139
to
+142
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
rg -n '^\s*edition\s*=' --glob 'Cargo.toml' .
cargo check -p perry --profile perry-dev
cargo check -p perry-codegen --tests --profile perry-devRepository: PerryTS/perry Length of output: 143 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
echo "== Cargo edition pins =="
rg -n '^\s*edition\s*=' --glob 'Cargo.toml' . || true
echo
echo "== Target file outline/size =="
if [ -f crates/perry-codegen/tests/loop_safepoint_purity.rs ]; then
wc -l crates/perry-codegen/tests/loop_safepoint_purity.rs
sed -n '1,220p' crates/perry-codegen/tests/loop_safepoint_purity.rs
else
fd -a 'loop_safepoint_purity.rs' .
fi
echo
echo "== perry-codegen Cargo metadata =="
fd -a 'Cargo.toml' crates/perry-codegen . | while read -r f; do
echo "--- $f"
sed -n '1,180p' "$f"
done || true
echo
echo "== std::env::set_var usages in tests/codegen =="
rg -n 'std::env::set_var|set_var\(' crates/perry-codegen crates/perry || true
echo
echo "== loop_polls variable usages =="
rg -n 'PERRY_GC_MOVING_LOOP_POLLS|loop_polls|back_edge_polls|OnceLock|Once' crates/perry-codegen crates/perry || trueRepository: PerryTS/perry Length of output: 50370 🌐 Web query:
💡 Result: In the Rust 2024 Edition, Citations:
Serialize the process-wide environment setup. Multiple tests in this binary can call 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| fn ir_for(name: &str, init: Vec<Stmt>) -> String { | ||
| enable_back_edge_polls(); | ||
| String::from_utf8(compile_module(&module_with_init(name, init), entry_opts()).unwrap()) | ||
| .expect("LLVM IR should be UTF-8") | ||
| } | ||
|
|
||
| /// Same, but `exported` names are exported module variables — which is what | ||
| /// promotes a module-level `let` to a `@perry_global_*` slot. | ||
| fn ir_for_with_exported_vars(name: &str, init: Vec<Stmt>, exported: &[&str]) -> String { | ||
| enable_back_edge_polls(); | ||
| let mut m = module_with_init(name, init); | ||
| m.exported_objects = exported.iter().map(|s| s.to_string()).collect(); | ||
| String::from_utf8(compile_module(&m, entry_opts()).unwrap()).expect("LLVM IR should be UTF-8") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 143
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 1383
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 39474
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 143
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 143
Correct the duplicated per-PR CI coverage claim.
Current in-repo guidance says integration-like test workflows include a PR-only scoped job, so these docs still say the suites are nightly/tag only. Update both
crates/perry-codegen/tests/loop_safepoint_purity.rs#L133-138andchangelog.d/7320-loop-safepoint-purity-knob.md#L16-18to remove that duplicated incorrect claim or to specify the exact historical CI state/exclusion that hid the regression.📍 Affects 2 files
crates/perry-codegen/tests/loop_safepoint_purity.rs#L134-L138(this comment)changelog.d/7320-loop-safepoint-purity-knob.md#L16-L18🤖 Prompt for AI Agents
Source: Learnings