Skip to content

Simplify GDB memory access by borrowing sandbox memory manager - #1736

Merged
ludfjig merged 1 commit into
mainfrom
ll/simplify_gdb
Aug 14, 2026
Merged

Simplify GDB memory access by borrowing sandbox memory manager#1736
ludfjig merged 1 commit into
mainfrom
ll/simplify_gdb

Conversation

@ludfjig

@ludfjig ludfjig commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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

@ludfjig ludfjig changed the title borrow memory manager instead of cloning, make manager non-clone Simplify GDB memory access by borrowing sandbox memory manager Aug 13, 2026
@ludfjig ludfjig added the kind/refactor For PRs that restructure or remove code without adding new functionality. label Aug 13, 2026
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
@ludfjig
ludfjig marked this pull request as ready for review August 13, 2026 22:45
Copilot AI lite review requested due to automatic review settings August 13, 2026 22:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SandboxMemoryManager handles used for GDB, and pass the live manager through the VM run/dispatch paths.
  • Introduce DebugMemoryView as 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.

Comment thread Justfile
Comment thread src/hyperlight_host/src/hypervisor/gdb/mod.rs

@syntactically syntactically left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@ludfjig

ludfjig commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

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.

@ludfjig
ludfjig merged commit e65c966 into main Aug 14, 2026
53 checks passed
@ludfjig
ludfjig deleted the ll/simplify_gdb branch August 14, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor For PRs that restructure or remove code without adding new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants