Simplify GDB memory access by borrowing sandbox memory manager - #1735
Closed
ludfjig wants to merge 1 commit into
Closed
Simplify GDB memory access by borrowing sandbox memory manager#1735ludfjig wants to merge 1 commit into
ludfjig wants to merge 1 commit into
Conversation
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
ludfjig
marked this pull request as ready for review
August 13, 2026 19:42
ludfjig
requested review from
andreiltd,
danbugs,
dblnz,
devigned,
jprendes,
jsturtevant,
simongdavies,
squillace and
syntactically
as code owners
August 13, 2026 19:42
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the GDB memory-access path in hyperlight_host to borrow the sandbox’s in-use SandboxMemoryManager rather than cloning and mutex-wrapping it, aiming to avoid stale memory views after snapshot restore and to simplify the init/run/dispatch plumbing.
Changes:
- Remove the cloned
SandboxMemoryManager+Arc<Mutex<…>>plumbing from sandbox initialization and VM dispatch paths. - Replace
DebugMemoryAccess(owned, mutex-backed) withDebugMemoryView(borrowed view) for GDB memory operations. - Drop
CloneonSandboxMemoryManagerand update call sites/tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Removes GDB-specific cloned/mutex memory-manager plumbing during evolve. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Removes stored debug memory manager and simplifies dispatch/init signatures. |
| src/hyperlight_host/src/mem/mgr.rs | Removes Clone derive from SandboxMemoryManager. |
| src/hyperlight_host/src/hypervisor/mod.rs | Updates hypervisor tests to match the simplified initialise signature. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs | Updates initialise/dispatch/debug handling to use borrowed DebugMemoryView. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs | Updates run to pass the live mem_mgr into debug handling without extra handles. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs | Updates initialise/dispatch calls to match the simplified run signature. |
| src/hyperlight_host/src/hypervisor/gdb/mod.rs | Introduces DebugMemoryView and refactors GDB mem-access tests/utilities. |
Suppressed comments (1)
src/hyperlight_host/src/hypervisor/gdb/mod.rs:551
- Same issue as the single-byte write test: this writes to a GPA that resolves to
BaseGpaRegion::Mmap, butDebugMemoryView::writeonly permits scratch (and snapshot underunshared_snapshot_mem). The test should either expectWriteToReadOnlyor write into scratch and validatescratch_mem.
let write_data = [0xAAu8; 16];
memory
.access()
.write(&write_data, (BASE_VIRT + offset) as u64)
.unwrap();
let slice = unsafe { memory.mmap_slice() };
assert_eq!(slice[offset..offset + 16], write_data);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
521
to
528
| let write_data = [0xCCu8; 1]; | ||
| mem_access | ||
| memory | ||
| .access() | ||
| .write(&write_data, (BASE_VIRT + offset) as u64) | ||
| .unwrap(); | ||
|
|
||
| let slice = unsafe { get_mmap_slice(&mut mem_access) }; | ||
| let slice = unsafe { memory.mmap_slice() }; | ||
| assert_eq!(slice[offset], write_data[0]); |
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
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.
Noticed this ugly code when reviewing another PR and thought it could be cleaned up. MemoryManager already contains everything needed for gdb so no need to clone it as we can borrow it instead. Also fixes potential stale memory access since snapshot restore can replace snapshot and scratch memory, and the cloned manager could potentially be out of sync. Also removed some now unnecessary synchronization and plumbing