Simplify GDB memory access by borrowing sandbox memory manager - #1736
Conversation
404f212 to
393fa42
Compare
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
393fa42 to
04f3a90
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the GDB memory-access path to borrow the sandbox’s live SandboxMemoryManager rather than cloning it, avoiding stale debug views after snapshot restore and simplifying synchronization/plumbing in the hypervisor run/dispatch flow.
Changes:
- Remove cloned
SandboxMemoryManagerhandles used for GDB, and pass the live manager through the VM run/dispatch paths. - Introduce
DebugMemoryViewas a borrowed memory accessor for GDB operations and update debug request processing accordingly. - Update GDB test wiring and a Just recipe to target the GDB-related unit tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Removes GDB-specific cloned memory manager plumbing during sandbox evolve. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Drops stored cloned debug memory handle and simplifies VM initialisation/dispatch calls. |
| src/hyperlight_host/src/mem/mgr.rs | Removes Clone derive from SandboxMemoryManager now that cloning is no longer needed. |
| src/hyperlight_host/src/hypervisor/mod.rs | Updates hypervisor tests to match the new initialise signature without a debug mem handle. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs | Switches debug handling to use DebugMemoryView and removes lock-based access. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs | Updates run() debug handling to call handle_debug with the live memory manager reference. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs | Removes the extra debug-memory argument from initialise/dispatch calls. |
| src/hyperlight_host/src/hypervisor/gdb/mod.rs | Replaces DebugMemoryAccess (Arc/Mutex) with borrowed DebugMemoryView and refreshes tests. |
| Justfile | Adjusts the GDB test recipe to run hyperlight-host lib tests filtered to the GDB module. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
This looks reasonable to me. Getting rid of the dbg_mem_access_fn clone has been on my todo list for a while because I had a sneaking suspicion that something was off with it even though I never had the time to actually check. It's a shame that the guest_mmap_regions vector is still unsafe to access.
The only real question I have with this approach is whether we want to support the idea of running the gdb thread in parallel with other sandbox threads at any point. This would I think make that difficult? I think that that is likely only useful if we are running multiple sandbox threads in parallel (or the sandbox and the host in parallel), in which case we would have to have some other solution to make the SandboxMemoryManager more shareable, so maybe this will not stay a single &mut reference forever. But, at least, getting rid of the separate dbg_mem_access_fn that was not staying in sync is a major improvement!
Agreed, for the future this might make it a bit trickier if we want to run it in parallel, but I think that's fine. |
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