Skip to content

test(gc): assert the poll guard's CFG shape, not its text order - #8126

Merged
proggeramlug merged 2 commits into
mainfrom
fix/loop-poll-guard-test-cfg
Aug 15, 2026
Merged

test(gc): assert the poll guard's CFG shape, not its text order#8126
proggeramlug merged 2 commits into
mainfrom
fix/loop-poll-guard-test-cfg

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

a_surviving_poll_is_guarded_by_the_arming_word fails on main, but the lowering is correct — every load volatile @PERRY_GC_POLL_ARMED is followed by icmp + br i1 into a gcpoll.N block holding the call.

The test's proxy for that property broke. It searched the whole remainder of the IR after each load and required the first br i1 to precede the first poll call — silently assuming each poll block is printed next to its guard. It isn't: guards sit inline in for.cond/for.body/for.update, while the gcpoll.N blocks are emitted together after the loop. So the first two segments contain no call, find(POLL) returned None, and the matches! arm rejected a correctly-guarded poll.

Now the search is bounded at the load's own basic block: that block must end in a conditional branch and must not contain the call. Same property, no dependence on block print order.

Teeth preserved: all six negative controls (coercible bound, coercible accumulator, call in body, object literal in body, module-global accumulator, string concat) still emit a poll and still run through this check — they pass.

Note this does not make e2e-scoped green on its own: loop_safepoint_purity has one other failure (proven_numeric_counted_loop_emits_no_back_edge_poll) which is a genuine regression — the loop lowers to js_dynamic_string_or_number_add/js_rel_lt, so the numeric proof is not applying — and that is tracked separately from this test-shape fix.

Summary by CodeRabbit

  • Bug Fixes

    • Improved compiler validation for loop safepoints by checking control-flow behavior directly, increasing reliability across different generated code layouts.
    • Preserved existing negative-case coverage to help prevent regressions in safepoint handling.
  • Tests

    • Updated guarded-poll validation to better reflect actual control-flow structure and avoid relying on generated output ordering.

`a_surviving_poll_is_guarded_by_the_arming_word` fails on main. The lowering is
correct: every `load volatile @PERRY_GC_POLL_ARMED` is followed by `icmp` and
`br i1` into a `gcpoll.N` block that holds the call, which then branches to
`gcpoll.done.N`. The safety property the test names — the call sits behind the
branch, not in the same straight line as the load — holds.

What broke is the test's proxy for it. It searched the whole remainder of the
IR after each load and required the first `br i1` to precede the first poll
call, which silently assumed each poll block is printed next to its guard. It
is not: the guards sit inline in `for.cond` / `for.body` / `for.update`, while
every `gcpoll.N` block is emitted together after the loop. Under that layout
the first two segments contain no call at all, so `after.find(POLL)` returned
`None` and the `matches!` arm — which requires `Some(c)` — rejected a
correctly-guarded poll.

Bound the search at the load's own basic block instead: that block must end in
a conditional branch and must not contain the call. This asserts the actual
CFG property and does not care where the successor block is printed.

The negative controls still pass, so the test keeps its teeth: the loops that
must KEEP a poll (coercible bound, coercible accumulator, call in the body,
object literal in the body, module-global accumulator, string concat) all
still emit one and are still checked through this same code.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c4fc8937-7ea6-45d5-a48b-8fe80960b79e

📥 Commits

Reviewing files that changed from the base of the PR and between 83b6b8c and 2c4ddc9.

📒 Files selected for processing (2)
  • changelog.d/8126-loop-poll-guard-cfg-assert.md
  • crates/perry-codegen/tests/loop_safepoint_purity.rs

📝 Walkthrough

Walkthrough

The loop safepoint purity assertion now validates each poll load within its basic block. It requires a conditional branch and excludes the poll call from that block. Negative controls remain unchanged.

Changes

Loop poll CFG assertion

Layer / File(s) Summary
Basic-block-local guard validation
crates/perry-codegen/tests/loop_safepoint_purity.rs, changelog.d/8126-loop-poll-guard-cfg-assert.md
The test replaces textual IR ordering checks with basic-block-local checks for a conditional branch and no poll call. The changelog records this update.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/loop-poll-guard-test-cfg

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug
proggeramlug marked this pull request as ready for review August 15, 2026 05:29
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Verified on a landing-equivalent tree (merged current main 83b6b8c69):

test a_surviving_poll_is_guarded_by_the_arming_word ... ok      <- was failing
test result: FAILED. 7 passed; 1 failed
             ^ proven_numeric_counted_loop_emits_no_back_edge_poll, a DIFFERENT
               pre-existing failure this PR does not touch

So this fixes exactly one of the eleven and leaves the other alone, which is the right blast radius.

The important consequence: one of the eleven was never a real failure. I have been using "27 suites / 1434 passed / 11 failed" as the codegen baseline all session, and treating those eleven as genuine pre-existing breakage. At least one was a broken test proxy — the lowering was correct the whole time. Worth re-examining the other ten in that light before anyone bisects them.

The diagnosis is right and worth restating: the old check searched the whole remainder of the IR after each load volatile @PERRY_GC_POLL_ARMED and required the first br i1 to precede the first poll call. That silently assumed each gcpoll.N block is printed next to its guard. It is not — guards sit inline in for.cond/for.body/for.update while the poll blocks are emitted together after the loop, so the first two segments contain no call at all and find(POLL) returned None.

Bounding the search at the load's own basic block asserts the actual CFG property — this block ends in a conditional branch and does not contain the call — with no dependence on print order. That is a strictly better assertion, not a weakened one.

@proggeramlug
proggeramlug merged commit 800def1 into main Aug 15, 2026
18 of 22 checks passed
@proggeramlug
proggeramlug deleted the fix/loop-poll-guard-test-cfg branch August 15, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant