Add PSYQo heap allocator viewer to Redux#2010
Merged
Merged
Conversation
PSYQo's allocator now registers a metadata struct with the emulator via a new pcsxhw register (0x1f8020a0), giving Redux live access to the heap's free list head, bottom, top, high-water mark, and marker pointers. The viewer walks the free list from guest memory each frame, reconstructs the full block layout, and displays it as a visual bar map with a detailed block table. The walker is hardened against memory corruption: bounds checks on every pointer read, free list ascending-order validation (catches cycles), size alignment and range checks, iteration caps, and accounting mismatch detection. Corruption is surfaced prominently in the UI with specific diagnostics. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
📝 WalkthroughWalkthroughRegisters PSYQo allocator metadata via a new MMIO register, stores the pointer in emulator memory, adds a Heap Viewer GUI widget that reads and validates the guest heap/free-list, and visualizes blocks with corruption reporting and jump-to-memory actions. Changes
Sequence DiagramsequenceDiagram
participant Alloc as PSYQo Allocator
participant HW as MMIO (0x1f8020a0)
participant Mem as Emulator Memory
participant GUI as GUI (HeapViewer)
participant UI as Display/UI
Alloc->>Alloc: initialize s_heap_metadata
Alloc->>HW: pcsx_registerHeapMetadata(&s_heap_metadata)
HW->>Mem: store pointer in m_psyqoHeapMetadata
UI->>GUI: enable Heap Viewer
GUI->>Mem: read m_psyqoHeapMetadata and heap memory
GUI->>GUI: walkHeap() — validate free-list, build block map
GUI->>UI: render heap bar, table, and errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
vsprojects/gui/gui.vcxproj.filters (1)
112-114: Place heap viewer entries under thewidgetsfilters for consistency.This keeps Visual Studio grouping aligned with the rest of widget files.
Diff suggestion
- <ClCompile Include="..\..\src\gui\widgets\heap_viewer.cc"> - <Filter>Source Files</Filter> + <ClCompile Include="..\..\src\gui\widgets\heap_viewer.cc"> + <Filter>Source Files\widgets</Filter> </ClCompile> @@ - <ClInclude Include="..\..\src\gui\widgets\heap_viewer.h"> - <Filter>Header Files</Filter> + <ClInclude Include="..\..\src\gui\widgets\heap_viewer.h"> + <Filter>Header Files\widgets</Filter> </ClInclude>Also applies to: 216-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vsprojects/gui/gui.vcxproj.filters` around lines 112 - 114, The project filter places heap_viewer.cc under "Source Files" rather than the "widgets" filter; update the <ClCompile Include="..\..\src\gui\widgets\heap_viewer.cc"> entry so its <Filter> is "widgets" (and similarly change the other occurrence noted around lines 216-218) to keep heap_viewer.cc grouped with other widget files in the VC++ filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 43-54: When walkHeap() reads Memory::m_psyqoHeapMetadata and uses
readGuest32(), treat a non-zero metaAddr that yields zeroed fields as a
read/metadata-corruption error instead of silently returning an empty
WalkResult; set result.error (a descriptive string mentioning metadata read
failure and metaAddr) and return that result so the caller/UI can show an
explicit error instead of "Heap not yet initialized." Concretely: in
PCSX::Widgets::HeapViewer::walkHeap(), after reading metaAddr and
headPtr/bottomPtr/topPtr via readGuest32(), if metaAddr != 0 and (headPtr==0 ||
bottomPtr==0 || topPtr==0) set result.error and return; also apply the same
check/update in the other heap-walking block later in the file (the similar code
around the second walkHeap-style logic) so both paths propagate metadata-read
errors instead of masking them as uninitialized heaps.
- Around line 76-80: The loop currently treats curr == 0 as a benign terminator;
instead, when iterating the free list starting from headPtr and encountering
curr == 0 (a smashed next pointer rather than markerPtr), set result.error to
indicate heap corruption and stop traversal (do not silently exit as a valid
terminator). Modify the loop around the while over curr/markerPtr/freeBlocks so
it no longer allows curr == 0 as a normal exit; explicitly check if curr == 0
inside the loop and set result.error (with a short descriptive message/context),
then break/return so the tail isn't misreported as allocated; keep using
markerPtr, freeBlocks and maxFreeBlocks as-is.
In `@src/mips/psyqo/src/alloc.c`:
- Around line 108-113: The macro alias '#define top s_heap_metadata.top'
collides with the parameter name 'top' in the function check_subintegrity;
rename the function parameter to top_block (in check_subintegrity signature) and
update all references inside that function (the three places currently using
'top') to use top_block instead so the macro won't be expanded; keep the macro
aliases unchanged and only adjust the parameter and its uses in
check_subintegrity.
---
Nitpick comments:
In `@vsprojects/gui/gui.vcxproj.filters`:
- Around line 112-114: The project filter places heap_viewer.cc under "Source
Files" rather than the "widgets" filter; update the <ClCompile
Include="..\..\src\gui\widgets\heap_viewer.cc"> entry so its <Filter> is
"widgets" (and similarly change the other occurrence noted around lines 216-218)
to keep heap_viewer.cc grouped with other widget files in the VC++ filters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d99ea1e-a461-4af8-989f-a7963a5fe280
📒 Files selected for processing (11)
src/core/psxhw.ccsrc/core/psxmem.ccsrc/core/psxmem.hsrc/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/heap_viewer.ccsrc/gui/widgets/heap_viewer.hsrc/mips/common/hardware/pcsxhw.hsrc/mips/psyqo/src/alloc.cvsprojects/gui/gui.vcxprojvsprojects/gui/gui.vcxproj.filters
nicolasnoble
force-pushed
the
psyqo-heap-viewer
branch
from
8eb643f to
4f52238
Compare
Clicking a block in the visual bar or a row in the block table fires a JumpToMemory event that opens the main memory editor at that location. For allocated blocks, the jump targets the user data (past the 8-byte header). For free blocks, it targets the block start. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
nicolasnoble
force-pushed
the
psyqo-heap-viewer
branch
from
4f52238 to
bc60f0c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/gui/widgets/heap_viewer.cc (1)
47-55:⚠️ Potential issue | 🟡 MinorNon-zero
metaAddrpointing to unmapped memory is silently treated as uninitialized.When
m_psyqoHeapMetadatais non-zero but points to unmapped or corrupted memory,readGuest32()returns 0 for all fields. Line 55 then returns an emptyWalkResultwithout an error, causing the UI to display "Heap not yet initialized" instead of surfacing the corruption.Consider distinguishing between "no heap registered" (
metaAddr == 0) and "metadata unreadable" (metaAddr != 0but fields are zeroed or unmapped).Suggested approach
uint32_t metaAddr = memory->m_psyqoHeapMetadata; if (metaAddr == 0) return result; + // Check if metadata is actually readable + if (!memory->getPointer(metaAddr)) { + result.error = fmt::format("Heap metadata at {:08x} is unreadable.", metaAddr); + return result; + } + uint32_t headPtr = readGuest32(memory, metaAddr + 0); uint32_t bottomPtr = readGuest32(memory, metaAddr + 4); uint32_t topPtr = readGuest32(memory, metaAddr + 8); uint32_t markerPtr = readGuest32(memory, metaAddr + 16); - if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) return result; + if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) { + result.error = fmt::format("Heap metadata at {:08x} contains invalid pointers.", metaAddr); + return result; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` around lines 47 - 55, The code currently treats a non-zero m_psyqoHeapMetadata that reads back zeros via readGuest32 as "uninitialized"; instead detect unreadable/corrupted metadata by checking metaAddr != 0 and validating the reads (e.g. use an API like memory->isMapped(metaAddr) or change readGuest32 to return a success flag/optional) and if reads for headPtr/bottomPtr/topPtr/markerPtr are all zero or the mapping check fails, return a WalkResult that indicates an unreadable/corrupted metadata error (or log and surface an error) rather than the "Heap not yet initialized" path; update the logic around metaAddr, readGuest32, bottomPtr/topPtr, and the WalkResult return so the UI can distinguish "no heap registered" (metaAddr == 0) from "metadata unreadable" (metaAddr != 0 but reads failed).
🧹 Nitpick comments (1)
src/gui/widgets/heap_viewer.cc (1)
173-214: Consider early exit on metadata read errors.When
result.erroris set bywalkHeap()butresult.blocksis empty (e.g., metadata validation failed before any blocks were collected), the UI shows "Heap not yet initialized" which may be misleading. The error message at line 204 is shown but immediately followed by the generic uninitialized message.Suggested refinement
if (blocks.empty()) { - ImGui::TextUnformatted("Heap not yet initialized."); + if (result.error.empty()) { + ImGui::TextUnformatted("Heap not yet initialized."); + } + // If error is set, it was already displayed above ImGui::End(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` around lines 173 - 214, The draw method in PCSX::Widgets::HeapViewer should avoid showing the generic "Heap not yet initialized." when walkHeap() returned an error but no blocks; modify HeapViewer::draw to check after calling walkHeap() whether result.error is non-empty and result.blocks is empty, and in that case keep the displayed error (result.error) and perform an early exit (call ImGui::End() then return) instead of falling through to the generic message; ensure you reference result.error and result.blocks from the walkHeap() return, and preserve proper ImGui state cleanup (ImGui::End()) before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 47-55: The code currently treats a non-zero m_psyqoHeapMetadata
that reads back zeros via readGuest32 as "uninitialized"; instead detect
unreadable/corrupted metadata by checking metaAddr != 0 and validating the reads
(e.g. use an API like memory->isMapped(metaAddr) or change readGuest32 to return
a success flag/optional) and if reads for headPtr/bottomPtr/topPtr/markerPtr are
all zero or the mapping check fails, return a WalkResult that indicates an
unreadable/corrupted metadata error (or log and surface an error) rather than
the "Heap not yet initialized" path; update the logic around metaAddr,
readGuest32, bottomPtr/topPtr, and the WalkResult return so the UI can
distinguish "no heap registered" (metaAddr == 0) from "metadata unreadable"
(metaAddr != 0 but reads failed).
---
Nitpick comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 173-214: The draw method in PCSX::Widgets::HeapViewer should avoid
showing the generic "Heap not yet initialized." when walkHeap() returned an
error but no blocks; modify HeapViewer::draw to check after calling walkHeap()
whether result.error is non-empty and result.blocks is empty, and in that case
keep the displayed error (result.error) and perform an early exit (call
ImGui::End() then return) instead of falling through to the generic message;
ensure you reference result.error and result.blocks from the walkHeap() return,
and preserve proper ImGui state cleanup (ImGui::End()) before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 747f7fcf-83a2-465f-a4cf-d65ec507d055
📒 Files selected for processing (9)
src/core/psxhw.ccsrc/core/psxmem.ccsrc/core/psxmem.hsrc/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/heap_viewer.ccsrc/gui/widgets/heap_viewer.hsrc/mips/common/hardware/pcsxhw.hsrc/mips/psyqo/src/alloc.c
✅ Files skipped from review due to trivial changes (1)
- src/gui/gui.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mips/common/hardware/pcsxhw.h
- src/gui/gui.h
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/mips/psyqo/src/alloc.c (1)
109-113:⚠️ Potential issue | 🔴 Critical
topmacro alias breaksALLOC_DEBUGpath due to identifier collision.The alias
#define top s_heap_metadata.toprewrites thecheck_subintegrity(..., top, ...)parameter and its uses, which breaks the debug build path.💡 Proposed fix
-static int check_subintegrity(const allocated_block *first, const allocated_block *top, size_t size_start, +static int check_subintegrity(const allocated_block *first, const allocated_block *top_block, size_t size_start, size_t hypothetical_size) { - if (first == top) { + if (first == top_block) { return 0; } - printf("Integrity check: checking sublist from %p to %p, size_start = %u, hypothetical_size: %u\n", first, top, + printf("Integrity check: checking sublist from %p to %p, size_start = %u, hypothetical_size: %u\n", first, + top_block, size_start, hypothetical_size); @@ - while (curr < top) { + while (curr < top_block) {#!/bin/bash set -euo pipefail python - <<'PY' import pathlib, re p = pathlib.Path("src/mips/psyqo/src/alloc.c") s = p.read_text() has_macro = bool(re.search(r'^\s*#define\s+top\s+s_heap_metadata\.top\b', s, re.M)) m = re.search(r'static\s+int\s+check_subintegrity\s*\((.*?)\)\s*{', s, re.S) has_param_top = bool(m and re.search(r'\bconst\s+allocated_block\s*\*\s*top\b', m.group(1))) print("macro_top_alias:", has_macro) print("check_subintegrity_param_top:", has_param_top) print("collision_present:", has_macro and has_param_top) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/psyqo/src/alloc.c` around lines 109 - 113, The macro "#define top s_heap_metadata.top" collides with the parameter named top in check_subintegrity(...) and breaks the ALLOC_DEBUG path; remove or rename the macro to avoid shadowing (e.g., rename the macro to heap_top or s_top) and update all uses of the macro in this file to reference the new macro name or directly use s_heap_metadata.top, leaving the other aliases (head, bottom, maximum_heap_end, marker) intact and then rebuild with ALLOC_DEBUG to verify the collision is resolved.src/gui/widgets/heap_viewer.cc (2)
81-81:⚠️ Potential issue | 🟠 Major
curr == 0should be reported as free-list corruption.Treating
0as a normal loop terminator can hide a smashednextpointer and misclassify trailing heap space.💡 Proposed fix
- while (curr != markerPtr && curr != 0 && (int)freeBlocks.size() < maxFreeBlocks) { + while (curr != markerPtr && (int)freeBlocks.size() < maxFreeBlocks) { + if (curr == 0) { + result.error = "Free list terminated with NULL instead of marker."; + break; + } // Free block must be within heap range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` at line 81, The loop that walks the free list (while (curr != markerPtr && curr != 0 && (int)freeBlocks.size() < maxFreeBlocks)) treats curr == 0 as a normal terminator; change this so a null next (curr == 0) is considered free-list corruption: remove curr != 0 from the normal loop condition and instead check curr == 0 immediately when encountered (in the loop body or right after advancing), log/report corruption (e.g., via the existing GUI logging/reporting mechanism), mark the free-list as corrupted or set an error flag, and stop walking to avoid misclassifying trailing heap space; reference the variables curr, markerPtr, freeBlocks and maxFreeBlocks when making the change.
55-55:⚠️ Potential issue | 🟠 MajorNon-zero metadata with invalid fields is still masked as “not initialized.”
When metadata is present but invalid/unreadable,
walkHeap()returns an empty success-like result, anddraw()falls back to “Heap not yet initialized.” This hides corruption/read failures.💡 Proposed fix
- if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) return result; + if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) { + result.error = fmt::format("Heap metadata at {:08x} is invalid or unreadable.", metaAddr); + return result; + }- if (blocks.empty()) { + if (blocks.empty()) { + if (!result.error.empty()) { + ImGui::End(); + return; + } ImGui::TextUnformatted("Heap not yet initialized."); ImGui::End(); return; }Also applies to: 210-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` at line 55, The current check in walkHeap() (using bottomPtr, topPtr) treats invalid-but-nonzero metadata as "not initialized" by returning an empty success-like result; change the logic so that only the case where both pointers are zero is considered uninitialized, and any other invalid combination (bottomPtr==0 || topPtr==0 || bottomPtr>=topPtr) sets and returns an explicit error/invalid status on the result (e.g., result.error or result.status = MetadataCorrupt) instead of an empty success; update draw() to inspect that error/status and show a corruption/read-failure message instead of "Heap not yet initialized." Apply the same fix for the similar check around lines 210-213.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Line 81: The loop that walks the free list (while (curr != markerPtr && curr
!= 0 && (int)freeBlocks.size() < maxFreeBlocks)) treats curr == 0 as a normal
terminator; change this so a null next (curr == 0) is considered free-list
corruption: remove curr != 0 from the normal loop condition and instead check
curr == 0 immediately when encountered (in the loop body or right after
advancing), log/report corruption (e.g., via the existing GUI logging/reporting
mechanism), mark the free-list as corrupted or set an error flag, and stop
walking to avoid misclassifying trailing heap space; reference the variables
curr, markerPtr, freeBlocks and maxFreeBlocks when making the change.
- Line 55: The current check in walkHeap() (using bottomPtr, topPtr) treats
invalid-but-nonzero metadata as "not initialized" by returning an empty
success-like result; change the logic so that only the case where both pointers
are zero is considered uninitialized, and any other invalid combination
(bottomPtr==0 || topPtr==0 || bottomPtr>=topPtr) sets and returns an explicit
error/invalid status on the result (e.g., result.error or result.status =
MetadataCorrupt) instead of an empty success; update draw() to inspect that
error/status and show a corruption/read-failure message instead of "Heap not yet
initialized." Apply the same fix for the similar check around lines 210-213.
In `@src/mips/psyqo/src/alloc.c`:
- Around line 109-113: The macro "#define top s_heap_metadata.top" collides with
the parameter named top in check_subintegrity(...) and breaks the ALLOC_DEBUG
path; remove or rename the macro to avoid shadowing (e.g., rename the macro to
heap_top or s_top) and update all uses of the macro in this file to reference
the new macro name or directly use s_heap_metadata.top, leaving the other
aliases (head, bottom, maximum_heap_end, marker) intact and then rebuild with
ALLOC_DEBUG to verify the collision is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc987f87-44be-44aa-bca6-d933497a3d09
📒 Files selected for processing (11)
src/core/psxhw.ccsrc/core/psxmem.ccsrc/core/psxmem.hsrc/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/heap_viewer.ccsrc/gui/widgets/heap_viewer.hsrc/mips/common/hardware/pcsxhw.hsrc/mips/psyqo/src/alloc.cvsprojects/gui/gui.vcxprojvsprojects/gui/gui.vcxproj.filters
✅ Files skipped from review due to trivial changes (3)
- vsprojects/gui/gui.vcxproj.filters
- src/gui/gui.cc
- vsprojects/gui/gui.vcxproj
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/psxmem.h
- src/mips/common/hardware/pcsxhw.h
# Conflicts: # src/gui/gui.h # vsprojects/gui/gui.vcxproj.filters
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/mips/psyqo/src/alloc.c (1)
108-113:⚠️ Potential issue | 🔴 CriticalRename the
topparameter used underALLOC_DEBUG.Line 111 defines
topas a macro, so thecheck_subintegrity(..., const allocated_block *top, ...)parameter at Line 131 is preprocessed into an invalid member-expression token sequence in debug builds.🐛 Proposed fix
-static int check_subintegrity(const allocated_block *first, const allocated_block *top, size_t size_start, +static int check_subintegrity(const allocated_block *first, const allocated_block *top_block, size_t size_start, size_t hypothetical_size) { - if (first == top) { + if (first == top_block) { return 0; } - printf("Integrity check: checking sublist from %p to %p, size_start = %u, hypothetical_size: %u\n", first, top, + printf("Integrity check: checking sublist from %p to %p, size_start = %u, hypothetical_size: %u\n", first, top_block, size_start, hypothetical_size); const allocated_block *curr = first; size_t size = size_start; - while (curr < top) { + while (curr < top_block) {Verification:
#!/bin/bash set -euo pipefail python - <<'PY' import pathlib import re p = pathlib.Path("src/mips/psyqo/src/alloc.c") s = p.read_text() macros = { m.group(1) for m in re.finditer(r'^\s*#define\s+([A-Za-z_]\w*)\b', s, re.M) } m = re.search(r'static\s+int\s+check_subintegrity\s*\(([^)]*)\)', s, re.S) if not m: raise SystemExit("check_subintegrity signature not found") params = m.group(1) hits = sorted(name for name in macros if re.search(rf'\b{name}\b', params)) print("Macro names reused in check_subintegrity parameters:", hits) if hits: raise SystemExit(1) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/psyqo/src/alloc.c` around lines 108 - 113, The function parameter name "top" in check_subintegrity conflicts with the `#define` macro top, causing invalid preprocessing in debug builds; rename the parameter to a non-macro name (e.g., top_block or top_alloc) in the check_subintegrity signature and update every use inside check_subintegrity and its declarations/callers (references to the old "top" symbol) to the new identifier so the parameter no longer expands into the macro. Ensure the new name is used consistently in any prototypes, definitions, and calls related to check_subintegrity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/mips/psyqo/src/alloc.c`:
- Around line 108-113: The function parameter name "top" in check_subintegrity
conflicts with the `#define` macro top, causing invalid preprocessing in debug
builds; rename the parameter to a non-macro name (e.g., top_block or top_alloc)
in the check_subintegrity signature and update every use inside
check_subintegrity and its declarations/callers (references to the old "top"
symbol) to the new identifier so the parameter no longer expands into the macro.
Ensure the new name is used consistently in any prototypes, definitions, and
calls related to check_subintegrity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74e00f3c-9cc1-4f38-8b87-4f69f47cb362
📒 Files selected for processing (5)
src/gui/gui.ccsrc/gui/gui.hsrc/mips/psyqo/src/alloc.cvsprojects/gui/gui.vcxprojvsprojects/gui/gui.vcxproj.filters
✅ Files skipped from review due to trivial changes (2)
- src/gui/gui.cc
- vsprojects/gui/gui.vcxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- vsprojects/gui/gui.vcxproj.filters
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/gui/widgets/heap_viewer.cc (2)
250-250: "Fragmentation: %u fragments" just re-prints the free-block count.This is the same number already shown as "Free: … (%u blocks)" one line above, so it doesn't convey fragmentation. Either drop it or compute something meaningful, e.g.
1 - largestFree/totalFreeas a fragmentation ratio, or the count of free blocks that are not adjacent to the top/bottom.
56-61: Diagnostic message doesn't match the condition, and line 61 is partially dead.
readGuest32returnsnulloptonly when the metadata struct itself is unmapped — not when the pointer fields happen to be 0. The message on line 57 ("contains invalid pointers") describes the wrong failure mode, and on line 61 thebottomPtr == 0 || topPtr == 0subchecks silently swallow a genuinely zero-valuedbottom/topas "not initialized" whilebottomPtr >= topPtris left with no diagnostic. Consider:Proposed fix
- if (!headPtr || !bottomPtr || !topPtr) { - result.error = "Heap metadata contains invalid pointers - likely corrupted metadata."; - return result; - } - - if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) return result; + if (!headPtr || !bottomPtr || !topPtr) { + result.error = fmt::format("Heap metadata at {:08x} is unreadable.", metaAddr); + return result; + } + + // Not-yet-initialized heap: bottom/top still zero. No error, just empty result. + if (*bottomPtr == 0 && *topPtr == 0) return result; + + if (*bottomPtr >= *topPtr) { + result.error = fmt::format("Heap bottom ({:08x}) is not below top ({:08x}).", *bottomPtr, *topPtr); + return result; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` around lines 56 - 61, The current checks conflate an unmapped metadata read (nullopt) with zero-valued pointer fields and silently swallow some errors; update the logic that inspects headPtr/bottomPtr/topPtr (from readGuest32) so that you first test for nullopt (e.g., if (!headPtr.has_value() || !bottomPtr.has_value() || !topPtr.has_value())) and set result.error to indicate the metadata struct is unmapped, then separately handle valid reads: if bottomPtr == 0 || topPtr == 0 set result.error to "heap not initialized" (or similar), and if bottomPtr >= topPtr set result.error to "heap metadata corrupted: bottom >= top"; reference the variables headPtr, bottomPtr, topPtr and ensure each branch returns result with the correct, distinct diagnostic instead of the mixed message currently present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 116-121: The bounds check using "if (curr + *size > *topPtr)" can
overflow due to uint32_t wraparound; replace it with the safe form used later so
you compare sizes instead of summed addresses — i.e., check "*size > (*topPtr -
curr)" (mirror the check at line 147) and set result.error the same way when the
condition is true, referencing curr, *size and *topPtr and the existing
result.error message construction in heap_viewer.cc.
---
Nitpick comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 56-61: The current checks conflate an unmapped metadata read
(nullopt) with zero-valued pointer fields and silently swallow some errors;
update the logic that inspects headPtr/bottomPtr/topPtr (from readGuest32) so
that you first test for nullopt (e.g., if (!headPtr.has_value() ||
!bottomPtr.has_value() || !topPtr.has_value())) and set result.error to indicate
the metadata struct is unmapped, then separately handle valid reads: if
bottomPtr == 0 || topPtr == 0 set result.error to "heap not initialized" (or
similar), and if bottomPtr >= topPtr set result.error to "heap metadata
corrupted: bottom >= top"; reference the variables headPtr, bottomPtr, topPtr
and ensure each branch returns result with the correct, distinct diagnostic
instead of the mixed message currently present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2f98545-e659-4d37-8937-819f3150b35f
📒 Files selected for processing (1)
src/gui/widgets/heap_viewer.cc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/gui/widgets/heap_viewer.cc (2)
122-127:⚠️ Potential issue | 🟡 MinorUse subtraction-form bounds checking to avoid
uint32_twraparound.Line 123 can wrap when corrupted guest data gives a huge size, letting an invalid free block pass the range check.
Suggested fix
- // Block must not extend past top. - if (curr + *size > *topPtr) { + // Block must not extend past top. Use subtraction to avoid uint32_t wraparound. + if (*size > (*topPtr - curr)) { result.error = fmt::format("Free block at {:08x} (size {}) extends past heap top {:08x}.", curr, *size, *topPtr); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` around lines 122 - 127, The current bounds check in heap_viewer.cc uses "curr + *size > *topPtr" which can wrap for uint32_t; replace it with subtraction-form checks to avoid wraparound by ensuring curr <= *topPtr and *size <= *topPtr - curr (or equivalently check if curr > *topPtr || *size > *topPtr - curr) before accepting the block; update the error path that sets result.error (the fmt::format call) so it still runs when the subtraction-form check fails and preserves the same diagnostic text referencing curr, *size, and *topPtr.
61-61:⚠️ Potential issue | 🟠 MajorReturn an error for invalid metadata instead of showing “not initialized.”
Line 61 still turns malformed registered metadata into an empty result, and Lines 227-232 then show the normal uninitialized fallback even after
walkHeap()set an error. That masks corruption diagnostics.Suggested fix
- if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) return result; + if (*bottomPtr == 0 || *topPtr == 0 || *bottomPtr >= *topPtr) { + result.error = fmt::format("Heap metadata at {:08x} has invalid heap bounds: bottom={:08x}, top={:08x}.", + metaAddr, *bottomPtr, *topPtr); + return result; + } @@ - if (blocks.empty()) { + if (blocks.empty() && result.error.empty()) { ImGui::TextUnformatted("Heap not yet initialized."); ImGui::End(); return; } + if (blocks.empty()) { + ImGui::End(); + return; + }Also applies to: 227-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` at line 61, The check in walkHeap that currently does "if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) return result;" must return/propagate an error state instead of an empty result so malformed metadata is reported; change walkHeap to set an error on the returned result (or throw) with a descriptive message when bottomPtr/topPtr are invalid, and update the caller that currently displays the fallback "not initialized" (the code showing "not initialized" around the earlier UI branch) to prefer and display the walkHeap error when present; reference the symbols walkHeap, bottomPtr, topPtr, and result and ensure the UI code that produces the "not initialized" message checks for result.error first and shows that error instead of the uninitialized fallback.
🧹 Nitpick comments (1)
src/mips/psyqo/src/alloc.c (1)
94-106: Lock down the heap metadata ABI with static assertions.The viewer hard-codes this struct’s guest layout, so a future field/type/layout change could silently break heap decoding. Please add compile-time offset checks next to the struct definition.
Suggested guardrails
-// Heap metadata struct, registered with the emulator so -// it can walk the allocator state for the heap viewer. -static struct { +// Heap metadata struct, registered with the emulator so +// it can walk the allocator state for the heap viewer. +typedef struct heap_metadata_ { empty_block *head; allocated_block *bottom; allocated_block *top; void *maximum_heap_end; @@ // be the last block in the list. empty_block marker; -} s_heap_metadata = {NULL, NULL, NULL, NULL, NULL}; +} heap_metadata; + +_Static_assert(sizeof(void *) == 4, "heap metadata viewer expects 32-bit guest pointers"); +_Static_assert(offsetof(heap_metadata, head) == 0, "heap metadata head offset changed"); +_Static_assert(offsetof(heap_metadata, bottom) == 4, "heap metadata bottom offset changed"); +_Static_assert(offsetof(heap_metadata, top) == 8, "heap metadata top offset changed"); +_Static_assert(offsetof(heap_metadata, maximum_heap_end) == 12, "heap metadata high-water offset changed"); +_Static_assert(offsetof(heap_metadata, marker) == 16, "heap metadata marker offset changed"); + +static heap_metadata s_heap_metadata = {NULL, NULL, NULL, NULL, {NULL, 0}};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mips/psyqo/src/alloc.c` around lines 94 - 106, Add compile-time assertions that lock the guest-visible layout of the heap metadata: use _Static_assert and offsetof to verify offsets of members head, bottom, top, maximum_heap_end, and marker within the anonymous struct instance s_heap_metadata (and assert sizeof(s_heap_metadata) and sizeof(marker) if needed). Place these assertions immediately after the struct definition so any change to empty_block, allocated_block, or the struct order/type will fail compilation; reference the member names (head, bottom, top, maximum_heap_end, marker) and the types empty_block and allocated_block in the assertions to ensure the ABI remains fixed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 113-131: The code dereferences next (from readGuest32) without
checking for std::nullopt, which can cause UB; before using *next (and before
pushing the block or setting curr = *next), verify next.has_value(), and if not
set result.error to a corruption message like "Free block at {curr} has
unreadable next pointer" (or similar) and break; update the block-parsing logic
that references readGuest32, next, curr, freeBlocks, and result.error to perform
this check so unreadable headers are reported instead of being dereferenced.
---
Duplicate comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 122-127: The current bounds check in heap_viewer.cc uses "curr +
*size > *topPtr" which can wrap for uint32_t; replace it with subtraction-form
checks to avoid wraparound by ensuring curr <= *topPtr and *size <= *topPtr -
curr (or equivalently check if curr > *topPtr || *size > *topPtr - curr) before
accepting the block; update the error path that sets result.error (the
fmt::format call) so it still runs when the subtraction-form check fails and
preserves the same diagnostic text referencing curr, *size, and *topPtr.
- Line 61: The check in walkHeap that currently does "if (bottomPtr == 0 ||
topPtr == 0 || bottomPtr >= topPtr) return result;" must return/propagate an
error state instead of an empty result so malformed metadata is reported; change
walkHeap to set an error on the returned result (or throw) with a descriptive
message when bottomPtr/topPtr are invalid, and update the caller that currently
displays the fallback "not initialized" (the code showing "not initialized"
around the earlier UI branch) to prefer and display the walkHeap error when
present; reference the symbols walkHeap, bottomPtr, topPtr, and result and
ensure the UI code that produces the "not initialized" message checks for
result.error first and shows that error instead of the uninitialized fallback.
---
Nitpick comments:
In `@src/mips/psyqo/src/alloc.c`:
- Around line 94-106: Add compile-time assertions that lock the guest-visible
layout of the heap metadata: use _Static_assert and offsetof to verify offsets
of members head, bottom, top, maximum_heap_end, and marker within the anonymous
struct instance s_heap_metadata (and assert sizeof(s_heap_metadata) and
sizeof(marker) if needed). Place these assertions immediately after the struct
definition so any change to empty_block, allocated_block, or the struct
order/type will fail compilation; reference the member names (head, bottom, top,
maximum_heap_end, marker) and the types empty_block and allocated_block in the
assertions to ensure the ABI remains fixed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26e5df92-eb4e-41b6-9446-f168865c01bf
📒 Files selected for processing (2)
src/gui/widgets/heap_viewer.ccsrc/mips/psyqo/src/alloc.c
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/gui/widgets/heap_viewer.cc (1)
61-61:⚠️ Potential issue | 🟠 MajorReport inverted heap ranges instead of underflowing the UI.
A non-zero
bottom >= topcurrently returns an empty walk result, while Line 217 can still display*topPtr - *bottomPtras a huge wrapped size. Treat inverted non-zero ranges as metadata corruption and only compute the displayed heap size after validating the range.🛡️ Proposed fix
- if (bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr) return result; + if (*bottomPtr == 0 || *topPtr == 0) return result; + if (*bottomPtr >= *topPtr) { + result.error = fmt::format("Heap metadata has invalid range [{:08x}, {:08x}).", *bottomPtr, *topPtr); + return result; + }- ImGui::Text("Heap range: %08x - %08x (%u bytes)", bottomPtr ? *bottomPtr : 0, topPtr ? *topPtr : 0, bottomPtr && topPtr ? *topPtr - *bottomPtr : 0); + const bool validHeapRange = bottomPtr && topPtr && *bottomPtr != 0 && *topPtr != 0 && *bottomPtr < *topPtr; + const uint32_t displayedHeapSize = validHeapRange ? *topPtr - *bottomPtr : 0; + ImGui::Text("Heap range: %08x - %08x (%u bytes)", bottomPtr ? *bottomPtr : 0, topPtr ? *topPtr : 0, + displayedHeapSize);Also applies to: 217-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/heap_viewer.cc` at line 61, The code currently returns an empty walk result for non-zero inverted ranges (bottomPtr >= topPtr) but later computes displayed size using "*topPtr - *bottomPtr", which underflows the UI; update the validation so that if bottomPtr == 0 || topPtr == 0 || bottomPtr >= topPtr you treat this as metadata corruption and bail out before any size computation. Concretely, in the function using bottomPtr, topPtr and result, ensure the early-return condition is unchanged or extended to flag corruption and that any code that computes displayed heap size (the "*topPtr - *bottomPtr" calculation referenced around lines 217-222) only runs after confirming bottomPtr < topPtr; do not compute or display a wrapped/huge size for inverted non-zero ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Around line 312-317: Allocated 8-byte allocated blocks currently jump past
their data because jumpAddr is set to b.address + 8 and jumpSize to b.size;
change the allocation branch so that for allocated blocks with b.size > 8 you
set jumpAddr = b.address + 8 and jumpSize = b.size - 8, but for allocated blocks
with b.size <= 8 fall back to the block header (jumpAddr = b.address, jumpSize =
b.size); keep the free-block behavior unchanged and emit the
Events::GUI::JumpToMemory with the same 0x80000000 flag.
- Around line 39-42: readGuest32 currently dereferences
memory->getPointer<uint32_t>(addr) which can cause undefined behavior for
unaligned guest addresses; modify readGuest32 to first check that addr is
aligned to 4 bytes (addr % alignof(uint32_t) == 0) and only use
getPointer<uint32_t> when aligned, otherwise perform a safe copy into a local
uint32_t (e.g. obtain raw bytes from the guest via memory APIs or
getPointer<uint8_t> and memcpy into a uint32_t) and return that value; keep
returning std::nullopt if the underlying memory read fails. Ensure references to
readGuest32 and PCSX::Memory are updated accordingly.
---
Duplicate comments:
In `@src/gui/widgets/heap_viewer.cc`:
- Line 61: The code currently returns an empty walk result for non-zero inverted
ranges (bottomPtr >= topPtr) but later computes displayed size using "*topPtr -
*bottomPtr", which underflows the UI; update the validation so that if bottomPtr
== 0 || topPtr == 0 || bottomPtr >= topPtr you treat this as metadata corruption
and bail out before any size computation. Concretely, in the function using
bottomPtr, topPtr and result, ensure the early-return condition is unchanged or
extended to flag corruption and that any code that computes displayed heap size
(the "*topPtr - *bottomPtr" calculation referenced around lines 217-222) only
runs after confirming bottomPtr < topPtr; do not compute or display a
wrapped/huge size for inverted non-zero ranges.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06435373-a11c-45f3-b506-0f860d0f8754
📒 Files selected for processing (1)
src/gui/widgets/heap_viewer.cc
Comment on lines
+39
to
+42
| static std::optional<uint32_t> readGuest32(PCSX::Memory* memory, uint32_t addr) { | ||
| auto* ptr = memory->getPointer<uint32_t>(addr); | ||
| if (!ptr) return std::nullopt; | ||
| return *ptr; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect readGuest32 and its call sites for raw typed dereferences and missing alignment checks.
rg -n -C3 'static std::optional<uint32_t> readGuest32|readGuest32\(memory, (metaAddr|curr|pos)' src/gui/widgets/heap_viewer.ccRepository: grumpycoders/pcsx-redux
Length of output: 1810
Avoid undefined behavior when reading unaligned guest 32-bit fields.
The readGuest32() function dereferences an arbitrary guest address as uint32_t*. If guest heap metadata is corrupted, curr or pos pointers passed to this function may be unaligned, triggering undefined behavior on some architectures before the viewer can report corruption.
Callers validate block size alignment but not address alignment. The function should explicitly check alignment and use a safe copy mechanism.
🛡️ Proposed fix
`#include` "gui/widgets/heap_viewer.h"
+#include <cstring>
`#include` <optional>
static std::optional<uint32_t> readGuest32(PCSX::Memory* memory, uint32_t addr) {
- auto* ptr = memory->getPointer<uint32_t>(addr);
- if (!ptr) return std::nullopt;
- return *ptr;
+ if ((addr & 3) != 0) return std::nullopt;
+
+ auto* ptr = memory->getPointer<uint8_t>(addr);
+ auto* end = memory->getPointer<uint8_t>(addr + sizeof(uint32_t) - 1);
+ if (!ptr || !end) return std::nullopt;
+
+ uint32_t value;
+ std::memcpy(&value, ptr, sizeof(value));
+ return value;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/widgets/heap_viewer.cc` around lines 39 - 42, readGuest32 currently
dereferences memory->getPointer<uint32_t>(addr) which can cause undefined
behavior for unaligned guest addresses; modify readGuest32 to first check that
addr is aligned to 4 bytes (addr % alignof(uint32_t) == 0) and only use
getPointer<uint32_t> when aligned, otherwise perform a safe copy into a local
uint32_t (e.g. obtain raw bytes from the guest via memory APIs or
getPointer<uint8_t> and memcpy into a uint32_t) and return that value; keep
returning std::nullopt if the underlying memory read fails. Ensure references to
readGuest32 and PCSX::Memory are updated accordingly.
Comment on lines
+312
to
+317
| // For allocated blocks, jump to user data (past the 8-byte header). | ||
| // For free blocks, jump to the block start. | ||
| uint32_t jumpAddr = b.free ? b.address : b.address + 8; | ||
| uint32_t jumpSize = b.free ? b.size : (b.size > 8 ? b.size - 8 : b.size); | ||
| g_system->m_eventBus->signal( | ||
| Events::GUI::JumpToMemory{jumpAddr | 0x80000000, jumpSize}); |
Contributor
There was a problem hiding this comment.
Don’t jump past 8-byte allocated blocks.
For allocated blocks with size == 8, jumpAddr becomes address + 8 while jumpSize is 8, so the memory viewer highlights bytes after the block. Fall back to the block header when there is no payload.
🐛 Proposed fix
- uint32_t jumpAddr = b.free ? b.address : b.address + 8;
- uint32_t jumpSize = b.free ? b.size : (b.size > 8 ? b.size - 8 : b.size);
+ uint32_t jumpAddr = b.address;
+ uint32_t jumpSize = b.size;
+ if (!b.free && b.size > 8) {
+ jumpAddr = b.address + 8;
+ jumpSize = b.size - 8;
+ }
g_system->m_eventBus->signal(
Events::GUI::JumpToMemory{jumpAddr | 0x80000000, jumpSize});- uint32_t jumpAddr = b.free ? b.address : b.address + 8;
- uint32_t jumpSize = b.free ? b.size : (b.size > 8 ? b.size - 8 : b.size);
+ uint32_t jumpAddr = b.address;
+ uint32_t jumpSize = b.size;
+ if (!b.free && b.size > 8) {
+ jumpAddr = b.address + 8;
+ jumpSize = b.size - 8;
+ }
g_system->m_eventBus->signal(Events::GUI::JumpToMemory{jumpAddr | 0x80000000, jumpSize});Also applies to: 360-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/widgets/heap_viewer.cc` around lines 312 - 317, Allocated 8-byte
allocated blocks currently jump past their data because jumpAddr is set to
b.address + 8 and jumpSize to b.size; change the allocation branch so that for
allocated blocks with b.size > 8 you set jumpAddr = b.address + 8 and jumpSize =
b.size - 8, but for allocated blocks with b.size <= 8 fall back to the block
header (jumpAddr = b.address, jumpSize = b.size); keep the free-block behavior
unchanged and emit the Events::GUI::JumpToMemory with the same 0x80000000 flag.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PSYQo's allocator now registers a metadata struct with the emulator via a new pcsxhw register (0x1f8020a0), giving Redux live access to the heap's free list head, bottom, top, high-water mark, and marker pointers. The viewer walks the free list from guest memory each frame, reconstructs the full block layout, and displays it as a visual bar map with a detailed block table.
The walker is hardened against memory corruption: bounds checks on every pointer read, free list ascending-order validation (catches cycles), size alignment and range checks, iteration caps, and accounting mismatch detection. Corruption is surfaced prominently in the UI with specific diagnostics.