Use IO<File> as data source for memory editors#2011
Conversation
Replace the raw void* data pointer API in the imgui memory editor with callback-based data access through IO<File>. This decouples the viewer from direct memory buffer access, allowing it to work with any File- derived source through the existing IO template system. The memory editor now uses std::function callbacks (ReadFn/WriteFn) for byte-level access, with a 4KB read-ahead cache (BulkReadFn) to batch individual byte reads into bulk readAt calls for performance. All non-VRAM editors (main memory, parallel port, scratch pad, hardware registers, BIOS) now go through getMemoryAsFile() with SubFile for address translation, instead of accessing raw m_wram/m_exp1/m_hard/ m_bios pointers directly. SubFile gains a FileOps::ReadWrite constructor and write/writeAt support, matching the existing tag-based pattern used by PosixFile and BufferFile for mutating file capabilities. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the memory editor to use offset-based Read/Write callbacks with a page cache, removes raw-memory rendering APIs, adds SubFile write support and write-pointer state, and updates GUI wrappers to bind editors to IO-backed Changes
Sequence Diagram(s)sequenceDiagram
participant GUI as GUI wrapper
participant MEM as MemoryEditor
participant CACHE as ReadCache
participant SUB as SubFile
participant FILE as File
GUI->>MEM: set ReadFn/WriteFn/Cache.BulkReadFn\nDrawWindow(title, mem_size)
MEM->>MEM: DrawContents(mem_size)
loop per-byte display
MEM->>CACHE: read(addr)
alt cache has page
CACHE-->>MEM: byte
else cache miss
CACHE->>SUB: BulkReadFn(dest, off, len)
SUB->>FILE: readAt(off, len)
FILE-->>SUB: bytes
SUB-->>CACHE: fill page
CACHE-->>MEM: bytes
end
end
MEM->>MEM: user edits -> WriteFn(addr, value)
MEM->>SUB: writeAt(addr, data)
SUB->>FILE: underlying writeAt
MEM->>CACHE: Cache.invalidate()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
third_party/imgui_memory_editor/imgui_memory_editor.cpp (1)
564-570: Consider markingCacheasmutableto avoidconst_cast.The
const_castis needed becauseDrawPreviewDataisconstbutReadBytemodifies the cache. Since the cache is internal state that doesn't affect the logical constness of the object, markingCacheasmutablein the header would be cleaner.Suggested change in imgui_memory_editor.h
- ReadCache Cache; + mutable ReadCache Cache;Then remove the
const_castinDrawPreviewData:- buf[i] = const_cast<MemoryEditor*>(this)->ReadByte(addr + i); + buf[i] = ReadByte(addr + i);This would also require making
ReadByteaconstmethod.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp` around lines 564 - 570, The DrawPreviewData const method currently uses const_cast to call ReadByte because the internal Cache member is non-const; make this cleaner by declaring the Cache member as mutable in imgui_memory_editor.h and then updating ReadByte to be a const method (MemoryEditor::ReadByte() const) so it can access the mutable Cache without casting; finally remove the const_cast from DrawPreviewData in imgui_memory_editor.cpp and ensure any other methods that read from Cache are adjusted to const-correctness.
🤖 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/gui.cc`:
- Around line 1642-1667: The lambdas capture a pointer to VRAM returned by
g_emulator->m_gpu->getVRAM() which is freed when the local Slice (vram) is
destroyed, causing use-after-free; keep the acquired memory alive by storing the
Slice as a member (e.g., add a Slice m_vramSlice to m_vramEditor), assign
m_vramEditor.m_vramSlice = g_emulator->m_gpu->getVRAM(/*...*/), then use
m_vramEditor.m_vramSlice.data() / size() when initializing
m_vramEditor.editor.ReadFn, WriteFn and Cache.BulkReadFn (capture by
pointer/reference to the member, not the temporary local vram) so the underlying
buffer remains valid for the editor lifetime; alternatively, if you prefer
backend changes, make getVRAM() in the OpenGL backend respect the
Ownership::BORROW parameter instead of malloc+acquire.
In `@third_party/imgui_memory_editor/imgui_memory_editor.h`:
- Around line 97-125: ReadCache::fill currently computes m_len =
std::min(kPageSize, m_totalSize - m_base) which underflows if m_base >=
m_totalSize; update fill (and optionally read) to guard against out-of-range
offsets by checking if m_base >= m_totalSize and in that case set m_len = 0,
clear m_buf (or resize to 0), and return without calling BulkReadFn; ensure
read(size_t off) also treats offsets >= m_totalSize as invalid (e.g., return 0)
so BulkReadFn is never invoked with an invalid length or base.
---
Nitpick comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp`:
- Around line 564-570: The DrawPreviewData const method currently uses
const_cast to call ReadByte because the internal Cache member is non-const; make
this cleaner by declaring the Cache member as mutable in imgui_memory_editor.h
and then updating ReadByte to be a const method (MemoryEditor::ReadByte() const)
so it can access the mutable Cache without casting; finally remove the
const_cast from DrawPreviewData in imgui_memory_editor.cpp and ensure any other
methods that read from Cache are adjusted to const-correctness.
🪄 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: bb42a222-2cab-43a9-ad37-b41b92557c10
📒 Files selected for processing (6)
src/gui/gui.ccsrc/gui/gui.hsrc/support/file.ccsrc/support/file.hthird_party/imgui_memory_editor/imgui_memory_editor.cppthird_party/imgui_memory_editor/imgui_memory_editor.h
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
third_party/imgui_memory_editor/imgui_memory_editor.h (1)
9-20:⚠️ Potential issue | 🟡 MinorUpdate the usage examples for the callback API.
The examples still show the removed pointer-based
DrawWindow/DrawContentssignatures, so copy-pasted usage no longer compiles.Proposed documentation update
// static MemoryEditor mem_edit_1; // static char data[0x10000]; // size_t data_size = 0x10000; -// mem_edit_1.DrawWindow("Memory Editor", data, data_size); +// mem_edit_1.ReadFn = [](size_t off) { return static_cast<ImU8>(data[off]); }; +// mem_edit_1.WriteFn = [](size_t off, ImU8 d) { data[off] = static_cast<char>(d); }; +// mem_edit_1.DrawWindow("Memory Editor", data_size); @@ // static MemoryEditor mem_edit_2; // ImGui::Begin("MyWindow") -// mem_edit_2.DrawContents(this, sizeof(*this), (size_t)this); +// mem_edit_2.ReadFn = [this](size_t off) { return reinterpret_cast<const ImU8*>(this)[off]; }; +// mem_edit_2.WriteFn = [this](size_t off, ImU8 d) { reinterpret_cast<ImU8*>(this)[off] = d; }; +// mem_edit_2.DrawContents(sizeof(*this)); // ImGui::End();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/imgui_memory_editor/imgui_memory_editor.h` around lines 9 - 20, The examples show old pointer-based signatures for MemoryEditor::DrawWindow and MemoryEditor::DrawContents which were removed; update the usage docs to the current callback-based API by replacing calls like mem_edit.DrawWindow("...", data, data_size) and mem_edit.DrawContents(this, sizeof(*this), (size_t)this) with the new overloads that accept a callback/lambda providing a pointer and size (e.g. pass a lambda that captures your buffer and calls the MemoryEditor API inside it), and adjust the parameter order to match the current MemoryEditor methods so the examples compile against the updated MemoryEditor class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.h`:
- Around line 189-190: The DrawPreviewData method is marked const but calls the
non-const ReadByte via const_cast; fix by either removing const from
DrawPreviewData (make DrawPreviewData(...) non-const) or by making ReadByte
const-compatible: change ReadByte signature to ImU8 ReadByte(size_t addr) const
and adjust its cache members to be mutable (or encapsulate cache in a
mutable/const-safe wrapper) so the method can be called from const
DrawPreviewData without casting; update all callers and implementation in
imgui_memory_editor.cpp (including the cache update logic) to use the new
const-correct signatures.
---
Outside diff comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.h`:
- Around line 9-20: The examples show old pointer-based signatures for
MemoryEditor::DrawWindow and MemoryEditor::DrawContents which were removed;
update the usage docs to the current callback-based API by replacing calls like
mem_edit.DrawWindow("...", data, data_size) and mem_edit.DrawContents(this,
sizeof(*this), (size_t)this) with the new overloads that accept a
callback/lambda providing a pointer and size (e.g. pass a lambda that captures
your buffer and calls the MemoryEditor API inside it), and adjust the parameter
order to match the current MemoryEditor methods so the examples compile against
the updated MemoryEditor class.
🪄 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: f3230810-7e58-4592-a26a-271c3e1ff044
📒 Files selected for processing (1)
third_party/imgui_memory_editor/imgui_memory_editor.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
third_party/imgui_memory_editor/imgui_memory_editor.h (1)
9-20:⚠️ Potential issue | 🟡 MinorUpdate the usage examples for the new callback API.
The header still documents removed raw-pointer overloads (
DrawWindow(..., data, data_size)andDrawContents(..., this, sizeof(*this), ...)) even though the public API now takes onlymem_sizeand requiresReadFn/optionalWriteFn.Proposed documentation direction
-// mem_edit_1.DrawWindow("Memory Editor", data, data_size); +// mem_edit_1.ReadFn = [](size_t off) { return (ImU8)data[off]; }; +// mem_edit_1.WriteFn = [](size_t off, ImU8 value) { data[off] = (char)value; }; +// mem_edit_1.DrawWindow("Memory Editor", data_size);-// mem_edit_2.DrawContents(this, sizeof(*this), (size_t)this); +// mem_edit_2.ReadFn = [](size_t off) { return /* byte at off */; }; +// mem_edit_2.DrawContents(/* mem_size */);Also applies to: 172-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/imgui_memory_editor/imgui_memory_editor.h` around lines 9 - 20, The header comments still show old overloads for MemoryEditor::DrawWindow and MemoryEditor::DrawContents that accept raw data pointers and sizes; update those usage examples to the new callback-based API by showing how to construct a MemoryEditor and call DrawWindow/DrawContents with a mem_size and a ReadFn (and optional WriteFn) lambda or function object instead of passing raw data pointers (reference the MemoryEditor class, DrawWindow, DrawContents, ReadFn, WriteFn and mem_size in the examples); also update the duplicate examples around lines 172-174 to the same callback form so documentation matches the current public API.third_party/imgui_memory_editor/imgui_memory_editor.cpp (1)
443-471:⚠️ Potential issue | 🟠 MajorGuard preview reads against out-of-range addresses.
has_valueonly checks for(size_t)-1; ifDataPreviewAddrbecomes>= mem_sizeduring the same frame,mem_size - addrunderflows inDrawPreviewData()and can feed an oversized copy intoEndianessCopy().Proposed fix
- bool has_value = DataPreviewAddr != (size_t)-1; + bool has_value = DataPreviewAddr != (size_t)-1 && DataPreviewAddr < mem_size; if (has_value) DrawPreviewData(DataPreviewAddr, mem_size, PreviewDataType, DataFormat_Dec, buf, (size_t)IM_ARRAYSIZE(buf));void MemoryEditor::DrawPreviewData(size_t addr, size_t mem_size, ImGuiDataType data_type, DataFormat data_format, char* out_buf, size_t out_buf_size) const { - uint8_t buf[8]; + if (addr >= mem_size) { + if (out_buf_size > 0) + out_buf[0] = 0; + return; + } + + uint8_t buf[8] = {}; size_t elem_size = DataTypeGetSize(data_type); - size_t size = addr + elem_size > mem_size ? mem_size - addr : elem_size; + size_t size = std::min(elem_size, mem_size - addr); for (int i = 0, n = (int)size; i < n; ++i) buf[i] = ReadByte(addr + i);Also applies to: 564-570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp` around lines 443 - 471, The preview reads currently only check DataPreviewAddr != (size_t)-1 but not bounds against mem_size, which can cause underflow in DrawPreviewData; change the guard in MemoryEditor::DrawPreviewLine so has_value is true only when DataPreviewAddr != (size_t)-1 AND DataPreviewAddr < mem_size, and skip calling DrawPreviewData (and showing values) when out of range; apply the same fix to the other preview block (the one around lines 564-570) that uses DataPreviewAddr, DrawPreviewData, PreviewDataType and PreviewEndianess to ensure no out-of-range reads occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp`:
- Around line 443-471: The preview reads currently only check DataPreviewAddr !=
(size_t)-1 but not bounds against mem_size, which can cause underflow in
DrawPreviewData; change the guard in MemoryEditor::DrawPreviewLine so has_value
is true only when DataPreviewAddr != (size_t)-1 AND DataPreviewAddr < mem_size,
and skip calling DrawPreviewData (and showing values) when out of range; apply
the same fix to the other preview block (the one around lines 564-570) that uses
DataPreviewAddr, DrawPreviewData, PreviewDataType and PreviewEndianess to ensure
no out-of-range reads occur.
In `@third_party/imgui_memory_editor/imgui_memory_editor.h`:
- Around line 9-20: The header comments still show old overloads for
MemoryEditor::DrawWindow and MemoryEditor::DrawContents that accept raw data
pointers and sizes; update those usage examples to the new callback-based API by
showing how to construct a MemoryEditor and call DrawWindow/DrawContents with a
mem_size and a ReadFn (and optional WriteFn) lambda or function object instead
of passing raw data pointers (reference the MemoryEditor class, DrawWindow,
DrawContents, ReadFn, WriteFn and mem_size in the examples); also update the
duplicate examples around lines 172-174 to the same callback form so
documentation matches the current public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49cd5553-e87a-4f7a-a4d7-bba098c9e677
📒 Files selected for processing (2)
third_party/imgui_memory_editor/imgui_memory_editor.cppthird_party/imgui_memory_editor/imgui_memory_editor.h
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
third_party/imgui_memory_editor/imgui_memory_editor.cpp (1)
151-154:⚠️ Potential issue | 🟠 MajorTreat missing
WriteFnas non-editable.
WriteFnis optional, but the editor still allows byte editing whenReadOnly == falseandWriteFn == nullptr; the input is then silently discarded at Line 268. Gate edit mode on write capability so read-only/file-backed sources do not appear writable.Proposed fix
bool data_next = false; + const bool can_write = !ReadOnly && static_cast<bool>(WriteFn); - if (ReadOnly || DataEditingAddr >= mem_size) + if (!can_write || DataEditingAddr >= mem_size) DataEditingAddr = (size_t)-1; @@ - if (WriteFn) { - WriteFn(addr, (ImU8)data_input_value); - Cache.invalidate(); - } + WriteFn(addr, (ImU8)data_input_value); + Cache.invalidate(); @@ - if (!ReadOnly && ImGui::IsItemHovered() && ImGui::IsMouseClicked(0)) + if (can_write && ImGui::IsItemHovered() && ImGui::IsMouseClicked(0)) { DataEditingTakeFocus = true; data_editing_addr_next = addr; @@ if (ImGui::InvisibleButton("ascii", ImVec2(s.PosAsciiEnd - s.PosAsciiStart, s.LineHeight))) { - DataEditingAddr = DataPreviewAddr = addr + (size_t)((ImGui::GetIO().MousePos.x - pos.x) / s.GlyphWidth); - DataEditingTakeFocus = true; + size_t clicked_addr = addr + (size_t)((ImGui::GetIO().MousePos.x - pos.x) / s.GlyphWidth); + DataPreviewAddr = clicked_addr; + if (can_write) + { + DataEditingAddr = clicked_addr; + DataEditingTakeFocus = true; + } }Also applies to: 268-271, 298-302, 313-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp` around lines 151 - 154, The editor currently allows entering edit mode whenever ReadOnly is false even if WriteFn is null; update all edit-enable checks to require write capability by gating on WriteFn as well. Replace conditions that use "ReadOnly" alone (e.g., the block that sets DataEditingAddr to (size_t)-1, the byte-input handling path that silently discards writes, and the other edit-mode checks around DataEditingAddr) with checks that treat missing WriteFn as read-only — i.e., require (!ReadOnly && WriteFn != nullptr) to permit editing, and set DataEditingAddr = (size_t)-1 when (ReadOnly || WriteFn == nullptr || DataEditingAddr >= mem_size); apply the same change to the other edit branches referenced (the blocks around the byte input/discard and the two other edit-condition sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.h`:
- Around line 104-109: The cache can contain stale bytes after short/partial
bulk reads because BulkReadFn's return value is ignored and m_buf.resize(m_len)
leaves prior data intact; update the contract and handling so partial reads are
handled: change BulkReadFn to return ssize_t (actual bytes read) and, in fill()
(called by read()), capture that return value, then set m_buf to the actual
bytes read and zero-fill any remaining bytes (e.g. use m_buf.assign(m_len, 0) or
m_buf.resize(bytesRead) followed by m_buf.insert/append zeros to reach m_len) so
unwritten bytes are never reused; alternatively, if you cannot change the
signature, modify the callback implementation to always zero-fill the
destination buffer (use assign instead of resize) before calling the underlying
readAt so short reads cannot expose stale data.
---
Outside diff comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp`:
- Around line 151-154: The editor currently allows entering edit mode whenever
ReadOnly is false even if WriteFn is null; update all edit-enable checks to
require write capability by gating on WriteFn as well. Replace conditions that
use "ReadOnly" alone (e.g., the block that sets DataEditingAddr to (size_t)-1,
the byte-input handling path that silently discards writes, and the other
edit-mode checks around DataEditingAddr) with checks that treat missing WriteFn
as read-only — i.e., require (!ReadOnly && WriteFn != nullptr) to permit
editing, and set DataEditingAddr = (size_t)-1 when (ReadOnly || WriteFn ==
nullptr || DataEditingAddr >= mem_size); apply the same change to the other edit
branches referenced (the blocks around the byte input/discard and the two other
edit-condition sites).
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 879494ca-9111-4eef-b756-d93f666e25ef
📒 Files selected for processing (2)
third_party/imgui_memory_editor/imgui_memory_editor.cppthird_party/imgui_memory_editor/imgui_memory_editor.h
| std::function<void(void* dest, size_t off, size_t len)> BulkReadFn = nullptr; | ||
|
|
||
| ImU8 read(size_t off) { | ||
| if (!BulkReadFn || off >= m_totalSize) return 0; | ||
| if (off < m_base || off >= m_base + m_len) fill(off); | ||
| return m_buf[off - m_base]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect BulkReadFn/readAt bindings for short-read handling.
# Expected: Each BulkReadFn either fills exactly `len` bytes, zero-fills unwritten bytes, or checks/handles the read count.
rg -n -C4 --type=cpp --type=cc --type=h '\bBulkReadFn\s*=|\.BulkReadFn\b|readAt\s*\(' .Repository: grumpycoders/pcsx-redux
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Corrected search for BulkReadFn usages (removed invalid file type 'cc')
rg -n -C4 '\bBulkReadFn\s*=|\.BulkReadFn\b' . 2>/dev/null | head -100Repository: grumpycoders/pcsx-redux
Length of output: 4330
🏁 Script executed:
#!/bin/bash
# Also search for readAt function definitions and usages
rg -n '\breadAt\b' . 2>/dev/null | head -50Repository: grumpycoders/pcsx-redux
Length of output: 4542
🏁 Script executed:
#!/bin/bash
# Read the memory editor header file to see the full context of the fill() method and the issue
cat -n third_party/imgui_memory_editor/imgui_memory_editor.h | sed -n '95,135p'Repository: grumpycoders/pcsx-redux
Length of output: 1824
Fix stale cache bytes on short bulk reads.
The BulkReadFn callback ignores the return value of readAt(), which can perform partial reads. When a short-read occurs, m_buf.resize(m_len) leaves unwritten bytes from the previous cache page visible, causing silent data corruption in the memory display and export functions.
At src/gui/gui.h:359-361, the callback wraps sub->readAt(dest, len, off) but discards the ssize_t return value. If the underlying read fails to fill the entire buffer, stale bytes persist. Replace resize() with assign() to zero-fill, or make the callback return the actual byte count.
Minimal fix
m_len = std::min(kPageSize, m_totalSize - m_base);
- m_buf.resize(m_len);
+ m_buf.assign(m_len, 0);
BulkReadFn(m_buf.data(), m_base, m_len);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@third_party/imgui_memory_editor/imgui_memory_editor.h` around lines 104 -
109, The cache can contain stale bytes after short/partial bulk reads because
BulkReadFn's return value is ignored and m_buf.resize(m_len) leaves prior data
intact; update the contract and handling so partial reads are handled: change
BulkReadFn to return ssize_t (actual bytes read) and, in fill() (called by
read()), capture that return value, then set m_buf to the actual bytes read and
zero-fill any remaining bytes (e.g. use m_buf.assign(m_len, 0) or
m_buf.resize(bytesRead) followed by m_buf.insert/append zeros to reach m_len) so
unwritten bytes are never reused; alternatively, if you cannot change the
signature, modify the callback implementation to always zero-fill the
destination buffer (use assign instead of resize) before calling the underlying
readAt so short reads cannot expose stale data.
There was a problem hiding this comment.
Verify this claim more thoroughly. The cache should be very short lived: in the spirit of ImGui, it should be invalidated at the beginning of every frame. Otherwise this would break the widget completely since input data can be changed between every frame. The point of the cache is to avoid repeated read operations within the same frame.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check DrawContents for cache invalidation call
rg -n 'Cache\.' third_party/imgui_memory_editor/imgui_memory_editor.cpp
# Also check SubFile::readAt implementation for short-read behavior
rg -n -A 20 'SubFile::readAt' src/support/file.cc
Length of output: 773
@nicolasnoble, you're right — I've verified this more carefully and am withdrawing the concern.
Line 119–120 of imgui_memory_editor.cpp confirms that Cache.setSize(mem_size) and Cache.invalidate() are called at the very top of DrawContents, so m_base is reset to SIZE_MAX on every frame. No stale bytes from a previous frame can survive into the next one.
Additionally, within a single frame, stale data from resize() vs assign() isn't a concern either: SubFile::readAt pre-trims the requested size by any excess that would go beyond the subfile bounds, then delegates to the underlying m_file->readAt. Since fill() already computes m_len = std::min(kPageSize, m_totalSize - m_base), the callback is never asked for more bytes than the subfile can deliver, and in-memory data sources don't do short reads within valid bounds.
The original concern was unfounded. The per-frame invalidation + correctly-clamped m_len ensures the cache is always coherent for this usage pattern.
( • ᴗ •)っ ✎ noted for future reviews
🧠 Learnings used
Learnt from: nicolasnoble
Repo: grumpycoders/pcsx-redux PR: 2011
File: src/gui/gui.cc:1642-1667
Timestamp: 2026-04-19T15:33:48.877Z
Learning: In PCSX-Redux's imgui_memory_editor integration, ReadFn/WriteFn/BulkReadFn lambdas that capture a raw pointer from a local Slice (e.g., from getVRAM()) are safe because DrawWindow() is fully synchronous and invokes all callbacks only within its own call frame while the Slice is still alive. The lambdas are re-assigned every frame before DrawWindow() is called. No use-after-free occurs in this pattern.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
third_party/imgui_memory_editor/imgui_memory_editor.cpp (1)
113-120: Per-frame cache invalidation is correct for live-emulator data.
Cache.setSize+Cache.invalidateat the top of everyDrawContentscall defeats inter-frame caching, but that's the right tradeoff: emulator memory mutates between frames and stale page contents would mislead the user. The 4 KB bulk-read amortization still pays off within a single frame because the visible window typically spans only a handful of pages, so each frame does ~O(visible_pages)BulkReadFncalls instead of O(visible_bytes)ReadFncalls. No action needed; just flagging the intent for future readers — a one-line comment beyond the existing "Invalidate cache at the start of each frame" explaining why (live data) wouldn't hurt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp` around lines 113 - 120, Add a one-line explanatory comment above the Cache.setSize(mem_size); Cache.invalidate(); calls in MemoryEditor::DrawContents to state that per-frame invalidation is intentional because the editor is used with live emulator memory which can change between frames, so we must avoid stale cached pages even though it prevents inter-frame caching; reference the Cache.setSize and Cache.invalidate calls and the DrawContents method to help future readers understand the tradeoff with bulk-read amortization within a single frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@third_party/imgui_memory_editor/imgui_memory_editor.cpp`:
- Around line 113-120: Add a one-line explanatory comment above the
Cache.setSize(mem_size); Cache.invalidate(); calls in MemoryEditor::DrawContents
to state that per-frame invalidation is intentional because the editor is used
with live emulator memory which can change between frames, so we must avoid
stale cached pages even though it prevents inter-frame caching; reference the
Cache.setSize and Cache.invalidate calls and the DrawContents method to help
future readers understand the tradeoff with bulk-read amortization within a
single frame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 944d12bf-85b9-419d-8906-2f770d7a47ba
📒 Files selected for processing (1)
third_party/imgui_memory_editor/imgui_memory_editor.cpp
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2011 +/- ##
==========================================
+ Coverage 9.11% 10.27% +1.16%
==========================================
Files 470 519 +49
Lines 147911 149850 +1939
==========================================
+ Hits 13482 15397 +1915
- Misses 134429 134453 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Replace the raw void* data pointer API in the imgui memory editor with callback-based data access through IO. This decouples the viewer from direct memory buffer access, allowing it to work with any File- derived source through the existing IO template system.
The memory editor now uses std::function callbacks (ReadFn/WriteFn) for byte-level access, with a 4KB read-ahead cache (BulkReadFn) to batch individual byte reads into bulk readAt calls for performance.
All non-VRAM editors (main memory, parallel port, scratch pad, hardware registers, BIOS) now go through getMemoryAsFile() with SubFile for address translation, instead of accessing raw m_wram/m_exp1/m_hard/ m_bios pointers directly.
SubFile gains a FileOps::ReadWrite constructor and write/writeAt support, matching the existing tag-based pattern used by PosixFile and BufferFile for mutating file capabilities.