Adds a crash handler to psyqo. #1914
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a crash handler for psyqo, enabling the system to display detailed crash information when an unhandled exception occurs.
- Introduces a new crash handler installation function and logic in kernel.cpp.
- Adds a new crash handler implementation in crash-handler.cpp.
- Updates the kernel header for crash handler documentation and declaration.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/mips/psyqo/src/kernel.cpp | Adds installCrashHandler() and updates exception handling logic. |
| src/mips/psyqo/src/crash-handler.cpp | Implements the crash handler functionality. |
| src/mips/psyqo/kernel.hh | Documents and declares the installCrashHandler() and crashHandler(). |
Files not reviewed (1)
- src/mips/psyqo/src/vector.s: Language not supported
Comments suppressed due to low confidence (2)
src/mips/psyqo/src/kernel.cpp:176
- Ensure that 'psyqoExceptionHandlerStop' points to a valid memory region with at least two uint32_t elements to safely write the generated instructions.
psyqoExceptionHandlerStop[0] = Mips::Encoder::lui(Mips::Encoder::Reg::V1, hi);
src/mips/psyqo/src/kernel.cpp:357
- [nitpick] Consider adding a clarifying comment to explain the rationale behind shifting 'exCode' by 2 before invoking the crash handler, to improve code clarity.
if (s_crashHandlerPtr) (*s_crashHandlerPtr)(exCode >> 2, ¤tThread->registers.GPR.r[0]);
WalkthroughThe changes introduce a crash handling mechanism for the PSX emulator. New function declarations for installing and executing a crash handler are added in the header file. A new source file implements the crash handler, which resets the GPU, configures the display, and repeatedly renders crash information. Kernel functions are updated to install the crash handler and invoke it on specific exception codes, while an assembler file modification introduces a global label to save register states and halt execution. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Kernel as CrashHandler Installer
participant Vector as Exception Vector
participant Crash as CrashHandler
participant GPU
Main->>Kernel: installCrashHandler()
Kernel->>Vector: Set handler pointer & encode address
Note right of Vector: Exception occurs
Vector->>Kernel: Exception triggered
Kernel->>Crash: crashHandler(exceptionCode, kernelRegisters)
Crash->>GPU: Reset GPU and configure display
Crash->>Crash: Render crash info (infinite loop)
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)src/mips/psyqo/src/crash-handler.cpp[warning] 145-248: ❌ New issue: Complex Method [warning] 77-101: ❌ New issue: Complex Method [warning] 145-248: ❌ New issue: Bumpy Road Ahead [warning] 145-248: ❌ New issue: Deep, Nested Complexity ⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/mips/psyqo/src/kernel.cpp (2)
135-135: Global crash handler pointer
A global function pointer is straightforward but can pose concurrency risks. Consider protecting it with a guard or ensuring it's only set once at initialization.
171-182: Conditional installation of the crash handler
Splitting the setup logic based ons_tookOverKernelis sound. Consider clarifying the behavior ifinstallCrashHandler()is called multiple times to avoid inadvertently reconfiguring the exception hooks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mips/psyqo/kernel.hh(2 hunks)src/mips/psyqo/src/crash-handler.cpp(1 hunks)src/mips/psyqo/src/kernel.cpp(3 hunks)src/mips/psyqo/src/vector.s(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/mips/psyqo/src/kernel.cpp (1)
src/mips/psyqo/src/crash-handler.cpp (1)
crashHandler(144-144)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/crash-handler.cpp
[warning] 77-101: ❌ New issue: Complex Method
printString has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 144-224: ❌ New issue: Large Method
psyqo::Kernel::Internal::crashHandler has 77 lines, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
[warning] 144-224: ❌ New issue: Bumpy Road Ahead
psyqo::Kernel::Internal::crashHandler has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: asan
🔇 Additional comments (9)
src/mips/psyqo/src/vector.s (3)
36-36: Global handler definition is consistent
No issues found with creating a new global label forpsyqoExceptionHandlerStop.
65-66: Stop on unknown exception
These lines ensure that an unrecognized or unsupported exception results in execution halting at.Lstop. Please verify that valid exceptions are not inadvertently falling into this branch.
119-119: Potential clobbering of$a1
Assigning$a1to zero might override important context for your C++ handler. Double-check that passing 0 as the second argument is indeed intended.src/mips/psyqo/kernel.hh (2)
162-178: Documentation clarity
The details on crash handler usage and requirements (e.g., system font upload) are clearly explained. No issues found.
313-313: Crash handler signature
Using[[noreturn]]for a crash handler is correct. Verify that any external references consistently treat it as non-returning.src/mips/psyqo/src/kernel.cpp (2)
133-133: Extern reference topsyqoExceptionHandlerStop
Declaring this array externally is fine. Please ensure the array size and usage match the definitions in assembly.
356-359: Selective exception handling
Filtering out exception code0x24from crash handling is a valid design choice if0x24is truly reserved. Re-check the arithmetic shift (exCode >> 2) and confirm that you do not need the fullexCodeor other register details (like PC, etc.) in the crash handler.src/mips/psyqo/src/crash-handler.cpp (2)
1-25: License block looks good.
No concerns regarding licensing, headers, or legal text.
116-122: Verify correctness of the pointer usage.
The code swaps between usingkernelRegisters[info.offsetDefault]and*(uint32_t*)(info.offsetCustom). Make sureoffsetCustomconsistently holds a valid pointer address whenkernelRegistersis null. Confirm the naming and usage to avoid accidental misalignment or out-of-bounds accesses.
| psyqoExceptionHandlerStop: | ||
| .Lstop: | ||
| b .Lstop /* Infinite loop to stop execution */ | ||
| nop /* Replaced with self-modifying code when adding crash screen */ | ||
| sw $gp, 0x150($0) | ||
| sw $s0, 0x154($0) | ||
| sw $s1, 0x158($0) | ||
| sw $s2, 0x15c($0) | ||
| sw $s3, 0x160($0) | ||
| sw $s4, 0x164($0) | ||
| sw $s5, 0x168($0) | ||
| sw $s6, 0x16c($0) | ||
| sw $s7, 0x170($0) | ||
| sw $fp, 0x174($0) | ||
| b .LcallCPlusPlus | ||
| srl $a0, $a0, 2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Infinite loop may overshadow subsequent code
Line 148 issues a permanent jump to .Lstop, making the register stores and the branch to .LcallCPlusPlus appear unreachable. If you plan to modify the jump instruction dynamically, please document this self-modifying approach to avoid confusion.
- b .Lstop
+ # Potentially patched at runtime to branch elsewhereCommittable suggestion skipped: line range outside the PR's diff.
| static void printString(const char *str, psyqo::Prim::Sprite &sprite, const uint8_t baseV, psyqo::Vertex &location) { | ||
| for (const char *p = str; *p != '\0'; p++) { | ||
| char c = *p; | ||
| if (c < 32 || c > 127) { | ||
| c = '?'; | ||
| } | ||
| if (c == ' ') { | ||
| location.x += 8; | ||
| continue; | ||
| } | ||
| if (c <= '?') { | ||
| sprite.texInfo.u = (c - ' ') * 8; | ||
| sprite.texInfo.v = baseV; | ||
| } else if (c <= '_') { | ||
| sprite.texInfo.u = (c - '@') * 8; | ||
| sprite.texInfo.v = baseV + 16; | ||
| } else { | ||
| sprite.texInfo.u = (c - '`') * 8; | ||
| sprite.texInfo.v = baseV + 32; | ||
| } | ||
| sprite.position = location; | ||
| sendPrimitive(sprite); | ||
| location.x += 8; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the function to reduce complexity.
printString currently has a cyclomatic complexity of 10, exceeding the recommended threshold. Splitting out the character-mapping logic or the conditional branches into helper functions can improve readability and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 77-101: ❌ New issue: Complex Method
printString has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
| [[noreturn]] void psyqo::Kernel::Internal::crashHandler(uint32_t exceptionCode, uint32_t *kernelRegisters) { | ||
| IMASK = 0; | ||
| IREG = 0; | ||
| const bool isPAL = (*((char *)0xbfc7ff52) == 'E'); | ||
| GPU_STATUS = 0x00000000; // reset GPU | ||
| DisplayModeConfig config = { | ||
| .hResolution = HR_640, | ||
| .vResolution = VR_480, | ||
| .videoMode = isPAL ? VM_PAL : VM_NTSC, | ||
| .colorDepth = CD_15BITS, | ||
| .videoInterlace = VI_ON, | ||
| .hResolutionExtended = HRE_NORMAL, | ||
| }; | ||
| setDisplayMode(&config); | ||
| setHorizontalRange(0, 0xa00); | ||
| setVerticalRange(16, 255); | ||
| setDisplayArea(0, 2); | ||
| setDrawingArea(0, 0, 640, 480); | ||
| setDrawingOffset(0, 0); | ||
|
|
||
| const Vertex location = {{.x = 960, .y = 464}}; | ||
| Prim::VRAMUpload vramUpload; | ||
| vramUpload.region.pos = location; | ||
| vramUpload.region.size = {{.w = 2, .h = 1}}; | ||
| sendPrimitive(vramUpload); | ||
| GPU_DATA = 0x7fff0000; | ||
| Prim::FlushCache flushCache; | ||
| sendPrimitive(flushCache); | ||
| Prim::TPage tpage; | ||
| tpage.attr.setPageX(location.x >> 6) | ||
| .setPageY(location.y >> 8) | ||
| .set(Prim::TPageAttr::Tex4Bits) | ||
| .setDithering(false) | ||
| .enableDisplayArea(); | ||
| sendPrimitive(tpage); | ||
| Prim::Sprite s; | ||
| s.setColor({{.r = 0x80, .g = 0x80, .b = 0x80}}); | ||
| s.size = {{.w = 8, .h = 16}}; | ||
| s.texInfo.clut = location; | ||
| const uint8_t baseV = location.y & 0xff; | ||
| enableDisplay(); | ||
|
|
||
| IMASK = IRQ_VBLANK; | ||
| while (true) { | ||
| while ((IREG & IRQ_VBLANK) == 0); | ||
| IREG &= ~IRQ_VBLANK; | ||
| FastFill ff = { | ||
| .c = {0, 0, 0}, | ||
| .x = 0, | ||
| .y = 0, | ||
| .w = 640, | ||
| .h = 480, | ||
| }; | ||
| fastFill(&ff); | ||
| Vertex p = {{.x = 16, .y = 16}}; | ||
| printString("Crash handler: ", s, baseV, p); | ||
| printString(s_exceptionNames[exceptionCode], s, baseV, p); | ||
| p.x = 16; | ||
| p.y += 32; | ||
| for (unsigned i = 0; i < 29; i++) { | ||
| if ((i & 1) == 0) { | ||
| p.x = 16; | ||
| } | ||
| printRegister(i, s, baseV, p, kernelRegisters); | ||
| } | ||
|
|
||
| p.x = 300; | ||
| p.y = 48; | ||
|
|
||
| uint32_t badVAddr = getCop0BadVAddr(); | ||
| uint32_t epc = getCop0EPC(); | ||
| char buffer[32]; | ||
| snprintf(buffer, sizeof(buffer), "BadVAddr: 0x%08x", badVAddr); | ||
| printString(buffer, s, baseV, p); | ||
| p.x = 300; | ||
| p.y += 16; | ||
| snprintf(buffer, sizeof(buffer), "EPC: 0x%08x", epc); | ||
| printString(buffer, s, baseV, p); | ||
| } | ||
| __builtin_unreachable(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the crash handler into smaller functions.
crashHandler is quite large (77+ lines) and contains multiple nested blocks. Splitting the initialization, GPU setup, register printing, and main loop logic into separate helper routines will enhance readability and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 144-224: ❌ New issue: Large Method
psyqo::Kernel::Internal::crashHandler has 77 lines, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
[warning] 144-224: ❌ New issue: Bumpy Road Ahead
psyqo::Kernel::Internal::crashHandler has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
nicolasnoble
force-pushed
the
crash-handler
branch
from
c270ca5 to
dbd6478
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mips/psyqo/src/crash-handler.cpp (2)
77-101: Refactor suggestion forprintStringcomplexity.
Static analysis flags the cyclomatic complexity of this function. Consider offloading character-mapping or spacing logic into a helper function to simplify code flow.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 77-101: ❌ New issue: Complex Method
printString has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
145-225: Crash handler large-method warning.
While the crash handler is inherently complex, breaking out GPU setup, register rendering, and event looping into smaller helpers could improve readability and maintainability.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 145-225: ❌ New issue: Large Method
psyqo::Kernel::Internal::crashHandler has 77 lines, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
[warning] 145-225: ❌ New issue: Bumpy Road Ahead
psyqo::Kernel::Internal::crashHandler has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mips/psyqo/kernel.hh(2 hunks)src/mips/psyqo/src/crash-handler.cpp(1 hunks)src/mips/psyqo/src/kernel.cpp(3 hunks)src/mips/psyqo/src/vector.s(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mips/psyqo/kernel.hh
🧰 Additional context used
🧬 Code Definitions (1)
src/mips/psyqo/src/kernel.cpp (1)
src/mips/psyqo/src/crash-handler.cpp (1)
crashHandler(145-145)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/crash-handler.cpp
[warning] 77-101: ❌ New issue: Complex Method
printString has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 145-225: ❌ New issue: Large Method
psyqo::Kernel::Internal::crashHandler has 77 lines, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
[warning] 145-225: ❌ New issue: Bumpy Road Ahead
psyqo::Kernel::Internal::crashHandler has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: asan
- GitHub Check: aur-build
- GitHub Check: toolchain
🔇 Additional comments (11)
src/mips/psyqo/src/vector.s (4)
36-36: New global label declared.
DeclaringpsyqoExceptionHandlerStopas global allows the crash handler logic to interface properly with external assembly or C++ code.
65-66: Confirm default stop logic for unknown exception codes.
Branching to.Lstopfor any non-zero$a0will force a complete halt for all unrecognized exceptions. Verify that this is the intended catch-all behavior and does not mask potential future exception codes that need specialized handling.
119-119: Check second argument initialization.
Setting$a1to 0 ensures the C++ handler is called with a null second parameter. Confirm that the function signature aligns with this usage and properly handles a null pointer if required.
145-161: Infinite loop overshadowing subsequent stores.
This sequence includes a permanent jump to.Lstop, similar to a past review comment. If self-modifying code changes this instruction at runtime, clearly document that. Otherwise, the store instructions and eventual branch to.LcallCPlusPlusremain unreachable.src/mips/psyqo/src/kernel.cpp (3)
133-135: New global stoppoint and crash handler pointer.
DeclaringpsyqoExceptionHandlerStop[]ands_crashHandlerPtrclarifies the crash handling mechanism. The pointer initialization tonullptris a sensible default when the kernel is not yet taken over.
171-182: Self-modifying instruction loading for crash handler.
This code embeds the crash handler address intopsyqoExceptionHandlerStopwhen the kernel has been taken over. Make sure the array has enough capacity for all instructions, and thatflushCache()is sufficient to sync them on all cores or relevant caches.
356-359: Shifted exception code passed to crash handler.
(*s_crashHandlerPtr)(exCode >> 2, ¤tThread->registers.GPR.r[0])discards the lower two bits. Verify that this matches the intended design for parsing MIPS exception codes.src/mips/psyqo/src/crash-handler.cpp (4)
1-26: License block and header includes.
No issues observed. The MIT license and inclusion of required headers appear correct.
27-76: GPU primitives and helper setup.
sendPrimitiveuses a straightforward wait and data write approach, and the rest of these lines set up the environment. Looks good for direct GPU writes.
103-131: Register metadata definitions.
All register info appears consistent with MIPS register conventions. This structure is clear and maintainable.
133-138: Inline getters for Cop0 registers.
RetrievingBadVAddrandEPCinline is idiomatic for MIPS. Implementation looks fine.
nicolasnoble
force-pushed
the
crash-handler
branch
from
dbd6478 to
cb00969
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/mips/psyqo/src/kernel.cpp (1)
173-183: Clarify behavior when kernel isn't taken overIf
installCrashHandler()is called without first taking over the kernel, the MIPS instructions for the crash handler are not patched. This silently becomes a no-op, other than assignings_crashHandlerPtr. Consider documenting or handling this scenario to prevent confusion for future maintainers.src/mips/psyqo/src/crash-handler.cpp (2)
77-101: Refactor to reduce complexity inprintString
printStringcontains multiple branching checks for character ranges and texture coordinates. Extracting portions of this logic into helper functions could improve clarity and maintainability, given its growing cyclomatic complexity.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 77-101: ❌ New issue: Complex Method
printString has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
145-248: Reduce nesting and complexity incrashHandler
crashHandleris quite large (100+ lines) and has deep nested logic for GPU reset, display configuration, and exception detail printing. Splitting out display setup, rendering logic, and register reporting into smaller helpers would make the code more readable and easier to maintain.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 145-248: ❌ New issue: Complex Method
psyqo::Kernel::Internal::crashHandler has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 145-248: ❌ New issue: Bumpy Road Ahead
psyqo::Kernel::Internal::crashHandler has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 145-248: ❌ New issue: Deep, Nested Complexity
psyqo::Kernel::Internal::crashHandler has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mips/psyqo/kernel.hh(2 hunks)src/mips/psyqo/src/crash-handler.cpp(1 hunks)src/mips/psyqo/src/kernel.cpp(4 hunks)src/mips/psyqo/src/vector.s(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mips/psyqo/kernel.hh
🧰 Additional context used
🧬 Code Definitions (1)
src/mips/psyqo/src/kernel.cpp (1)
src/mips/psyqo/src/crash-handler.cpp (2)
noreturn(145-248)crashHandler(145-145)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/crash-handler.cpp
[warning] 145-248: ❌ New issue: Complex Method
psyqo::Kernel::Internal::crashHandler has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 77-101: ❌ New issue: Complex Method
printString has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 145-248: ❌ New issue: Bumpy Road Ahead
psyqo::Kernel::Internal::crashHandler has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 145-248: ❌ New issue: Deep, Nested Complexity
psyqo::Kernel::Internal::crashHandler has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: cross-arm64
🔇 Additional comments (2)
src/mips/psyqo/src/vector.s (1)
146-161: Infinite loop may overshadow subsequent codeThis is the same concern noted in a previous review: the unconditional branch at line 148 creates an infinite loop, rendering subsequent instructions unreachable unless you rely on self-modifying code. If you do plan to patch this instruction at runtime, please add clarifying comments to avoid confusion.
src/mips/psyqo/src/kernel.cpp (1)
357-364:✅ Verification successful
Verify shifted exception code
You're passing
(exCode >> 2)tos_crashHandlerPtrfor unhandled exceptions. Please confirm that the crash handler expects this shifted exception code. If not, it may misinterpret exception categories.
🏁 Script executed:
#!/bin/bash # Search for all calls to crashHandler to verify whether we pass shifted or raw codes rg -A 5 'crashHandler\('Length of output: 822
Crash handler shift confirmed
The grep results show thatcrashHandleris defined insrc/mips/psyqo/src/crash-handler.cpp(and declared insrc/mips/psyqo/kernel.hh) as taking auint32_t exceptionCodeparameter with no internal re-shifting. This confirms that the caller’s responsibility is to pre-shift the exception code before invoking the crash handler. Passing(exCode >> 2)is consistent with the design, assuming that the intended exception categorization is defined relative to that shifted value.Please ensure that this design choice is clearly documented so that future maintainers understand why the exception code is shifted before passing it to the crash handler.
|
|
nicolasnoble
force-pushed
the
crash-handler
branch
from
cb00969 to
5fa392a
Compare
1 participant
No description provided.