Add front-end for debug variable values#18
Conversation
Implement debug variable tracking and display in the debugger: - Add DebugVarTracker to track variables throughout execution - Add DebugVarSnapshot for variable state at specific clock cycles - Add stub types for DebugVarInfo and DebugVarLocation (pending new miden-core) - Add resolve_variable_value (currently returns None, pending new miden-core) - Add :vars/:variables/:locals commands to display current variables The vars command shows variables in format "name=value" or "name=location" if the value cannot be resolved. Note: Full variable value resolution requires new version of miden-core.
Load link libraries and stdlib before calling with_dependencies() to ensure the dependency resolver has all required libraries registered. This fixes the "Dependency not found in resolver" panic when running programs that depend on the standard library.
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
Looks good!
There's two changes I'd like to make here:
- I think we would benefit from using string interning for debug info, considering how repetitive some of the information is (variable names). I provided more detail in another comment, but the gist is that we can use
midenc_hir_symbolfor that, and replace uses ofStringwithSymbolanywhere that we expect a string to be used a lot. We could also usecompact_str, butSymbolhas some additional properties that make it nicer for certain things (namely cheap copies, small size, and using it as a key in key/value data structures). - We should not bake in
miden-stdlib(or any Miden package/library), and should instead lean on an installedmidenuptoolchain for similar convenience. Since most (virtually all) users of the debugger will use it throughmiden debug, the benefits are the same - and once we migrate to v0.21, we'll additionally be able to usemiden-project.tomlto resolve dependencies more precisely (and theLibrarystruct is going away in that release as well, so baking inmiden-stdlibisn't likely to continue being an option regardless.
| /// This is a stub type until miden-core provides DebugVarInfo. | ||
| #[derive(Debug, Clone)] | ||
| pub struct DebugVarInfo { | ||
| name: String, |
There was a problem hiding this comment.
Let's add midenc_hir_symbol as a dependency, and replace this with midenc_hir_symbol::Symbol. That way all variable names are interned, and the debug info gets a lot more compact in memory.
| /// Variable is a local at the given frame offset | ||
| Local(u16), | ||
| /// Variable location is computed via an expression | ||
| Expression(Vec<u8>), |
There was a problem hiding this comment.
Is there any reason to represent this as a byte vector, rather than decoding it into the bytecode/opcodes needed to evaluate the expression?
I think we'll want to define those types in miden-mast-package, since that's where the types serialized to the package format are canonically defined, but we can have our own versions of them here in the debugger too that we map the serialized representation to when deserializing (if needed). I think the bytecode ops we'd need to support wouldn't require us to do that, but let me know if there are any issues that come to mind.
If we do that, then we can leave this as-is for the time being, and then surface the actual ops when we migrate to v0.21
| /// This structure maintains a mapping from variable names to their most recent | ||
| /// location information at each clock cycle. It's designed to work with the | ||
| /// debugger to provide source-level variable inspection. | ||
| pub struct DebugVarTracker { |
There was a problem hiding this comment.
Note: I think we'll be able to remove this once we migrate to v0.21, since we won't need to compute an execution trace (and associated state) before we start debugging.
| }; | ||
|
|
||
| /// Get the standard library as an Arc<Library> | ||
| fn get_stdlib() -> Arc<Library> { |
There was a problem hiding this comment.
Issue: I would prefer not to do this - we do it in the compiler out of necessity/historical reasons, but we're moving away from that with the advent of midenup and the toolchain it manages.
Hardcoding it here like this means that the debugger only works with that exact version of the stdlib - and we need it to be much more flexible than that.
What we can do, is check for the MIDENUP_HOME environment variable, which is set by miden when executing any component executables (e.g. miden debug). If present, we can use that to locate and load any packages from the lib directory of the active toolchain. That will make it convenient for your average user (i.e. they won't need to manually pass flags to load the standard library by hand when using miden debug).
One issue I just found, and I'll open a PR for today, is that midenup is only setting MIDENUP_HOME, but not providing the active toolchain sysroot via the environment as well (e.g. MIDENUP_SYSROOT). However, you can obtain this as follows:
# `miden` always sets PATH for the executables it calls, placing the $MIDENUP_SYSROOT/opt directory at the front
let path_raw = std::env::var("PATH").unwrap();
# This is guaranteed to succeed when `miden debug` is used, but we probably want to handle the case where someone has set MIDENUP_HOME in their environment and is running `miden-debug` directly
let toolchain_opt_dir = Path::new(path_raw.split_once(':').unwrap().0);
# The $MIDENUP_SYSROOT/lib directory contains any installed Miden packages that ship as part of the toolchain
let toolchain_lib_dir = toolchain_opt_dir.parent().unwrap().join("lib");
|
Will continue with #17, since it will be ported to |
2 participants
It depends on the
miden-vmandcompiler, that is why it is just a placeholder for now.