Conversation
bitwalker
left a comment
There was a problem hiding this comment.
Nice work! This is not a thorough review since this is still a draft, but wanted to at least provide some initial feedback
crates/dap/Cargo.toml
Outdated
| @@ -0,0 +1,26 @@ | |||
| [package] | |||
| name = "dap" | |||
There was a problem hiding this comment.
Let's name this miden-debug-dap
crates/dap/Cargo.toml
Outdated
| [package] | ||
| name = "dap" | ||
| version = "0.1.0" | ||
| edition = "2021" |
crates/dap/Cargo.toml
Outdated
| categories = ["development-tools::debugging"] | ||
|
|
||
| [lib] | ||
| doctest = false |
There was a problem hiding this comment.
Any specific reason for this? I don't inherently have an issue with it, but like to know why we we use any non-default configurations.
There was a problem hiding this comment.
Not needed. I used it locally. Will drop it.
crates/dap/Cargo.toml
Outdated
| fake = { version = "2.*", features = ["derive"], optional = true } | ||
| rand = { version = "0.*", optional = true } | ||
|
|
||
| [features] |
There was a problem hiding this comment.
We generally always put [features] above [dependencies], since it makes it easier to reason about the latter
There was a problem hiding this comment.
Looks like we should convert the repo into a workspace now, we really have a couple of different crates:
miden-debug-dap- the DAP server componentmiden-debug-engine, (currently the[lib]target ofmiden-debug) - the execution engine component and core data typesmiden-debug, (the current[bin]target) - the CLI driver and terminal user interface component
The value in breaking out miden-debug-engine is that all of the TUI/CLI dependencies can be avoided in downstream dependents (e.g. the compiler, miden-testing).
We may also end up with more crates in the future, but that's unclear. Using a workspace we can share dependencies and the release process stays simple, so I prefer that route when we have multiple crates in a single repo.
src/ui/state.rs
Outdated
| Transaction, | ||
| /// Debugging remotely via a DAP server connection. | ||
| #[cfg(feature = "dap")] | ||
| RemoteDap, |
There was a problem hiding this comment.
Let's just call this Remote. We can also remove the feature-gate, since it will simplify any code that matches on DebugMode (we can simply return an error if it is used without DAP support enabled)
src/ui/state.rs
Outdated
| return Err(Report::msg("reload is not supported in transaction debug mode")); | ||
| } | ||
| #[cfg(feature = "dap")] | ||
| if self.debug_mode == DebugMode::RemoteDap { |
There was a problem hiding this comment.
I think we probably can consider removing the current reload command, and instead support a remote DAP server telling us to reload (providing the new artifact and where to resume to).
Not something we need to do now, but trying to figure out how we can support edit-and-continue style workflows in IDEs which are debugging code via miden-debug
There was a problem hiding this comment.
I agree with that direction. For now I kept reload as a local-only command and made the remote case fail explicitly, because we don’t yet have a protocol for the server to push a replacement artifact plus resume location/state back to the client.
I think the right long-term shape is what you describe: in remote mode, reload should be driven by the DAP server/debuggee side rather than by the TUI client trying to reconstruct execution locally. I’d prefer to keep that as a follow-up once we define the remote reload/edit-and-continue flow more concretely.
I will add a TODO here.
src/ui/state.rs
Outdated
|
|
||
| #[cfg(feature = "dap")] | ||
| if self.debug_mode == DebugMode::RemoteDap { | ||
| return Err("memory reads are not supported in DAP remote debug mode".into()); |
There was a problem hiding this comment.
That's a brutal limitation IMO - why is this something we cannot support?
My mental model for all this is as follows:
- Someone wants to debug a transaction in
miden-client, so they runmiden-clientwith something likemiden-client ... --start-debug-server "127.0.0.1:4000". When the client goes to execute the transaction, themiden-debugexecutor is instantiated in DAP server mode, and starts listening for DAP clients on the configured interface and port - Either from my IDE (using an extension built on top of
miden-debug's internals), or using the CLI via something likemiden-debug --remote 127.0.0.1:4000, we usemiden-debugin DAP client mode to connect to the DAP server. - The
miden-debugDAP client is simply relaying commands to themiden-debugDAP server, and receiving whatever data is necessary to render the UI (or answer queries) over the wire. - EIther the DAP client exits (and the server can then execute to completion normally), or the DAP server exits (because the transaction has finished execution, or it was killed)
So both the client and the server can perform memory reads, it is just that the server handles reading the necessary bytes from its state, which the client expects to be sent over the wire, and the decoding of the value (and rendering) is handled client-side.
That said, this is just what I had sketched out in my mind, the actual implementation likely differs. Could you elaborate on your overall vision for how these pieces fit together, and how we'll support some of these important debugging flows?
There was a problem hiding this comment.
I agree, and this was just an implementation gap, not a fundamental limitation. I’ve removed that restriction in this branch.
In remote mode, memory reads are now proxied to the DAP server/debuggee side: the remote client sends a read request over DAP, the server evaluates it against its current processor state, and returns the rendered result. So the client no longer tries to read remote memory from local state.
More broadly, my intended model is very close to what you described:
- miden-client starts transaction execution with a miden-debug DAP-backed executor on the debuggee side
- miden-debug --remote ... acts as a DAP client/TUI
- the server/debuggee owns the real execution state
- the client relays stepping/continue/read-style requests over DAP and refreshes its UI from server responses
The main simplification in the current implementation is that after a stop we still refresh state by querying the server (stack_trace, variables, evaluate("__miden_state")) rather than having the server push a richer state update proactively. I think push/event-driven updates would be a good follow-up, but the overall direction is the same: the remote client is a UI over server-owned execution state, not an independent executor.
| // DAP CLIENT MODE | ||
| // ================================================================================================ | ||
|
|
||
| #[cfg(feature = "dap")] |
There was a problem hiding this comment.
Perhaps we should split up the State internals into two structs, one for normal/server mode, and one for client mode? The requirements are quite different for each, and the client in particular isn't actually responsible for maintaining much state (as the DAP server is the one holding it). Instead, the DAP client relays all commands/requests to the DAP server, and then handles the details of deserializing the responses and updating the UI.
Both "normal" execution and DAP server mode execution are essentially the same from a state perspective, so those can be combined I think.
| } | ||
|
|
||
| /// Refresh the executor state from the DAP server after a step command. | ||
| pub fn refresh_from_dap(&mut self) -> Result<(), Report> { |
There was a problem hiding this comment.
Can we rely on the DAP server to tell us about changes rather than querying for them? That would likely be a lot more efficient, but perhaps the protocol doesn't work that way.
No description provided.