Add monitor command to retrieve the shared memory name. Only for wram so far #1954
Conversation
WalkthroughA new "sharedmem" monitor command was introduced to the GDB server, allowing retrieval of the WRAM shared memory name. Supporting changes include exposing the shared memory name via a new getter, ensuring consistent state management of the shared memory name, and granting GdbClient access to Memory internals. Changes
Sequence Diagram(s)sequenceDiagram
participant GDB_Client
participant GDB_Server
participant Memory
participant SharedMem
GDB_Client->>GDB_Server: monitor sharedmem wram
GDB_Server->>Memory: (access to WRAM shared memory object)
GDB_Server->>SharedMem: getSharedName()
SharedMem-->>GDB_Server: shared memory name
GDB_Server-->>GDB_Client: shared memory name\nOK
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 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 (1)
src/core/gdb-server.cc (1)
763-772: Consider refactoring to address complexity growth.The static analysis tool flagged increasing cyclomatic complexity in
processMonitorCommand. While this specific addition is reasonable, consider extracting individual monitor command handlers into separate methods to improve maintainability as more commands are added.For example:
void PCSX::GdbClient::processMonitorCommand(const std::string& cmd) { auto words = StringsHelpers::split(cmd, " "); if (words[0] == "reset") { handleResetCommand(words); } else if (words[0] == "mountcd") { handleMountCdCommand(cmd, words); } else if (words[0] == "sharedmem") { handleSharedMemCommand(words); } write("OK"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/gdb-server.cc(1 hunks)src/core/psxmem.h(1 hunks)src/support/sharedmem-windows.cc(3 hunks)src/support/sharedmem.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in the psyqo codebase, the `safe` enum is defined in the `safe-memory.hh` header file and should be ...
Learnt from: nicolasnoble
PR: grumpycoders/pcsx-redux#1850
File: src/mips/psyqo/gpu.hh:354-354
Timestamp: 2025-01-21T18:09:06.681Z
Learning: In the psyqo codebase, the `Safe` enum is defined in the `safe-memory.hh` header file and should be explicitly included when using the `Safe` enum, rather than relying on transitive includes through other headers.
Applied to files:
src/core/psxmem.h
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/core/gdb-server.cc
[warning] 763-772: ❌ Getting worse: Complex Method
PCSX::GdbClient::processMonitorCommand increases in cyclomatic complexity from 13 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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: build-openbios
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
🔇 Additional comments (5)
src/core/psxmem.h (1)
310-310: LGTM! Friend class declaration enables GDB server integration.This change grants
GdbClientaccess toMemory's private members, which is necessary for the new "sharedmem" monitor command to retrieve shared memory names from the WRAM shared memory object.src/support/sharedmem.h (1)
52-52: LGTM! Clean getter method implementation.The public accessor method properly exposes the shared memory name while maintaining encapsulation by returning a copy of the string.
src/core/gdb-server.cc (1)
763-772: LGTM! Well-structured monitor command implementation.The new "sharedmem" command follows the established pattern with proper:
- Argument validation
- Clear usage messages
- Error handling for unsupported types
- Access to the shared memory name via the new getter
src/support/sharedmem-windows.cc (2)
40-40: LGTM! Proper conversion from local to member variable.The change from local
fullnamevariable to memberm_sharedNameenables persistent storage of the shared memory name while maintaining the same functionality.Also applies to: 44-44
59-59: LGTM! Comprehensive cleanup of member variable.Good practice to clear
m_sharedNamein all failure paths and the destructor, ensuring consistent state management of the shared memory name.Also applies to: 63-63, 81-81
src/support/sharedmem.h
Outdated
| uint8_t* getPtr() { return m_mem; } | ||
| size_t getSize() { return m_size; } | ||
|
|
||
| std::string getSharedName() { return m_sharedName; } |
There was a problem hiding this comment.
const std::string& getSharedName() const { return m_sharedName; }
src/core/gdb-server.cc
Outdated
| writeEscaped("Usage: sharedmem <type>"); | ||
| } else { | ||
| if (words[1] == "wram") { | ||
| writeEscaped(g_emulator->m_mem->m_wramShared.getSharedName().c_str()); |
There was a problem hiding this comment.
writeEscaped(g_emulator->m_mem->m_wramShared.getSharedName());
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/gdb-server.cc (3)
763-772: Consider refactoring to reduce function complexity.While the new functionality is well-implemented, the static analysis tool correctly flags that
processMonitorCommandis growing in complexity (now at 16 vs threshold of 9).Consider extracting monitor command handlers into separate methods:
+ } else if (words[0] == "sharedmem") { + processSharedMemCommand(words); }Then implement:
void PCSX::GdbClient::processSharedMemCommand(const std::vector<std::string>& words) { if (words.size() != 2) { writeEscaped("Usage: sharedmem <type>"); } else { if (words[1] == "wram") { writeEscaped(g_emulator->m_mem->m_wramShared.getSharedName()); } else { writeEscaped("Unknown type. Valid types: wram"); } } }
765-765: Add missing newline in usage message.The usage message is missing a trailing newline for consistency with other error messages in the function.
- writeEscaped("Usage: sharedmem <type>"); + writeEscaped("Usage: sharedmem <type>\n");
770-770: Add missing newline in error message.The error message is missing a trailing newline for consistency with other messages in the function.
- writeEscaped("Unknown type. Valid types: wram"); + writeEscaped("Unknown type. Valid types: wram\n");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/gdb-server.cc(1 hunks)src/support/sharedmem.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/support/sharedmem.h
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/core/gdb-server.cc
[warning] 763-772: ❌ Getting worse: Complex Method
PCSX::GdbClient::processMonitorCommand increases in cyclomatic complexity from 13 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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cross-arm64
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/core/gdb-server.cc (1)
763-772: LGTM! Clean implementation of the sharedmem monitor command.The implementation correctly validates arguments, handles the "wram" type as specified in the PR objectives, and provides appropriate error messages for invalid usage.
No description provided.