feat(corelib): add core::debug::print_mem_addr#3207
Conversation
8abca5c to
ed36e79
Compare
Adds `miden::core::debug::print_mem_addr`, which prints the contents of a single memory cell given its address. Combined with `locaddr`, this gives a clean way to print a procedure local (e.g. `locaddr.0 exec.debug::print_mem_addr`), covering the original `print_local` request from #3203 without needing the debug procedure to resolve a caller's local index. The procedure emits a dedicated `print_mem_addr` event handled by the existing `DebugPrinter`, which validates the address (rejecting out-of-bounds the same way `print_mem` does) and prints the cell value, or an "uninitialized" message for an untouched address. Follows #3185 and #3201. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ed36e79 to
524c468
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline.
Address bobbin's review on #3207: - print_mem_addr reuses PRINT_MEM_EVENT via `dup add.1 swap` (range [addr, addr+1)) instead of a dedicated PRINT_MEM_ADDR_EVENT. - print_mem_all reuses PRINT_MEM_EVENT over the full range [0, u32::MAX), dropping PRINT_MEM_ALL_EVENT (mirrors print_adv_stack_all). - Rename PRINT_ADV_MAP_ALL_EVENT -> PRINT_ADV_MAP_EVENT. To make a full-range print viable, the PRINT_MEM_EVENT handler now enumerates only initialized cells (filtering get_mem_state by the range) instead of iterating the raw address space, which removes the need for the MAX_PRINT_MEM_RANGE cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This PR contains unsigned commits. All commits must be cryptographically signed (GPG or SSH). Unsigned commits:
For instructions on setting up commit signing and re-signing existing commits, see: |
Address bobbin's review on #3207: - print_mem_addr reuses PRINT_MEM_EVENT via `dup add.1 swap` (range [addr, addr+1)) instead of a dedicated PRINT_MEM_ADDR_EVENT. - print_mem_all reuses PRINT_MEM_EVENT over the full range [0, u32::MAX), dropping PRINT_MEM_ALL_EVENT (mirrors print_adv_stack_all). - Rename PRINT_ADV_MAP_ALL_EVENT -> PRINT_ADV_MAP_EVENT. To make a full-range print viable, the PRINT_MEM_EVENT handler now enumerates only initialized cells (filtering get_mem_state by the range) instead of iterating the raw address space, which removes the need for the MAX_PRINT_MEM_RANGE cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
53a51b0 to
f20727e
Compare
huitseeker
left a comment
There was a problem hiding this comment.
LGTM, apart for the edge case — how would we want to address? 🤔
| #! | ||
| #! Cycles: 6 | ||
| pub proc print_mem_addr | ||
| dup add.1 swap |
There was a problem hiding this comment.
Nit: This misses the last valid memory address. If addr == u32::MAX, add.1 makes the exclusive end larger than u32::MAX, and get_mem_addr_range rejects that end before the handler can print the cell. The old dedicated handler avoided this edge case.
There was a problem hiding this comment.
Great catch! At u32::MAX the add.1 pushes the exclusive end to 2^32, and get_mem_addr_range processor/src/lib.rs:235-237 errors on anything past u32::MAX.
The fix is to change get_mem_addr_range to allow an end up to 2^32 and return Range<u64> then print_mem_addr will work as-is.
And having print_mem_all push 2^32 instead of u32::MAX so it picks up the last cell fixes print_mem_all
| #! Cycles: 7 | ||
| pub proc print_mem_all | ||
| emit.PRINT_MEM_ALL_EVENT | ||
| push.4294967295 push.0 |
There was a problem hiding this comment.
Nit: This range is [0, u32::MAX), so it excludes an initialized cell at address u32::MAX. The old full-memory event printed the whole initialized memory map, including that last address. We may need to keep a distinct full-memory path or special-case this boundary.
A half-open [start, end) range over a u32 address space cannot include the cell at u32::MAX with a u32 exclusive end. As a result print_mem_addr at u32::MAX aborted (add.1 produced 2^32, which get_mem_addr_range rejected) and print_mem_all silently excluded that cell. Widen the exclusive end: get_mem_addr_range now permits an end up to 2^32 and returns Range<u64>. print_mem_addr works unchanged; print_mem_all pushes 2^32 so [0, 2^32) covers u32::MAX. sorted_array keeps its strict u32 bound and narrows back. Adds regression tests for the u32::MAX boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
thank you @huitseeker for catching this! Went ahead and implemented this. Fixes not being able to print the memory cell at
|
| fn write_mem_range<W: fmt::Write>( | ||
| w: &mut W, | ||
| process: &ProcessorState, | ||
| range: core::ops::Range<u32>, | ||
| range: core::ops::Range<u64>, | ||
| ) -> fmt::Result { |
There was a problem hiding this comment.
I'm not too sure about this change. Technically, the range of valid addresses here should be limited to u32 values. I think the issue is that we are using Range which is exclusive of the last value. Maybe a better solution would be to use RangeInclusive? This way, we properly constrain the valid addresspace.
There was a problem hiding this comment.
I would change the type of range to a new type parameter R, bounded as R: core::ops::RangeBounds<u32>. That allows range syntax with both inclusive/exclusive ranges to be used
There was a problem hiding this comment.
Implemented @bitwalker's suggestion of a new type parameter R: RangeBounds<u32> on write_mem_range, so the address space stays constrained to u32 while still accepting both inclusive and exclusive ranges.
Per review, revert get_mem_addr_range to Range<u32> (both bounds <= u32::MAX) and remove the sorted_array narrowing hack. The debug printer gets its own read_mem_print_range helper returning RangeInclusive<u32>, so the u32::MAX cell stays reachable while no address value exceeds u32::MAX. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Use empty brackets [] and list only the relevant items in the Inputs/Outputs stack annotations of core::debug procedures; regenerate docs/debug.md.
| /// cell at `u32::MAX` can be printed (the range `[addr, addr+1)` for `addr == u32::MAX`, or the | ||
| /// full range `[0, 2^32)`). The inclusive representation keeps both bounds within `u32`, so no | ||
| /// address value ever exceeds `u32::MAX`. | ||
| fn read_mem_print_range( |
There was a problem hiding this comment.
The next branch had a 1024-cell limit on print ranges (enforced in the handler). That limit and the associated test (print_mem_rejects_oversized_range) are gone in this PR. Was removing the limit intentional? The old test would catch a caller accidentally passing a huge range.
There was a problem hiding this comment.
Wasn't intentional. It got dropped along with the 1024-cell limit when I consolidated the events. Brought both back: the limit now exempts that full range, and print_mem_rejects_oversized_range is passing again.
…bug printing Re-add the 1024-address cap on explicit print_mem ranges (exempting the full [0, 2^32) range used by print_mem_all) along with the print_mem_rejects_oversized_range test, and make write_mem_range generic over RangeBounds<u32> so the empty range no longer needs a reversed RangeInclusive.
| let Some((start, end)) = resolve_addr_bounds(&range) else { | ||
| return writeln!(w, "Memory state before step {clk} for context {ctx}: range is empty."); | ||
| }; | ||
| let items: Vec<_> = process |
There was a problem hiding this comment.
Could we keep explicit print_mem enumerating every address in the requested range? With:
push.42 push.100 mem_store
push.110 push.100
exec.debug::print_mem
this prints only 0x64..0x67, even though the header says [100, 109]. On next, write_mem_range iterated the requested range and write_interval showed missing cells as EMPTY, so addresses 104..109 did not disappear.
There was a problem hiding this comment.
Fixed in 78f56b2. print_mem enumerates every address in the requested range again and shows uninitialized cells as EMPTY, same as on next. Added a test for your exact repro.
| let range = read_mem_print_range(process, 1, 2)?; | ||
| // Guard against an accidentally huge explicit range; the full `[0, 2^32)` range used by | ||
| // `print_mem_all` is exempt. | ||
| if let Some((first, last)) = resolve_addr_bounds(&range) |
There was a problem hiding this comment.
Could this exemption be limited to print_mem_all? A program can call the shared event path directly:
push.4294967296 push.0
exec.debug::print_mem
That succeeds and prints the full [0, u32::MAX] range, so ordinary print_mem can skip the documented 1024-address cap. A separate full-memory event or a marker for print_mem_all would keep the cap true for user-supplied ranges.
There was a problem hiding this comment.
Good point. Brought back a dedicated print_mem_all event in 78f56b2, so the 1024 address cap now applies to every print_mem range with no exemption. Added a test that the full range is rejected. The shared event turned out to lose real semantics here: print_mem enumerates a capped range while print_mem_all lists only initialized cells.
…umeration Per review on #3207: - print_mem enumerates every address in the requested range again, showing uninitialized cells as EMPTY (as on next), instead of silently dropping them. - print_mem_all is back on its own event, so the 1024-address cap on print_mem applies unconditionally and can no longer be bypassed by passing the full [0, 2^32) range directly.
…umeration Per review on #3207: - print_mem enumerates every address in the requested range again, showing uninitialized cells as EMPTY (as on next), instead of silently dropping them. - print_mem_all is back on its own event, so the 1024-address cap on print_mem applies unconditionally and can no longer be bypassed by passing the full [0, 2^32) range directly.
78f56b2 to
dc5132e
Compare
huitseeker
left a comment
There was a problem hiding this comment.
This LGTM, but I think there's a possible simpler concrete bounds shape instead of resolving RangeBounds twice.
| "print_mem range length {len} exceeds maximum of {MAX_PRINT_MEM_RANGE}" | ||
| ) | ||
| .into()); | ||
| let range = read_mem_print_range(process, 1, 2)?; |
There was a problem hiding this comment.
Nit: Could this helper return concrete bounds after it parses the stack values? read_mem_print_range already turns the half-open stack range into either an empty range or inclusive u32 bounds, so returning something like Option<(u32, u32)> would let the cap check and write_mem_range share the same value.
That keeps the u32::MAX case, but avoids resolving RangeBounds twice and encoding empty as included/excluded bounds on the same address.
There was a problem hiding this comment.
Good idea, will look into this
There was a problem hiding this comment.
thank you for this suggestion, I updated read_mem_print_range to now return Option<(u32, u32)> for inclusive bounds and None for an empty range.
|
This PR contains unsigned commits. All commits must be cryptographically signed (GPG or SSH). Unsigned commits:
For instructions on setting up commit signing and re-signing existing commits, see: |
The helper now returns Option<(u32, u32)> inclusive address bounds (None for an empty range), so the 1024-address cap check and write_mem_range share the same parsed value instead of resolving RangeBounds twice and encoding an empty range as included/excluded bounds on the same address. resolve_addr_bounds is no longer needed and is removed.
aa46350 to
ab2bddb
Compare
Summary
Closes #3203.
Adds
miden::core::debug::print_mem_addr, which prints a single memory cell given its address. Combine withlocaddrto print a procedure local:This covers the
print_localrequest from #3203: a debug procedure can't resolve a caller's local index, butlocaddralready turns a local into an absolute address — what was missing was a way to print one cell (print_memonly takes a[start, end)range).Changes
asm/debug.masm: newprint_mem_addrproc emitting a dedicated event.src/handlers/debug.rs: handler registered in the default/noop/full sets; validates the address likeprint_memand prints the cell (or "uninitialized").docs/debug.md, and a CHANGELOG entry.Follows #3185 and #3201. Supersedes #3205 (branch renamed to
ajl-print-mem-addr).