Fix getting memory from ananas #112
Conversation
WalkthroughReplaces the debugger's internal memory-read path to call a new Changes
Sequence Diagram(s)sequenceDiagram
participant Debugger as api-debugger
participant Interpreter as Interpreter
participant Memory as Memory (pages)
rect rgb(245,250,255)
Debugger->>Interpreter: getMemory(faultRes, address, length)
note right of Interpreter: Delegates to Memory.getMemory
Interpreter->>Memory: scan pages for faults (no allocations)
alt fault detected
Memory-->>Interpreter: set faultRes.isFault = true\nreturn null
Interpreter-->>Debugger: return null (fault)
else no fault
Memory->>Memory: allocate destination buffer
Memory-->>Interpreter: Uint8Array (data)
Interpreter-->>Debugger: return Uint8Array
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assembly/api-debugger.ts(1 hunks)assembly/memory.test.ts(1 hunks)assembly/memory.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 92
File: assembly/api-debugger.ts:170-182
Timestamp: 2025-10-16T15:17:10.864Z
Learning: In assembly/memory.ts, the `bytesRead` method signature is: `bytesRead(faultRes: MaybePageFault, address: u32, destination: Uint8Array, destinationOffset: u32): void`. The last parameter is the offset within the destination buffer to start writing at, not the number of bytes to read. The method reads until the destination buffer is filled from destinationOffset to destination.length.
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
📚 Learning: 2025-10-16T15:17:10.864Z
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 92
File: assembly/api-debugger.ts:170-182
Timestamp: 2025-10-16T15:17:10.864Z
Learning: In assembly/memory.ts, the `bytesRead` method signature is: `bytesRead(faultRes: MaybePageFault, address: u32, destination: Uint8Array, destinationOffset: u32): void`. The last parameter is the offset within the destination buffer to start writing at, not the number of bytes to read. The method reads until the destination buffer is filled from destinationOffset to destination.length.
Applied to files:
assembly/api-debugger.tsassembly/memory.test.tsassembly/memory.ts
📚 Learning: 2025-04-22T15:58:24.284Z
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Applied to files:
assembly/memory.test.tsassembly/memory.ts
🧬 Code graph analysis (3)
assembly/api-debugger.ts (2)
assembly/memory.ts (2)
getMemory(199-223)MaybePageFault(15-19)assembly/interpreter.ts (1)
Interpreter(21-198)
assembly/memory.test.ts (1)
assembly/memory.ts (2)
MemoryBuilder(38-88)MaybePageFault(15-19)
assembly/memory.ts (1)
assembly/memory-page.ts (1)
PAGE_SIZE(5-5)
🪛 GitHub Actions: Node.js CI
assembly/api-debugger.ts
[error] 179-179: File content differs from formatting output.
assembly/memory.test.ts
[error] 113-114: File content differs from formatting output.
assembly/memory.ts
[error] 198-199: File content differs from formatting output.
[error] 201-205: File content differs from formatting output.
[warning] 206-206: iLength is never reassigned.
[error] 211-216: File content differs from formatting output.
🔇 Additional comments (6)
assembly/memory.test.ts (2)
113-125: LGTM!The test correctly verifies that
getMemorydetects page faults when attempting to read beyond allocated memory.
126-138: LGTM!The test correctly verifies that
getMemoryhandles allocation failures for oversized memory requests.assembly/api-debugger.ts (2)
174-201: Good documentation of the deprecation rationale.The detailed explanation of why this API is inefficient and what the better approach should be is helpful for future refactoring.
202-213: LGTM!The change correctly uses the new
getMemoryAPI which handles fault detection before allocation.assembly/memory.ts (2)
252-252: LGTM!Clear documentation for the
bytesWritemethod.
361-361: LGTM!Clear documentation for the
setBytesmethod.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
assembly/memory.test.ts (1)
113-125: Test validates overflow protection correctly.This test properly checks that
getMemoryfails when the address range would overflow u32 (2343629385 + 2145386496 = 4489015881 > 4294967295). The assertions correctly expect a page fault and null result.Consider adding a brief comment explaining the overflow scenario being tested for future maintainability:
test("should page fault when going beyond memory", (assert) => { + // Test u32 overflow: address + length > u32::MAX const address = 2343629385; const length = 2145386496;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
assembly/api-debugger.ts(1 hunks)assembly/memory.test.ts(1 hunks)assembly/memory.ts(3 hunks)biome.jsonc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- assembly/api-debugger.ts
- assembly/memory.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 92
File: assembly/api-debugger.ts:170-182
Timestamp: 2025-10-16T15:17:10.864Z
Learning: In assembly/memory.ts, the `bytesRead` method signature is: `bytesRead(faultRes: MaybePageFault, address: u32, destination: Uint8Array, destinationOffset: u32): void`. The last parameter is the offset within the destination buffer to start writing at, not the number of bytes to read. The method reads until the destination buffer is filled from destinationOffset to destination.length.
📚 Learning: 2025-10-16T15:17:10.864Z
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 92
File: assembly/api-debugger.ts:170-182
Timestamp: 2025-10-16T15:17:10.864Z
Learning: In assembly/memory.ts, the `bytesRead` method signature is: `bytesRead(faultRes: MaybePageFault, address: u32, destination: Uint8Array, destinationOffset: u32): void`. The last parameter is the offset within the destination buffer to start writing at, not the number of bytes to read. The method reads until the destination buffer is filled from destinationOffset to destination.length.
Applied to files:
assembly/memory.test.ts
📚 Learning: 2025-04-22T15:58:24.284Z
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Applied to files:
assembly/memory.test.ts
🧬 Code graph analysis (1)
assembly/memory.test.ts (1)
assembly/memory.ts (2)
MemoryBuilder(38-88)MaybePageFault(15-19)
🔇 Additional comments (2)
biome.jsonc (1)
46-46: LGTM! Good practice to exclude build artifacts.Excluding the build directory from linting and formatting is appropriate and prevents unnecessary processing of generated files.
assembly/memory.test.ts (1)
126-138: Test correctly validates large allocation handling.This test properly verifies that
getMemoryfails safely when attempting to read a very large length (~2GB) from an unallocated address, preventing potentially problematic buffer allocation attempts.Note: The formatting issue mentioned in the previous review (missing semicolon on line 115) has been resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assembly/memory.test.ts(1 hunks)assembly/memory.ts(3 hunks)biome.jsonc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- assembly/memory.test.ts
- biome.jsonc
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 92
File: assembly/api-debugger.ts:170-182
Timestamp: 2025-10-16T15:17:10.864Z
Learning: In assembly/memory.ts, the `bytesRead` method signature is: `bytesRead(faultRes: MaybePageFault, address: u32, destination: Uint8Array, destinationOffset: u32): void`. The last parameter is the offset within the destination buffer to start writing at, not the number of bytes to read. The method reads until the destination buffer is filled from destinationOffset to destination.length.
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
📚 Learning: 2025-10-16T15:17:10.864Z
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 92
File: assembly/api-debugger.ts:170-182
Timestamp: 2025-10-16T15:17:10.864Z
Learning: In assembly/memory.ts, the `bytesRead` method signature is: `bytesRead(faultRes: MaybePageFault, address: u32, destination: Uint8Array, destinationOffset: u32): void`. The last parameter is the offset within the destination buffer to start writing at, not the number of bytes to read. The method reads until the destination buffer is filled from destinationOffset to destination.length.
Applied to files:
assembly/memory.ts
📚 Learning: 2025-04-22T15:58:24.284Z
Learnt from: tomusdrw
Repo: tomusdrw/anan-as PR: 49
File: assembly/memory.ts:335-335
Timestamp: 2025-04-22T15:58:24.284Z
Learning: In the bytesWrite method, memory writes should be atomic. If a page fault occurs during a multi-page write, no memory should be altered (current implementation allows partial writes).
Applied to files:
assembly/memory.ts
🧬 Code graph analysis (1)
assembly/memory.ts (1)
assembly/memory-page.ts (1)
PAGE_SIZE(5-5)
🔇 Additional comments (2)
assembly/memory.ts (2)
251-251: LGTM!The JSDoc comment accurately describes the
bytesWritemethod's purpose and parameters.
360-360: LGTM!The JSDoc comment accurately describes the
setBytesmethod's behavior.
Current implementation was always attempting to allocate the destination vector (which could simply be impossible).
This PR patches the behavior by first checking if all pages are accessbile, yet the ultimate solution should be to get rid of this API in favour of a more performant one.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores