Adding "msan"-like feature. #1855
Conversation
WalkthroughThis pull request introduces memory sanitization (MSAN) support across multiple modules. The changes modify the debug logic to exclude MSAN addresses from kernel checks, add new MSAN-specific memory management functions in hardware access routines, and extend the Memory class with methods and fields to manage MSAN allocations. PSX address handling is also updated to incorporate the new MSAN segment and type. Changes
Sequence Diagram(s)sequenceDiagram
participant CPU
participant HW
participant Memory
participant MSAN
CPU->>HW: read32(address=0x1f80208c, size)
HW->>Memory: msanAlloc(size)
Memory->>MSAN: Allocate memory & update bitmap
MSAN-->>Memory: Return allocated pointer
Memory-->>HW: Return pointer
HW-->>CPU: Return pointer
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (6)
src/core/psxmem.cc (1)
155-155: Use format specifiers matching the variable type.
rom_sizeis of typesize_t, so consider using%zuinstead of%iwhen printing the loaded bytes to avoid potential mismatch warnings.- g_system->printf(_("Loaded %i bytes to EXP1 from file: %s\n"), rom_size, exp1Path.string()); + g_system->printf(_("Loaded %zu bytes to EXP1 from file: %s\n"), rom_size, exp1Path.string());src/core/psxmem.h (2)
80-84: Add documentation for the MSAN methods.The methods provide a comprehensive API for memory sanitization, but they lack documentation explaining their purpose, parameters, and return values.
Add documentation for each method:
+ // Initializes memory sanitizer structures void initMsan(); + // Returns true if memory sanitizer is initialized bool msanInitialized() { return m_msanRAM != nullptr; } + // Allocates size bytes of memory and returns the pointer + // @param size Number of bytes to allocate + // @return Pointer to allocated memory uint32_t msanAlloc(uint32_t size); + // Frees memory at the given pointer + // @param ptr Pointer to memory to free void msanFree(uint32_t ptr); + // Reallocates memory at ptr to new size + // @param ptr Pointer to memory to reallocate + // @param size New size in bytes + // @return Pointer to reallocated memory uint32_t msanRealloc(uint32_t ptr, uint32_t size);
261-268: Add documentation for the MSAN member variables.The member variables provide the necessary state for memory sanitization, but they lack documentation explaining their purpose and relationships.
Add documentation for each member:
+ // Maximum size for MSAN memory (1.5GB) static constexpr uint32_t c_msanSize = 1'610'612'736; + // Memory buffer for MSAN allocations uint8_t *m_msanRAM = nullptr; + // Bitmap tracking memory state uint8_t *m_msanBitmap = nullptr; + // Bitmap tracking written memory uint8_t *m_msanWrittenBitmap = nullptr; + // Current allocation pointer uint32_t m_msanPtr = 1024; + // Event listener for handling memory events EventBus::Listener m_listener; + // Map tracking memory allocations (ptr -> size) std::unordered_map<uint32_t, uint32_t> m_msanAllocs;src/core/psxhw.cc (3)
458-460: Consider refactoring the write8 method.The code correctly initializes MSAN, but the method's complexity is increasing beyond recommended thresholds.
Consider extracting the switch statement into smaller functions grouped by functionality:
- initializeMemory()
- handleIO()
- handleDebug()
etc.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 458-460: ❌ Getting worse: Complex Method
PCSX::HW::write8 increases in cyclomatic complexity from 23 to 24, 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.
363-367: Consider refactoring the read32 method.The code correctly handles memory allocation, but the method's complexity is increasing beyond recommended thresholds.
Consider extracting the switch statement into smaller functions grouped by functionality:
- handleMemoryOperations()
- handleDMAOperations()
- handleTimerOperations()
etc.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 363-373: ❌ Getting worse: Complex Method
PCSX::HW::read32 increases in cyclomatic complexity from 24 to 26, 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.
804-807: Consider refactoring the write32 method.The code correctly handles memory deallocation, but the method's complexity is increasing beyond recommended thresholds.
Consider extracting the switch statement into smaller functions grouped by functionality:
- handleMemoryOperations()
- handleGPUOperations()
- handleMDECOperations()
etc.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 804-807: ❌ Getting worse: Complex Method
PCSX::HW::write32 increases in cyclomatic complexity from 48 to 49, 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.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/debug.cc(1 hunks)src/core/psxhw.cc(3 hunks)src/core/psxmem.cc(11 hunks)src/core/psxmem.h(4 hunks)src/mips/common/hardware/pcsxhw.h(1 hunks)src/supportpsx/memory.h(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/core/psxhw.cc
[warning] 363-373: ❌ Getting worse: Complex Method
PCSX::HW::read32 increases in cyclomatic complexity from 24 to 26, 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] 458-460: ❌ Getting worse: Complex Method
PCSX::HW::write8 increases in cyclomatic complexity from 23 to 24, 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] 804-807: ❌ Getting worse: Complex Method
PCSX::HW::write32 increases in cyclomatic complexity from 48 to 49, 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.
src/core/psxmem.cc
[warning] 284-297: ❌ Getting worse: Complex Method
PCSX::Memory::read8 increases in cyclomatic complexity from 14 to 19, 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] 284-297: ❌ Getting worse: Complex Conditional
PCSX::Memory::read8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 333-349: ❌ Getting worse: Complex Method
PCSX::Memory::read16 increases in cyclomatic complexity from 12 to 18, 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] 333-349: ❌ Getting worse: Complex Conditional
PCSX::Memory::read16 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 382-398: ❌ Getting worse: Complex Method
PCSX::Memory::read32 increases in cyclomatic complexity from 14 to 20, 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] 382-398: ❌ Getting worse: Complex Conditional
PCSX::Memory::read32 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 508-516: ❌ Getting worse: Complex Method
PCSX::Memory::write8 increases in cyclomatic complexity from 12 to 16, 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] 508-516: ❌ Getting worse: Complex Conditional
PCSX::Memory::write8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 545-555: ❌ Getting worse: Complex Method
PCSX::Memory::write16 increases in cyclomatic complexity from 12 to 17, 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] 545-555: ❌ Getting worse: Complex Conditional
PCSX::Memory::write16 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 585-595: ❌ Getting worse: Complex Method
PCSX::Memory::write32 increases in cyclomatic complexity from 19 to 24, 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] 585-595: ❌ Getting worse: Complex Conditional
PCSX::Memory::write32 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 886-929: ❌ New issue: Complex Method
PCSX::Memory::msanRealloc has a cyclomatic complexity of 9, 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.
⏰ 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 (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: toolchain
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
- GitHub Check: aur-build
- GitHub Check: coverage
🔇 Additional comments (19)
src/core/psxmem.cc (9)
88-98: Memory constructor initialization looks correct.It properly registers a listener for the reset event and frees MSAN-related memory to avoid leaks upon reset. No immediate issues spotted.
265-273: Shutdown now frees MSAN memory.This ensures MSAN-related data is cleaned up properly, preventing memory leaks. The approach is straightforward and safe because calling
freeon a null pointer is valid.
284-297: Fix typo and consider reducing method complexity.
- Typo in log message: "unsable" → "unusable".
- Static analysis flags this method as increasingly complex. Consider extracting MSAN checks into a helper function to improve readability and reduce cyclomatic complexity.
[nitpick, refactor_suggestion_good_to_have]
- g_system->log(LogClass::CPU, _("8-bit read from unsable msan memory: %8.8lx\n"), address); + g_system->log(LogClass::CPU, _("8-bit read from unusable msan memory: %8.8lx\n"), address);🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 284-297: ❌ Getting worse: Complex Method
PCSX::Memory::read8 increases in cyclomatic complexity from 14 to 19, 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] 284-297: ❌ Getting worse: Complex Conditional
PCSX::Memory::read8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
333-349: Fix typo and consider reducing method complexity.
- Typo in log message: "unsable" → "unusable".
- This method’s cyclomatic complexity is high due to multiple nested checks. A helper utility for MSAN reads could reduce complexity and improve maintainability.
[nitpick, refactor_suggestion_good_to_have]
- g_system->log(LogClass::CPU, _("16-bit read from unsable msan memory: %8.8lx\n"), + g_system->log(LogClass::CPU, _("16-bit read from unusable msan memory: %8.8lx\n"),🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 333-349: ❌ Getting worse: Complex Method
PCSX::Memory::read16 increases in cyclomatic complexity from 12 to 18, 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] 333-349: ❌ Getting worse: Complex Conditional
PCSX::Memory::read16 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
382-398: Correct spelling and refactor large conditional blocks.
- “unsable” → “unusable” in the log message.
- This function’s branching contributes to high complexity. Factor out repeated MSAN checks into a dedicated helper to keep the core logic concise.
[nitpick, refactor_suggestion_good_to_have]
- g_system->log(LogClass::CPU, _("32-bit read from unsable msan memory: %8.8lx\n"), + g_system->log(LogClass::CPU, _("32-bit read from unusable msan memory: %8.8lx\n"),🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 382-398: ❌ Getting worse: Complex Method
PCSX::Memory::read32 increases in cyclomatic complexity from 14 to 20, 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] 382-398: ❌ Getting worse: Complex Conditional
PCSX::Memory::read32 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
508-516: Resolve spelling error and reduce branching.
- Use “unusable” instead of “unsable”.
- Consider a dedicated MSAN write helper function for shorter condition blocks and improved readability.
[nitpick, refactor_suggestion_good_to_have]
- g_system->log(LogClass::CPU, _("8-bit write to unsable msan memory: %8.8lx\n"), address); + g_system->log(LogClass::CPU, _("8-bit write to unusable msan memory: %8.8lx\n"), address);🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 508-516: ❌ Getting worse: Complex Method
PCSX::Memory::write8 increases in cyclomatic complexity from 12 to 16, 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] 508-516: ❌ Getting worse: Complex Conditional
PCSX::Memory::write8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
545-555: Typo fix and method complexity concerns.
- Change “unsable” → “unusable” in the log.
- Extracting MSAN write checks into a helper could lessen duplication and meet complexity warnings.
[nitpick, refactor_suggestion_good_to_have]
- g_system->log(LogClass::CPU, _("16-bit write to unsable msan memory: %8.8lx\n"), address); + g_system->log(LogClass::CPU, _("16-bit write to unusable msan memory: %8.8lx\n"), address);🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 545-555: ❌ Getting worse: Complex Method
PCSX::Memory::write16 increases in cyclomatic complexity from 12 to 17, 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] 545-555: ❌ Getting worse: Complex Conditional
PCSX::Memory::write16 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
585-595: Correct the log message and simplify flow.
- Replace “unsable” with “unusable”.
- Consider breaking out MSAN checks to contain the branching, helping maintain clearer logic for main write operations.
[nitpick, refactor_suggestion_good_to_have]
- g_system->log(LogClass::CPU, _("32-bit write to unsable msan memory: %8.8lx\n"), address); + g_system->log(LogClass::CPU, _("32-bit write to unusable msan memory: %8.8lx\n"), address);🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 585-595: ❌ Getting worse: Complex Method
PCSX::Memory::write32 increases in cyclomatic complexity from 19 to 24, 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] 585-595: ❌ Getting worse: Complex Conditional
PCSX::Memory::write32 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
820-929: Clarify bitwise alignment expression and consider concurrency.
- The expression
(actualSize + 15 & ~15)is parsed as((actualSize + 15) & ~15). Add parentheses for clarity.- If multiple threads can call these MSAN routines, consider adding thread-safety to
m_msanAllocsand pointer arithmetic operations.[refactor_suggestion_good_to_have, architecture_advice]
- actualSize = actualSize + 15 & ~15; + actualSize = (actualSize + 15) & ~15;🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 886-929: ❌ New issue: Complex Method
PCSX::Memory::msanRealloc has a cyclomatic complexity of 9, 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.src/mips/common/hardware/pcsxhw.h (1)
38-49: Validate calling convention constraints for these inline stubs.Storing parameters in
asm("a0")is highly compiler-specific and may require additional barriers or extended inline assembly to ensure correctness. Verify that this matches your toolchain’s calling convention expectations and that no further synchronization is necessary.src/supportpsx/memory.h (5)
28-33: Ensure address range aligns with MSAN region size.This condition sets the segment to
MSANfor addresses between0x20000000and0x80000000, implying a 1.5GB span. Double-check that this matches the actual MSAN memory size (1.5GB vs. 0x60000000 bytes = 1.6GB).
105-107: MSAN type offset looks correct.Adding
0x20000000tophysicalis consistent with the earlier assignment in the constructor.
110-110: Extending segment case for MSAN is valid.Accepts
Segment::MSANas a standard path, returning the computed address.
131-132: New MSAN enum value in Type.Makes sense to define a dedicated type for MSAN memory to keep logic distinct from standard RAM.
139-140: New MSAN segment enum value.Allows structured handling of MSAN addresses as a separate segment.
src/core/psxmem.h (2)
23-23: LGTM!The new includes are required for the new member variables
m_msanAllocsandm_listener.Also applies to: 27-27
52-52: LGTM!The default constructor is required to initialize the new MSAN-related member variables.
src/core/debug.cc (1)
62-62: LGTM!The change correctly excludes MSAN addresses from being treated as kernel addresses, which is necessary for proper memory access checks.
src/core/psxhw.cc (1)
368-373: LGTM!The code correctly handles memory reallocation by retrieving the pointer and size from CPU registers.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 363-373: ❌ Getting worse: Complex Method
PCSX::HW::read32 increases in cyclomatic complexity from 24 to 26, 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.
nicolasnoble
force-pushed
the
psxmsan
branch
from
541813b to
1359143
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/core/psxmem.cc (1)
508-516: 🛠️ Refactor suggestionReduce code duplication in write methods.
The MSAN checks in write8, write16, and write32 follow the same pattern as the read methods. The same refactoring suggestions apply:
- Extract common MSAN validation logic
- Use templates for size handling
- Unify error reporting
This will maintain consistency with the read method improvements.
Also applies to: 545-555, 585-595
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 508-516: ❌ Getting worse: Complex Method
PCSX::Memory::write8 increases in cyclomatic complexity from 12 to 16, 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] 508-516: ❌ Getting worse: Complex Conditional
PCSX::Memory::write8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
🧹 Nitpick comments (3)
src/mips/common/hardware/pcsxhw.h (1)
40-49: LGTM! Well-structured memory management functions.The functions provide a clean interface for MSAN memory operations. Consider adding documentation to explain:
- The purpose of each memory-mapped address
- The expected register values and return types
- Any error conditions or edge cases
src/core/psxmem.h (1)
261-268: LGTM! Comprehensive MSAN state management.The member variables provide a solid foundation for MSAN functionality. Consider adding documentation to explain:
- The purpose and layout of the bitmaps
- The significance of the size constant
- The structure of the allocation map
src/core/psxmem.cc (1)
843-938: Add additional safety checks to memory management.The memory management implementation is solid but could be enhanced:
- Add alignment checks for pointers
- Validate allocation sizes (e.g., maximum size limits)
- Consider adding memory poisoning for freed regions
- Add debug assertions for invariants
This will help catch memory-related issues earlier.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 895-938: ❌ New issue: Complex Method
PCSX::Memory::msanRealloc has a cyclomatic complexity of 9, 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.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/debug.cc(1 hunks)src/core/psxhw.cc(3 hunks)src/core/psxmem.cc(11 hunks)src/core/psxmem.h(4 hunks)src/mips/common/hardware/pcsxhw.h(1 hunks)src/supportpsx/memory.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/debug.cc
- src/supportpsx/memory.h
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/core/psxhw.cc
[warning] 363-373: ❌ Getting worse: Complex Method
PCSX::HW::read32 increases in cyclomatic complexity from 24 to 26, 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] 458-460: ❌ Getting worse: Complex Method
PCSX::HW::write8 increases in cyclomatic complexity from 23 to 24, 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] 804-807: ❌ Getting worse: Complex Method
PCSX::HW::write32 increases in cyclomatic complexity from 48 to 49, 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.
src/core/psxmem.cc
[warning] 284-297: ❌ Getting worse: Complex Method
PCSX::Memory::read8 increases in cyclomatic complexity from 14 to 19, 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] 284-297: ❌ Getting worse: Complex Conditional
PCSX::Memory::read8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 333-349: ❌ Getting worse: Complex Method
PCSX::Memory::read16 increases in cyclomatic complexity from 12 to 18, 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] 333-349: ❌ Getting worse: Complex Conditional
PCSX::Memory::read16 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 382-398: ❌ Getting worse: Complex Method
PCSX::Memory::read32 increases in cyclomatic complexity from 14 to 20, 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] 382-398: ❌ Getting worse: Complex Conditional
PCSX::Memory::read32 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 508-516: ❌ Getting worse: Complex Method
PCSX::Memory::write8 increases in cyclomatic complexity from 12 to 16, 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] 508-516: ❌ Getting worse: Complex Conditional
PCSX::Memory::write8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 545-555: ❌ Getting worse: Complex Method
PCSX::Memory::write16 increases in cyclomatic complexity from 12 to 17, 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] 545-555: ❌ Getting worse: Complex Conditional
PCSX::Memory::write16 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 585-595: ❌ Getting worse: Complex Method
PCSX::Memory::write32 increases in cyclomatic complexity from 19 to 24, 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] 585-595: ❌ Getting worse: Complex Conditional
PCSX::Memory::write32 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 895-938: ❌ New issue: Complex Method
PCSX::Memory::msanRealloc has a cyclomatic complexity of 9, 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.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: asan
🔇 Additional comments (3)
src/mips/common/hardware/pcsxhw.h (1)
38-39: LGTM! Clear and focused MSAN initialization functions.The functions provide a clean interface for initializing and resetting MSAN functionality.
src/core/psxmem.h (1)
80-84: LGTM! Well-defined MSAN method declarations.The method signatures are clear and follow a consistent pattern.
src/core/psxmem.cc (1)
88-98: LGTM! Clean event handling and resource management.The constructor properly sets up reset event handling and ensures proper cleanup of MSAN resources.
| case 0x1f80208c: { | ||
| uint32_t size = g_emulator->m_cpu->m_regs.GPR.n.a0; | ||
| hard = g_emulator->m_mem->msanAlloc(size); | ||
| break; | ||
| } | ||
| case 0x1f802090: { | ||
| uint32_t ptr = g_emulator->m_cpu->m_regs.GPR.n.a0; | ||
| uint32_t size = g_emulator->m_cpu->m_regs.GPR.n.a1; | ||
| hard = g_emulator->m_mem->msanRealloc(ptr, size); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce complexity.
The read32 and write32 methods have high cyclomatic complexity (26 and 49 respectively) and are getting more complex with the new MSAN cases. Consider:
- Extracting MSAN-related operations into separate methods
- Using a command pattern or lookup table for different address handlers
- Breaking down the large switch statements into smaller, focused functions
This will improve maintainability and make future additions easier.
Would you like me to propose a refactored structure that reduces the complexity?
Also applies to: 804-807
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 363-373: ❌ Getting worse: Complex Method
PCSX::HW::read32 increases in cyclomatic complexity from 24 to 26, 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.
| } else if (msanInitialized() && (address >= 0x20000000) && (address < (0x20000000 + c_msanSize))) { | ||
| uint32_t msanAddress = address - 0x20000000; | ||
| if ((m_msanWrittenBitmap[msanAddress / 8] & (1 << (msanAddress % 8))) == 0) { | ||
| g_system->log(LogClass::CPU, _("8-bit read from usable but uninitialized msan memory: %8.8lx\n"), address); | ||
| g_system->pause(); | ||
| return 0; | ||
| } | ||
| if (m_msanBitmap[msanAddress / 8] & (1 << (msanAddress % 8))) { | ||
| return m_msanRAM[msanAddress]; | ||
| } else { | ||
| g_system->log(LogClass::CPU, _("8-bit read from unsable msan memory: %8.8lx\n"), address); | ||
| g_system->pause(); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in read methods.
The MSAN checks in read8, read16, and read32 are very similar. Consider:
- Extracting common MSAN validation logic into helper methods
- Using templates to handle different sizes
- Creating a unified error reporting function
This will reduce duplication and make the code more maintainable.
Would you like me to propose a refactored structure that reduces duplication?
Also applies to: 333-349, 382-398
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 284-297: ❌ Getting worse: Complex Method
PCSX::Memory::read8 increases in cyclomatic complexity from 14 to 19, 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] 284-297: ❌ Getting worse: Complex Conditional
PCSX::Memory::read8 increases from 2 complex conditionals with 4 branches to 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
| void PCSX::Memory::initMsan(bool reset) { | ||
| if (reset) { | ||
| free(m_msanRAM); | ||
| free(m_msanBitmap); | ||
| free(m_msanWrittenBitmap); | ||
| m_msanRAM = nullptr; | ||
| m_msanBitmap = nullptr; | ||
| m_msanWrittenBitmap = nullptr; | ||
| m_msanAllocs.clear(); | ||
| } | ||
| if (msanInitialized()) { | ||
| g_system->printf(_("MSAN system was already initialized.\n")); | ||
| g_system->pause(); | ||
| return; | ||
| } | ||
|
|
||
| // 1.5GB of RAM, with 384MB worth of bitmap, between 0x20000000 and 0x80000000 | ||
| m_msanRAM = (uint8_t *)calloc(c_msanSize, 1); | ||
| m_msanBitmap = (uint8_t *)calloc(c_msanSize / 8, 1); | ||
| m_msanWrittenBitmap = (uint8_t *)calloc(c_msanSize / 8, 1); | ||
| m_msanPtr = 1024; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for memory allocation failures.
The initialization looks good but should handle allocation failures. Consider:
- Checking allocation results
- Cleaning up on partial allocation failures
- Returning a status code to indicate success/failure
void PCSX::Memory::initMsan(bool reset) {
// ... reset code ...
m_msanRAM = (uint8_t *)calloc(c_msanSize, 1);
+ if (!m_msanRAM) {
+ g_system->printf(_("Failed to allocate MSAN RAM\n"));
+ g_system->pause();
+ return;
+ }
m_msanBitmap = (uint8_t *)calloc(c_msanSize / 8, 1);
+ if (!m_msanBitmap) {
+ free(m_msanRAM);
+ m_msanRAM = nullptr;
+ g_system->printf(_("Failed to allocate MSAN bitmap\n"));
+ g_system->pause();
+ return;
+ }
m_msanWrittenBitmap = (uint8_t *)calloc(c_msanSize / 8, 1);
+ if (!m_msanWrittenBitmap) {
+ free(m_msanRAM);
+ free(m_msanBitmap);
+ m_msanRAM = nullptr;
+ m_msanBitmap = nullptr;
+ g_system->printf(_("Failed to allocate MSAN written bitmap\n"));
+ g_system->pause();
+ return;
+ }
m_msanPtr = 1024;
}📝 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.
| void PCSX::Memory::initMsan(bool reset) { | |
| if (reset) { | |
| free(m_msanRAM); | |
| free(m_msanBitmap); | |
| free(m_msanWrittenBitmap); | |
| m_msanRAM = nullptr; | |
| m_msanBitmap = nullptr; | |
| m_msanWrittenBitmap = nullptr; | |
| m_msanAllocs.clear(); | |
| } | |
| if (msanInitialized()) { | |
| g_system->printf(_("MSAN system was already initialized.\n")); | |
| g_system->pause(); | |
| return; | |
| } | |
| // 1.5GB of RAM, with 384MB worth of bitmap, between 0x20000000 and 0x80000000 | |
| m_msanRAM = (uint8_t *)calloc(c_msanSize, 1); | |
| m_msanBitmap = (uint8_t *)calloc(c_msanSize / 8, 1); | |
| m_msanWrittenBitmap = (uint8_t *)calloc(c_msanSize / 8, 1); | |
| m_msanPtr = 1024; | |
| } | |
| void PCSX::Memory::initMsan(bool reset) { | |
| if (reset) { | |
| free(m_msanRAM); | |
| free(m_msanBitmap); | |
| free(m_msanWrittenBitmap); | |
| m_msanRAM = nullptr; | |
| m_msanBitmap = nullptr; | |
| m_msanWrittenBitmap = nullptr; | |
| m_msanAllocs.clear(); | |
| } | |
| if (msanInitialized()) { | |
| g_system->printf(_("MSAN system was already initialized.\n")); | |
| g_system->pause(); | |
| return; | |
| } | |
| // 1.5GB of RAM, with 384MB worth of bitmap, between 0x20000000 and 0x80000000 | |
| m_msanRAM = (uint8_t *)calloc(c_msanSize, 1); | |
| if (!m_msanRAM) { | |
| g_system->printf(_("Failed to allocate MSAN RAM\n")); | |
| g_system->pause(); | |
| return; | |
| } | |
| m_msanBitmap = (uint8_t *)calloc(c_msanSize / 8, 1); | |
| if (!m_msanBitmap) { | |
| free(m_msanRAM); | |
| m_msanRAM = nullptr; | |
| g_system->printf(_("Failed to allocate MSAN bitmap\n")); | |
| g_system->pause(); | |
| return; | |
| } | |
| m_msanWrittenBitmap = (uint8_t *)calloc(c_msanSize / 8, 1); | |
| if (!m_msanWrittenBitmap) { | |
| free(m_msanRAM); | |
| free(m_msanBitmap); | |
| m_msanRAM = nullptr; | |
| m_msanBitmap = nullptr; | |
| g_system->printf(_("Failed to allocate MSAN written bitmap\n")); | |
| g_system->pause(); | |
| return; | |
| } | |
| m_msanPtr = 1024; | |
| } |
1 participant
No description provided.