Adding ucontext.#1934
Conversation
|
""" WalkthroughA minimal user-level context switching API and its MIPS assembly implementation were introduced for the PlayStation (PSX) environment. A stackful coroutine system using this API was added to the PsyQO codebase, along with a new example demonstrating coroutine usage with timed delays and GPU text rendering. The build system was updated to include the new assembly source. Changes
Sequence Diagram(s)sequenceDiagram
participant MainApp
participant CoroutineContext
participant TimerCallback
MainApp->>CoroutineContext: initialize coroutine with function
MainApp->>CoroutineContext: resume()
CoroutineContext->>MainApp: yield() (after setting timer)
TimerCallback->>CoroutineContext: resume() (on timer expiry)
CoroutineContext->>MainApp: yield() (after next timer)
Note right of CoroutineContext: Repeats until coroutine finishes
sequenceDiagram
participant Caller
participant ContextA
participant ContextB
Caller->>ContextA: getcontext(&ContextA)
Caller->>ContextB: makecontext(&ContextB, func, arg)
Caller->>ContextA: swapcontext(&ContextA, &ContextB)
ContextB->>ContextA: swapcontext(&ContextB, &ContextA) (when yielding back)
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new ucontext implementation for the PSX environment.
- Introduces ucontext assembly routines for saving and restoring execution context.
- Provides a corresponding header file with structure definitions and function prototypes.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mips/common/psxlibc/ucontext.s | Adds assembly routines for getcontext, setcontext, makecontext, and swapcontext. |
| src/mips/common/psxlibc/ucontext.h | Provides header declarations and structure definitions for ucontext management. |
| sw $a1, FUNCPTR($a0) | ||
| sw $a2, ARGPTR($a0) | ||
| sw $a0, S0PTR($a0) | ||
| addu $v1, $t0 |
There was a problem hiding this comment.
The addu instruction appears to be missing a second source register. If the intended operation is $v1 = $v1 + $t0, please update the line to 'addu $v1, $v1, $t0' to ensure correct addition.
| addu $v1, $t0 | |
| addu $v1, $v1, $t0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/common/psxlibc/ucontext.h(1 hunks)src/mips/common/psxlibc/ucontext.s(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
🔇 Additional comments (19)
src/mips/common/psxlibc/ucontext.h (6)
51-54: LGTM! Function prototypes follow POSIX conventions.The function signatures are well-designed and follow standard ucontext API conventions. The use of
constfor read-only parameters is appropriate.
36-49:Details
✅ Verification successful
Verify structure layout compatibility with assembly implementation.
The structure definitions look well-designed for a minimal ucontext implementation. However, the assembly file uses offsets that extend beyond the 64-byte
mcontext_t.reserved[16]array to store additional context data like function pointers and arguments.Let me verify the structure layout matches the assembly expectations:
🏁 Script executed:
#!/bin/bash # Description: Verify the structure offsets used in assembly match the header definitions # Expected: Assembly offsets should align with the C structure layout echo "Checking assembly offset definitions..." rg -A 20 "\.set.*PTR," src/mips/common/psxlibc/ucontext.s echo -e "\nChecking structure size assumptions..." rg -B 5 -A 10 "struct.*context_t" src/mips/common/psxlibc/ucontext.hLength of output: 1512
Verified:
ucontext_tlayout matches assembly offsetsThe C definitions line up exactly with the
.setdirectives inucontext.s:
mcontext_t.reserved[16](16×4 = 64 bytes) covers RAPTR…UNUSED2, including FUNCPTR (48) and ARGPTR (52).uc_linklives at offset 64 (LINKPTR).uc_stack.ss_spat offset 68 (STACKPTR) anduc_stack.ss_sizeat 72 (STACKSIZE).No changes required.
36-49: Well-designed minimal ucontext structures.The structure definitions provide a clean, minimal implementation suitable for PSX context switching. The layout aligns well with the assembly implementation offsets.
51-54: Function prototypes match standard ucontext API.The function signatures are consistent with POSIX ucontext API conventions, making this a good drop-in replacement for PSX environments.
51-54: LGTM: Function prototypes are correctly defined.The function signatures match the standard ucontext API conventions and provide the necessary interface for context switching operations.
41-49:Details
✅ Verification successful
Verify structure layout consistency with assembly implementation.
The
mcontext_tstructure defines 16 uint32_t values (64 bytes), but the assembly implementation uses offsets that extend beyond this range and appear to access the entireucontext_tstructure. Ensure the assembly offsets align with the intended structure layout.Run the following script to verify the structure layout assumptions:
🏁 Script executed:
#!/bin/bash # Verify that assembly offsets match structure layout expectations echo "Checking assembly offset definitions..." rg -A 20 "\.set.*PTR.*," src/mips/common/psxlibc/ucontext.s echo "Checking structure definitions..." rg -A 10 "struct.*\{" src/mips/common/psxlibc/ucontext.hLength of output: 1486
Assembly Offsets and Structure Layout Are Consistent
The offsets defined in
ucontext.salign exactly with the fields inucontext.h:
- RAPTR…UNUSED2 (0–60) cover the 16×
uint32_tinmcontext_tLINKPTR = 64→uc_linkSTACKPTR = 68→uc_stack.ss_spSTACKSIZE = 72→uc_stack.ss_sizeNo changes required.
src/mips/common/psxlibc/ucontext.s (13)
27-46: LGTM! Well-organized symbolic offset definitions.The symbolic offsets provide excellent clarity and maintainability for the assembly code. The layout efficiently uses the available space in the context structure.
52-67: LGTM! Correct getcontext implementation.The function properly saves all callee-saved registers and returns 0 as expected for the initial return from getcontext.
73-87: LGTM! Correct setcontext implementation.The function properly restores all saved registers and jumps to the saved return address. The return value of 0 is appropriate for context restoration.
130-157: LGTM! Correct swapcontext implementation.The function properly saves the current context and restores the target context in the correct order. The implementation follows the expected swapcontext semantics.
27-46: Offset definitions correctly align with C structure layout.The symbolic offsets properly match the memory layout of the
ucontext_tstructure defined in the header file.
52-67: getcontext implementation is correct.Properly saves all callee-saved registers and returns 0 as expected for the initial context capture.
73-87: setcontext implementation is correct.Correctly restores all saved registers and jumps to the saved return address.
130-156: swapcontext implementation is correct.Properly saves the current context and restores the target context in a single atomic operation.
113-124:⚠️ Potential issueStack pointer handling issue in makecontext.
The stack pointer calculation and storage has problems: the code adds the stack size to the base (wrong direction for typical stack growth) and stores the result back to
STACKPTRinstead ofSPPTR.Fix the stack handling:
makecontext: lw $v1, STACKPTR($a0) lw $t0, STACKSIZE($a0) la $v0, contexttrampoline sw $v0, RAPTR($a0) sw $a1, FUNCPTR($a0) sw $a2, ARGPTR($a0) sw $a0, S0PTR($a0) - addu $v1, $t0 - sw $v1, STACKPTR($a0) + addu $v1, $v1, $t0 + sw $v1, SPPTR($a0) jr $raLikely an incorrect or invalid review comment.
27-46: LGTM: Well-organized symbolic offset definitions.The symbolic offsets provide clear, maintainable definitions for the context structure layout. This approach improves code readability and reduces magic numbers.
52-67: LGTM: Correct implementation of getcontext.The function properly saves all callee-saved registers and the return address, following MIPS calling conventions. The return value of 0 is correct for successful context capture.
73-87: LGTM: Correct implementation of setcontext.The function properly restores all previously saved registers and returns to the saved execution point. The implementation correctly complements the getcontext function.
130-157: LGTM: Correct implementation of swapcontext.The function correctly saves the current context to the first argument and restores the new context from the second argument. This atomic swap operation is properly implemented following MIPS calling conventions.
| contexttrampoline: | ||
| lw $a0, ARGPTR($s0) | ||
| lw $a1, FUNCPTR($s0) | ||
| lw $sp, SPPTR($s0) | ||
| lw $s8, S8PTR($s0) | ||
| lw $s0, S0PTR($s0) | ||
| lw $s1, S1PTR($s0) | ||
| lw $s2, S2PTR($s0) | ||
| lw $s3, S3PTR($s0) | ||
| lw $s4, S4PTR($s0) | ||
| lw $s5, S5PTR($s0) | ||
| lw $s6, S6PTR($s0) | ||
| lw $s7, S7PTR($s0) | ||
| lw $gp, GPPTR($s0) | ||
| jalr $a1 | ||
| lw $a0, LINKPTR($s0) | ||
| j setcontext | ||
|
|
There was a problem hiding this comment.
Fix register corruption in contexttrampoline.
There's a critical bug in the register restoration sequence. Line 100 loads S0PTR($s0) into $s0, but $s0 is being used as the base address for all loads. This will corrupt subsequent loads.
contexttrampoline:
lw $a0, ARGPTR($s0)
lw $a1, FUNCPTR($s0)
lw $sp, SPPTR($s0)
lw $s8, S8PTR($s0)
- lw $s0, S0PTR($s0)
lw $s1, S1PTR($s0)
lw $s2, S2PTR($s0)
lw $s3, S3PTR($s0)
lw $s4, S4PTR($s0)
lw $s5, S5PTR($s0)
lw $s6, S6PTR($s0)
lw $s7, S7PTR($s0)
lw $gp, GPPTR($s0)
+ lw $s0, S0PTR($s0)
jalr $a1
lw $a0, LINKPTR($s0)
j setcontext📝 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.
| contexttrampoline: | |
| lw $a0, ARGPTR($s0) | |
| lw $a1, FUNCPTR($s0) | |
| lw $sp, SPPTR($s0) | |
| lw $s8, S8PTR($s0) | |
| lw $s0, S0PTR($s0) | |
| lw $s1, S1PTR($s0) | |
| lw $s2, S2PTR($s0) | |
| lw $s3, S3PTR($s0) | |
| lw $s4, S4PTR($s0) | |
| lw $s5, S5PTR($s0) | |
| lw $s6, S6PTR($s0) | |
| lw $s7, S7PTR($s0) | |
| lw $gp, GPPTR($s0) | |
| jalr $a1 | |
| lw $a0, LINKPTR($s0) | |
| j setcontext | |
| contexttrampoline: | |
| lw $a0, ARGPTR($s0) | |
| lw $a1, FUNCPTR($s0) | |
| lw $sp, SPPTR($s0) | |
| lw $s8, S8PTR($s0) | |
| lw $s1, S1PTR($s0) | |
| lw $s2, S2PTR($s0) | |
| lw $s3, S3PTR($s0) | |
| lw $s4, S4PTR($s0) | |
| lw $s5, S5PTR($s0) | |
| lw $s6, S6PTR($s0) | |
| lw $s7, S7PTR($s0) | |
| lw $gp, GPPTR($s0) | |
| lw $s0, S0PTR($s0) | |
| jalr $a1 | |
| lw $a0, LINKPTR($s0) | |
| j setcontext |
🤖 Prompt for AI Agents
In src/mips/common/psxlibc/ucontext.s lines 95 to 112, the code loads $s0 from
S0PTR($s0) while $s0 is used as the base pointer for all loads, causing register
corruption. To fix this, first load all registers except $s0 using $s0 as the
base, then load $s0 last or use a temporary register to hold the base address
before loading $s0, ensuring the base pointer remains unchanged during the
sequence.
Critical register dependency issue in contexttrampoline.
The function has a serious register dependency bug where $s0 is used as a base register for loads, but then $s0 itself is overwritten on line 100, corrupting subsequent loads.
Fix the register dependency issue:
contexttrampoline:
lw $a0, ARGPTR($s0)
lw $a1, FUNCPTR($s0)
+ lw $t0, LINKPTR($s0)
lw $sp, SPPTR($s0)
lw $s8, S8PTR($s0)
lw $s0, S0PTR($s0)
lw $s1, S1PTR($s0)
lw $s2, S2PTR($s0)
lw $s3, S3PTR($s0)
lw $s4, S4PTR($s0)
lw $s5, S5PTR($s0)
lw $s6, S6PTR($s0)
lw $s7, S7PTR($s0)
lw $gp, GPPTR($s0)
jalr $a1
- lw $a0, LINKPTR($s0)
+ move $a0, $t0
j setcontextCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/mips/common/psxlibc/ucontext.s lines 95 to 112, the code uses $s0 as the
base register for multiple lw instructions but overwrites $s0 on line 100 before
all loads are complete, causing corrupted data loads. To fix this, first load
all needed values using $s0 as the base into temporary registers or reorder the
loads so that $s0 is not overwritten until all dependent loads are done. Ensure
$s0 remains unchanged while it is used as the base for all lw instructions.
Critical: Fix delay slot ordering in contexttrampoline.
The instruction on line 110 (lw $a0, LINKPTR($s0)) is in the delay slot of the jalr instruction and will execute before the function call, corrupting the function argument.
Fix the delay slot issue:
- jalr $a1
- lw $a0, LINKPTR($s0)
+ jalr $a1
+ nop
+ lw $a0, LINKPTR($s0)📝 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.
| contexttrampoline: | |
| lw $a0, ARGPTR($s0) | |
| lw $a1, FUNCPTR($s0) | |
| lw $sp, SPPTR($s0) | |
| lw $s8, S8PTR($s0) | |
| lw $s0, S0PTR($s0) | |
| lw $s1, S1PTR($s0) | |
| lw $s2, S2PTR($s0) | |
| lw $s3, S3PTR($s0) | |
| lw $s4, S4PTR($s0) | |
| lw $s5, S5PTR($s0) | |
| lw $s6, S6PTR($s0) | |
| lw $s7, S7PTR($s0) | |
| lw $gp, GPPTR($s0) | |
| jalr $a1 | |
| lw $a0, LINKPTR($s0) | |
| j setcontext | |
| contexttrampoline: | |
| lw $a0, ARGPTR($s0) | |
| lw $a1, FUNCPTR($s0) | |
| lw $sp, SPPTR($s0) | |
| lw $s8, S8PTR($s0) | |
| lw $s0, S0PTR($s0) | |
| lw $s1, S1PTR($s0) | |
| lw $s2, S2PTR($s0) | |
| lw $s3, S3PTR($s0) | |
| lw $s4, S4PTR($s0) | |
| lw $s5, S5PTR($s0) | |
| lw $s6, S6PTR($s0) | |
| lw $s7, S7PTR($s0) | |
| lw $gp, GPPTR($s0) | |
| jalr $a1 | |
| nop | |
| lw $a0, LINKPTR($s0) | |
| j setcontext |
🤖 Prompt for AI Agents
In src/mips/common/psxlibc/ucontext.s between lines 95 and 112, the instruction
'lw $a0, LINKPTR($s0)' is incorrectly placed in the delay slot of the 'jalr $a1'
instruction, causing it to execute before the function call and corrupt the
argument. To fix this, move the 'lw $a0, LINKPTR($s0)' instruction so it
executes after the 'jalr' call, ensuring the delay slot contains a safe
instruction or a nop, preserving correct argument passing.
nicolasnoble
force-pushed
the
ucontext
branch
2 times, most recently
from
fb70401 to
e19d916
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1934 +/- ##
=======================================
Coverage 9.30% 9.30%
=======================================
Files 469 469
Lines 145099 145099
=======================================
Hits 13508 13508
Misses 131591 131591 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/mips/psyqo/coroutine.hh (1)
214-217: Consider adding error handling to the public interface.The current interface doesn't provide feedback on initialization or operation failures. Consider whether error reporting is needed for this low-level API.
If error handling is desired, consider:
- void initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size); + bool initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size); - void resume(); + bool resume();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mips/psyqo/Makefile(1 hunks)src/mips/psyqo/coroutine.hh(2 hunks)src/mips/psyqo/examples/coroutine-demo-3/Makefile(1 hunks)src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/mips/psyqo/examples/coroutine-demo-3/Makefile
- src/mips/psyqo/Makefile
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: aur-build
🔇 Additional comments (9)
src/mips/psyqo/coroutine.hh (4)
32-32: Good addition of ucontext dependency.The include for
"common/psxlibc/ucontext.h"is appropriately placed and necessary for the new stackful coroutine functionality.
224-227: Static trampoline pattern implemented correctly.The static trampoline function properly bridges C-style callbacks to C++ member functions, which is essential for the ucontext makecontext integration.
248-251: Stack size alignment calculation is correct.The compile-time calculation
(StackSize + 7) & ~7properly ensures 8-byte alignment of the stack, which is important for MIPS architecture requirements.
299-302: Stack structure alignment looks good.The
alignas(8)directive on the Stack struct ensures proper alignment, and the stack buffer sizing uses the aligned constant correctly.src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp (5)
61-68: GPU configuration setup looks appropriate.The configuration choices (320x240, auto video mode, 15-bit color, progressive scan) are reasonable for a PSX demo application.
75-79: Coroutine initialization and startup handled correctly.Good practice to assert the start reason and immediately resume the coroutine after initialization to begin execution.
52-52: Stack size selection is appropriate.The 65536-byte (64KB) stack size should be sufficient for the simple coroutine logic in this demo, and matches the default stack size in the template.
84-88: Timer and yield pattern correctly implemented.The pattern of setting a timer with a lambda callback that updates text and resumes the coroutine, followed by yielding, is the correct cooperative multitasking approach.
Also applies to: 90-94, 96-100
108-113: Frame rendering implementation is clean.Simple and effective rendering that clears the background and displays the current text state.
| class StackfulBase { | ||
| protected: | ||
| void initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size); | ||
| void resume(); | ||
| void yield(); | ||
| [[nodiscard]] bool isAlive() const { return m_isAlive; } | ||
|
|
||
| StackfulBase() = default; | ||
| StackfulBase(const StackfulBase&) = delete; | ||
| StackfulBase& operator=(const StackfulBase&) = delete; | ||
|
|
||
| private: | ||
| static void trampoline(void* arg) { | ||
| StackfulBase* self = static_cast<StackfulBase*>(arg); | ||
| self->trampoline(); | ||
| } | ||
| void trampoline(); | ||
| ucontext_t m_coroutine; | ||
| ucontext_t m_return; | ||
| eastl::function<void()> m_func; | ||
| bool m_isAlive = false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
StackfulBase implementation looks solid but missing destructor.
The base class design is well-structured with proper encapsulation. However, consider adding a virtual destructor to ensure proper cleanup of ucontext resources.
class StackfulBase {
protected:
+ virtual ~StackfulBase() = default;
void initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size);📝 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.
| class StackfulBase { | |
| protected: | |
| void initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size); | |
| void resume(); | |
| void yield(); | |
| [[nodiscard]] bool isAlive() const { return m_isAlive; } | |
| StackfulBase() = default; | |
| StackfulBase(const StackfulBase&) = delete; | |
| StackfulBase& operator=(const StackfulBase&) = delete; | |
| private: | |
| static void trampoline(void* arg) { | |
| StackfulBase* self = static_cast<StackfulBase*>(arg); | |
| self->trampoline(); | |
| } | |
| void trampoline(); | |
| ucontext_t m_coroutine; | |
| ucontext_t m_return; | |
| eastl::function<void()> m_func; | |
| bool m_isAlive = false; | |
| }; | |
| class StackfulBase { | |
| protected: | |
| virtual ~StackfulBase() = default; | |
| void initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size); | |
| void resume(); | |
| void yield(); | |
| [[nodiscard]] bool isAlive() const { return m_isAlive; } | |
| StackfulBase() = default; | |
| StackfulBase(const StackfulBase&) = delete; | |
| StackfulBase& operator=(const StackfulBase&) = delete; | |
| private: | |
| static void trampoline(void* arg) { | |
| StackfulBase* self = static_cast<StackfulBase*>(arg); | |
| self->trampoline(); | |
| } | |
| void trampoline(); | |
| ucontext_t m_coroutine; | |
| ucontext_t m_return; | |
| eastl::function<void()> m_func; | |
| bool m_isAlive = false; | |
| }; |
🤖 Prompt for AI Agents
In src/mips/psyqo/coroutine.hh around lines 212 to 233, the StackfulBase class
lacks a virtual destructor, which is necessary for proper cleanup when derived
classes are deleted through a base class pointer. Add a virtual destructor to
the StackfulBase class to ensure correct resource management and avoid potential
memory leaks or undefined behavior.
There was a problem hiding this comment.
Pull Request Overview
Adds a PSX-compatible ucontext API and a stackful coroutine example to demonstrate delayed execution via timers.
- Introduces
ucontext.h/ucontext.sfor context switching on PSX. - Extends
psyqo::Stackfulwith ucontext-based trampoline, resume, and yield. - Provides a new
coroutine-demo-3example and updates Makefiles to include the ucontext implementation.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp | New demo showing stackful coroutine usage with timed sleeps |
| src/mips/psyqo/examples/coroutine-demo-3/Makefile | Build configuration for the coroutine-demo-3 example |
| src/mips/psyqo/coroutine.hh | Added StackfulBase declarations and ucontext include |
| src/mips/psyqo/Makefile | Included ucontext.s in the build of psyqo library |
| src/mips/common/psxlibc/ucontext.s | Assembly implementation of getcontext, setcontext, etc. |
| src/mips/common/psxlibc/ucontext.h | Header definitions for PSX ucontext structures and functions |
Comments suppressed due to low confidence (4)
src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp:91
- [nitpick] The status message is identical to the previous sleep stage and may confuse readers; consider updating it to clearly indicate this is the final 5s sleep, e.g., "Starting final 5s sleep".
m_text = "Waking up... sleeping for 5s this time";
src/mips/common/psxlibc/ucontext.s:47
- The new assembly routines for context switching (
getcontext,setcontext,makecontext,swapcontext) lack dedicated tests; consider adding unit tests to validate correct register saving/restoring and stack behavior.
.section .text.getcontext, "ax", @progbits
src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp:106
- The coroutine function returns immediately after scheduling the final timer without yielding; add a
m_coroutine.yield()before exit so the "All done." callback can resume this coroutine correctly.
}
src/mips/psyqo/coroutine.hh:214
- Methods
initializeInternal,resume, andyieldare declared but not defined in any translation unit, which will cause linker errors; add their implementations in a corresponding.cppfile.
void initializeInternal(eastl::function<void()>&& func, void* ss_sp, unsigned ss_size);
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mips/psyqo/Makefile(1 hunks)src/mips/psyqo/coroutine.hh(2 hunks)src/mips/psyqo/examples/coroutine-demo-3/Makefile(1 hunks)src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp(1 hunks)src/mips/psyqo/src/coroutine.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mips/psyqo/Makefile
- src/mips/psyqo/examples/coroutine-demo-3/Makefile
- src/mips/psyqo/examples/coroutine-demo-3/coroutine-demo-3.cpp
- src/mips/psyqo/coroutine.hh
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/mips/psyqo/src/coroutine.cpp (1)
src/mips/common/psxlibc/ucontext.h (3)
getcontext(55-55)makecontext(57-57)swapcontext(58-58)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
- GitHub Check: asan
- GitHub Check: toolchain
- GitHub Check: build
- GitHub Check: aur-build
- GitHub Check: coverage
- GitHub Check: build-openbios
🔇 Additional comments (5)
src/mips/psyqo/src/coroutine.cpp (5)
1-25: LGTM: Standard MIT license header.The license header is properly formatted and appropriate for the project.
27-27: LGTM: Include directive is correct.The include properly references the coroutine header file.
29-37: LGTM: Coroutine initialization logic is sound.The initialization method correctly:
- Sets the coroutine as alive
- Moves the function to avoid unnecessary copying
- Sets up the ucontext structure with stack parameters
- Links the return context for proper cleanup
- Binds the trampoline function to the coroutine context
44-47: LGTM: Resume method implementation is correct.The method properly:
- Checks if the coroutine is alive before attempting to resume
- Uses
swapcontextto switch from caller context to coroutine context- Handles the case where the coroutine has already completed
49-52: LGTM: Yield method implementation is correct.The method properly:
- Checks if the coroutine is alive before yielding
- Uses
swapcontextto switch from coroutine context back to caller context- Prevents yielding from a dead coroutine
No description provided.