Fix coroutine lifetime and realloc fragmentation#1999
Merged
nicolasnoble merged 7 commits intogrumpycoders:mainfrom Apr 8, 2026
Merged
Fix coroutine lifetime and realloc fragmentation#1999nicolasnoble merged 7 commits intogrumpycoders:mainfrom
nicolasnoble merged 7 commits intogrumpycoders:mainfrom
Conversation
The merge check compared block + size instead of block + old_size, making it dead code. Leftover fragments were never merged with adjacent free blocks. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Transfers the coroutine handle to a separate ChainAwaiter that GCC reliably promotes to the coroutine frame, working around known temporary lifetime bugs in GCC 10-15+. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces rvalue-only coroutine chaining via Changes
Sequence DiagramsequenceDiagram
participant Awaiting as Awaiting Coroutine
participant Source as Source Coroutine
participant Chain as ChainAwaiter
participant Promise as Promise<T>
Awaiting->>Source: co_await(source) (rvalue)
Source->>Chain: operator co_await() && → ChainAwaiter
Chain->>Chain: await_ready() checks source done / early-resume
alt source not done
Chain->>Promise: await_suspend(awaiting_handle) stores m_awaitingCoroutine
Chain->>Source: optionally resume(source_handle) if not done
Source->>Promise: run to completion → final_suspend
Promise->>Awaiting: resume stored m_awaitingCoroutine
end
Awaiting->>Chain: await_resume()
Chain->>Source: destroy source handle
alt value coroutine
Chain->>Awaiting: move and return stored value
else void coroutine
Chain->>Awaiting: return nothing
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mips/psyqo/src/alloc.c (1)
520-523:⚠️ Potential issue | 🟠 MajorShrink adjacency fix is still inconsistent in one branch.
Line 588 is corrected, but Line 522 still compares against
block + size(new free block start). For shrink adjacency, it should compare againstblock + old_size(original allocation end), otherwise theblock < headpath can still skip merges and leave fragmentation.Proposed fix
- if (head == (empty_block *)((char *)block + size)) { + if ((char *)block + old_size == (char *)head) {Also applies to: 587-588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/psyqo/src/alloc.c` around lines 520 - 523, The shrink adjacency check wrongly compares the new free-block start (block + size) instead of the original allocation end, so merges can be skipped when shrinking; update the comparison in the branch that handles shrink-to-smaller (the code creating empty_block *new_block and checking if (head == (empty_block *)((char *)block + size))) to use the original end pointer (block + old_size) instead of block + size, and apply the same correction to the symmetric check around the other branch (the block < head path) so both merge conditions use old_size and thus properly coalesce the newly freed space with head.
🧹 Nitpick comments (1)
src/mips/psyqo/coroutine.hh (1)
248-251: Minor: Redundanthandle.done()check.Since
await_ready()returningfalseis a precondition forawait_suspend()being called, theif (!handle.done())guard at line 250 is always true. This is harmless defensive code, so no change required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/psyqo/coroutine.hh` around lines 248 - 251, Redundant guard in await_suspend: the check `if (!handle.done())` is unnecessary because await_suspend is only called when await_ready() returned false, so remove the `if (!handle.done())` conditional inside the await_suspend(std::coroutine_handle<> h) implementation and call handle.resume() unconditionally; update the await_suspend method (referencing await_suspend, handle, and await_ready) to always resume the coroutine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/support/coroutine.h`:
- Around line 132-135: The template Yield hook yield_value currently discards
the passed awaiter and returns Awaiter(nullptr), causing null-pointer deref in
Awaiter::await_ready (m_coroutine == nullptr); modify yield_value(From &&from)
in coroutine.h to forward the incoming awaiter instead of constructing
Awaiter(nullptr) so the original awaiter instance (the one providing
m_coroutine/m_earlyResume) is returned; locate the yield_value template and
replace the return with a forwarded return of the provided object (e.g., return
std::forward<From>(from) or construct an Awaiter from the forwarded argument) so
await_ready() operates on the real awaiter.
- Around line 153-156: The template yield_value(From &&from) currently returns
Awaiter(nullptr) which drops the provided awaiter; modify
Promise/Coroutine::yield_value to forward the incoming awaiter instead of
passing nullptr by returning the forwarded argument (use
std::forward<From>(from) or otherwise construct/convert to Awaiter from the
forwarded From), and ensure <utility> is included if needed so the Awaiter
returned reflects the actual awaitable passed into yield_value.
---
Outside diff comments:
In `@src/mips/psyqo/src/alloc.c`:
- Around line 520-523: The shrink adjacency check wrongly compares the new
free-block start (block + size) instead of the original allocation end, so
merges can be skipped when shrinking; update the comparison in the branch that
handles shrink-to-smaller (the code creating empty_block *new_block and checking
if (head == (empty_block *)((char *)block + size))) to use the original end
pointer (block + old_size) instead of block + size, and apply the same
correction to the symmetric check around the other branch (the block < head
path) so both merge conditions use old_size and thus properly coalesce the newly
freed space with head.
---
Nitpick comments:
In `@src/mips/psyqo/coroutine.hh`:
- Around line 248-251: Redundant guard in await_suspend: the check `if
(!handle.done())` is unnecessary because await_suspend is only called when
await_ready() returned false, so remove the `if (!handle.done())` conditional
inside the await_suspend(std::coroutine_handle<> h) implementation and call
handle.resume() unconditionally; update the await_suspend method (referencing
await_suspend, handle, and await_ready) to always resume the coroutine.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f5c55ff-e0a1-4d8e-b424-740bdc317352
📒 Files selected for processing (3)
src/mips/psyqo/coroutine.hhsrc/mips/psyqo/src/alloc.csrc/support/coroutine.h
Brings the host-side coroutine in line with psyqo's fixes: proper move semantics, destructor, awaiting coroutine resumption on co_return, and operator co_await for safe chaining. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
nicolasnoble
force-pushed
the
fix-coroutine-lifetime
branch
from
14125f6 to
c487ff4
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses two runtime correctness areas in the codebase: heap fragmentation during realloc shrinking in PSYQo’s allocator, and coroutine lifetime/ownership issues to support reliable coroutine chaining (notably as a workaround for GCC coroutine temporary lifetime problems).
Changes:
- Fix
psyqo_realloc()shrink-path adjacency detection to correctly merge newly freed tail space with the next free block. - Add
operator co_await() &&-based coroutine chaining support (handle-transfer awaiter) to work around GCC coroutine temporary lifetime issues. - Align
PCSX::Coroutinesemantics with PSYQo’s approach (explicit move/destruction behavior and chaining support).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/support/coroutine.h |
Adds chaining via operator co_await() and adjusts lifetime/move semantics for PCSX::Coroutine. |
src/mips/psyqo/src/alloc.c |
Fixes shrink adjacency check to prevent unmerged free fragments on realloc shrink. |
src/mips/psyqo/coroutine.hh |
Introduces ChainAwaiter + operator co_await() && for coroutine-to-coroutine chaining in PSYQo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Same bug as psyqo's allocator: merge checks compared block + size instead of block + old_size, affecting both the block < head and general shrink paths. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Prevents handle leak if the awaiting coroutine is destroyed while suspended. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/support/coroutine.h`:
- Around line 130-140: The coroutine currently resumes its awaiting continuation
inside return_void()/return_value(), which can cause the child to be destroyed
while still running; move the continuation handoff into final_suspend() instead:
remove any m_awaitingCoroutine.resume()/reset logic from return_void() and
return_value() and implement final_suspend() to, when m_awaitingCoroutine is
set, resume that coroutine and then clear m_awaitingCoroutine; this ensures the
parent is resumed only after the child has reached final suspension (addresses
ChainAwaiter::await_resume() calling handle.destroy()).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddfef967-1868-41d0-a138-89a7c27ae262
📒 Files selected for processing (2)
src/mips/psyqo/coroutine.hhsrc/support/coroutine.h
Resuming the awaiting coroutine from return_void/return_value meant destroy() could be called on the inner coroutine before it reached final_suspend, which is UB. Move the handoff to final_suspend's await_suspend using symmetric transfer. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/support/coroutine.h`:
- Around line 92-97: The awaiter() method must be made lvalue-only to avoid
returning an Awaiter that holds a pointer to a temporary; change the awaiter()
member so it only participates for lvalues (e.g., add an & qualifier on
Coroutine::awaiter or explicitly delete the rvalue overload) so Awaiter(this)
can only be created from a persistent Coroutine object; update the declaration
for awaiter() (and keep Awaiter and Coroutine names intact) to enforce this at
compile time.
- Around line 55-67: The move-assignment operator (Coroutine::operator=) is not
exception-safe because it destroys the current m_handle before completing moves,
which can leave both objects owning the same handle if a later operation throws;
fix it by first atomically preparing the new state: capture other.m_handle into
a local (e.g., new_handle), move other.m_value into a local (new_value) and copy
other.m_suspended and other.m_earlyResume, then destroy the current m_handle and
assign the locals into this->m_handle, m_value, m_suspended, m_earlyResume, and
finally reset other.* to defaults; reference members m_handle, m_value,
m_suspended, m_earlyResume and the operator method name operator= to locate and
implement this change.
- Around line 200-215: The awaiter methods of ChainAwaiter must guard against a
null handle: update await_ready(), await_suspend(std::coroutine_handle<>), and
await_resume() to first check if (handle) before calling handle.done(),
handle.promise(), handle.resume(), or handle.destroy(); implement await_ready()
to return true when handle is null, make await_suspend() do nothing (and not
access handle) when handle is null, and in await_resume() only move the promise
value and call handle.destroy() when handle is valid—otherwise return (for void)
or return a default-constructed T for non-void paths; reference the ChainAwaiter
type and its await_ready/await_suspend/await_resume methods when making the
changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 345bba31-90ca-4884-a791-dc4ca4242dbc
📒 Files selected for processing (1)
src/support/coroutine.h
Fix exception safety in move operations by transferring handle ownership before the potentially-throwing value move. Restrict awaiter() to lvalues in both PCSX and psyqo coroutines to prevent dangling pointer from temporaries. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
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.
operator co_await()topsyqo::Coroutineto work around GCC 10-15+ coroutine temporary lifetime bugsPCSX::Coroutinein line with psyqo's fixes: proper move semantics, destructor, chaining support