Skip to content

Comments

Add missing clobbers to coroutine context switch assembly#329

Merged
lalinsky merged 1 commit intomainfrom
add-missing-clobbers
Feb 19, 2026
Merged

Add missing clobbers to coroutine context switch assembly#329
lalinsky merged 1 commit intomainfrom
add-missing-clobbers

Conversation

@lalinsky
Copy link
Owner

Summary

  • Add condition flag and FP status register clobbers that were missing from the inline assembly context switch
  • x86_64: cc, st0-st7
  • aarch64: nzcv
  • arm/thumb: cpsr
  • riscv64/riscv32: fflags, frm
  • loongarch64: fcc0-fcc7, fcsr0-fcsr3
  • powerpc64 was already complete

Test plan

  • ./check.sh --filter "Coroutine" passes (all 12 tests)

Add condition flag and FP status register clobbers that were missing:
- x86_64: cc, st0-st7
- aarch64: nzcv
- arm/thumb: cpsr
- riscv64/riscv32: fflags, frm
- loongarch64: fcc0-fcc7, fcsr0-fcsr3
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 19, 2026

📝 Walkthrough

Walkthrough

Expanded inline assembly context-switching code in src/coro/coroutines.zig to preserve additional processor and floating-point state across multiple architectures (x86_64, aarch64, arm, riscv, powerpc) by adding flags and registers to clobber sets and corresponding restoration logic.

Changes

Cohort / File(s) Summary
Coroutine Context-Switch State Preservation
src/coro/coroutines.zig
Extended inline-asm clobber sets and restoration paths for x86_64 (.cc, .st0–.st7, .dirflag), aarch64 (.nzcv), arm (.cpsr), riscv64/riscv32 (.fflags, .frm), and powerpc (expanded FP/status registers). Control flow unchanged; only expanded CPU/FP state coverage.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

✨ Across the silicon landscape wide,
State gets tucked and pushed aside,
Flags and registers find their place,
From x86 to RISC's embrace—
One switch, all cores remember the ride. 🔄

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add missing clobbers to coroutine context switch assembly' directly and specifically describes the main change in the changeset.
Description check ✅ Passed The description details the specific clobbers added across multiple architectures and includes a test plan demonstrating the changes work correctly.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-missing-clobbers

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 and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/coro/coroutines.zig (1)

240-253: x86_64: .cc alongside .rflags — potentially redundant but harmless.

Line 243 already declares .rflags = true, and line 244 adds .cc = true. In LLVM's model, cc is the canonical condition-code clobber and typically maps to the same flags register. Having both declared shouldn't cause problems — the compiler just gets told twice that flags are toast. The st0st7 additions are spot-on; x87 FPU state isn't saved/restored across the switch, so clobbering them is necessary.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/coro/coroutines.zig` around lines 240 - 253, Remove the redundant ".cc =
true" entry from the clobber list so only ".rflags = true" declares the
condition-code/flags clobber; keep the x87 st0–st7 entries as-is. Locate the
clobber structure containing ".rflags = true" and ".cc = true" in
src/coro/coroutines.zig and delete the ".cc = true" line to avoid the duplicate
flags declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/coro/coroutines.zig`:
- Around line 240-253: Remove the redundant ".cc = true" entry from the clobber
list so only ".rflags = true" declares the condition-code/flags clobber; keep
the x87 st0–st7 entries as-is. Locate the clobber structure containing ".rflags
= true" and ".cc = true" in src/coro/coroutines.zig and delete the ".cc = true"
line to avoid the duplicate flags declaration.

@lalinsky lalinsky merged commit 502805d into main Feb 19, 2026
18 checks passed
@lalinsky lalinsky deleted the add-missing-clobbers branch February 19, 2026 20:12
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