Skip to content

Conversation

@DrEverr
Copy link
Collaborator

@DrEverr DrEverr commented Oct 16, 2025

deleted memory.free() and logging exit code

Summary by CodeRabbit

  • New Features

    • Added a memory inspection tool to retrieve arbitrary memory ranges as byte arrays.
  • Improvements

    • Enhanced error logging to include exit codes for failed runs.
    • Adjusted program execution resource handling to change when memory is released.
  • Documentation

    • README updated to "JAM PVM (64-bit)" with reorganized sections, clearer installation/usage instructions, and updated examples.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

Walkthrough

Added a debugger memory-read API getMemory(address: u32, length: u32): Uint8Array. executeProgram now logs exit codes for non-ok outcomes and no longer calls explicit memory free at the end. README updated wording, headings, and installation/usage sections; project name changed to "JAM PVM (64-bit)".

Changes

Cohort / File(s) Summary
Debugger API Memory Access
assembly/api-debugger.ts
Added exported getMemory(address: u32, length: u32): Uint8Array. Returns an empty array if interpreter is null or a memory read fault occurs; otherwise fills and returns a buffer via interpreter.memory.bytesRead with a MaybePageFault.
Internal Execution Logging & Memory Management
assembly/api-internal.ts
In runVm, memory pages are freed (int.memory.free()) after executeProgram returns. In executeProgram, added logging of the exit code when outcome is non-ok and removed the explicit int.memory.free() call at the end.
Documentation / README
README.md
Updated project header from "JAM PVM (32bit)" to "JAM PVM (64-bit)"; restructured headings (Todo, Why, Useful where), added Installation and Usage sections, corrected wording/typos, standardized command fence language specifiers, and applied minor formatting/reflow edits.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant DebuggerAPI as api-debugger
    participant Interpreter
    Note over DebuggerAPI,Interpreter #DDEBF7: getMemory flow
    Client->>DebuggerAPI: getMemory(address, length)
    DebuggerAPI->>Interpreter: check interpreter
    alt interpreter null
        DebuggerAPI-->>Client: return Uint8Array([]) 
    else
        DebuggerAPI->>Interpreter: memory.bytesRead(address, length, MaybePageFault)
        alt page fault
            DebuggerAPI-->>Client: return Uint8Array([]) 
        else success
            DebuggerAPI-->>Client: return filled Uint8Array
        end
    end
Loading
sequenceDiagram
    participant Caller as runVm/Caller
    participant Executor as executeProgram
    participant Memory as int.memory
    participant Logger
    Note over Executor,Memory #FFF2CC: Execution end behavior change
    Caller->>Executor: executeProgram(...)
    Executor->>Logger: log outcome
    alt outcome != ok
        Executor->>Logger: log "Exit code: {int.exitCode}"
    end
    Caller->>Memory: int.memory.free()  -- moved here (in runVm)
    Note right of Executor: Previously: Memory.free() called inside executeProgram
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Decode standard program #78 — Modifies VM execution flow and memory free placement similarly; likely related to memory free ordering changes.

Suggested reviewers

  • tomusdrw

Poem

🐇 I nibble bytes behind the scenes,
I hop through pages, soft and keen.
If faults appear I leave a blank,
I log a sigh beside the bank.
New reads and logs — a tiny scene.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Added getMemory to api-debugger" is specific and accurately describes a concrete change in the pull request: the addition of a new exported function getMemory(address: u32, length: u32): Uint8Array to assembly/api-debugger.ts. The title is clear and non-vague, directly referencing an actual modification present in the changeset. However, the pull request includes other significant changes beyond getMemory, notably memory management refactoring in assembly/api-internal.ts (moving memory.free() from executeProgram to runVm) and enhanced logging, along with substantial README documentation updates. The title captures one important aspect of the change but does not encompass the full scope of the modifications.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b0e2b2a and 3086b43.

📒 Files selected for processing (1)
  • assembly/api-internal.ts (2 hunks)
🔇 Additional comments (2)
assembly/api-internal.ts (2)

152-152: LGTM—useful debugging enhancement.

Logging the exit code when execution fails provides valuable diagnostic information and is consistent with the other log statements in this block.


78-81: Good fix—addresses previous reviewer feedback and is safe.

Moving memory.free() to runVm correctly follows the ownership principle: runVm creates the Interpreter, so it should manage cleanup. The memory state is captured in output.memory via getOutputChunks (line 185) before the free call, ensuring no data loss. The only external call to runVm (in api-utils.ts:95) immediately returns the result without accessing memory afterward, so there's no risk of use-after-free.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
assembly/api-internal.ts (1)

178-186: Reintroduce int.memory.free() after extracting memory chunks to avoid leaks
executeProgram no longer frees int.memory, and neither runVm nor its callers free it, leading to a potential memory leak; re-add int.memory.free() after getOutputChunks(int.memory) or ensure cleanup elsewhere.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 791e5e1 and 9a1098f.

📒 Files selected for processing (2)
  • assembly/api-debugger.ts (1 hunks)
  • assembly/api-internal.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
assembly/api-debugger.ts (1)
assembly/interpreter.ts (1)
  • Interpreter (21-200)
🪛 GitHub Actions: Node.js CI
assembly/api-debugger.ts

[error] 176-176: TS2554: Expected 4 arguments, but got 2. (in assembly/api-debugger.ts:176)


[error] 176-176: TS2322: Type 'void' is not assignable to type ''. (in assembly/api-debugger.ts:176)


[error] 177-177: TS2304: Cannot find name 'fault'. (in assembly/api-debugger.ts:177)

🔇 Additional comments (1)
assembly/api-internal.ts (1)

150-150: LGTM! Improved error diagnostics.

The addition of exit code logging enhances debugging capabilities by providing more context when execution fails.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a1098f and 2e19ccf.

📒 Files selected for processing (1)
  • assembly/api-debugger.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
assembly/api-debugger.ts (2)
assembly/interpreter.ts (1)
  • Interpreter (21-200)
assembly/memory.ts (1)
  • MaybePageFault (15-19)

@DrEverr DrEverr requested a review from tomusdrw October 16, 2025 11:48
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
README.md (1)

3-3: Use canonical project name AssemblyScript.

Official naming keeps it as one word. Adjusting avoids confusion in docs.

-Assembly Script implementation of the JAM PVM (64bit).
+AssemblyScript implementation of the JAM PVM (64bit).
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e19ccf and c641288.

📒 Files selected for processing (1)
  • README.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[grammar] ~3-~3: Ensure spelling is correct
Context: ...y Script implementation of the JAM PVM (64bit). Demo ###...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
README.md (2)

95-96: Standardize shell fence language to bash

Commands are OS‑agnostic. Using bash aligns with other blocks and supports inline comments if needed.

-```cmd
+```bash
 npm ci
-```cmd
+```bash
 npm run build
-```cmd
+```bash
 npm run web
-```cmd
+```bash
 npm start ./path/to/tests/*.json

Also applies to: 101-102, 107-108, 113-114


56-61: Document the new debugger getMemory API (example usage)

Add a brief note and example so users can discover it easily from README.

Proposed addition after the Raw Bindings paragraph:

### Debugger API

The debugger exposes helpers for inspecting memory. For example, `getMemory(address: u32, length: u32): Uint8Array` returns a slice of linear memory.

ESM:
```javascript
import ananAs from '@fluffylabs/anan-as/debug';
// ... after running a program
const bytes = ananAs.getMemory(0x1000, 64);
console.log(bytes);

Raw bindings:

import { instantiate } from '@fluffylabs/anan-as/raw';
const { exports } = await instantiate(module);
// ... after running a program
const bytes = exports.getMemory(0x1000, 64);
console.log(bytes);

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between c641288cdf32930efa0f511f28e4fa0b43fab774 and 42a544665952f6fe01ec6cae3900108314d2c1fa.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

* `README.md` (4 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🪛 LanguageTool</summary>

<details>
<summary>README.md</summary>

[grammar] ~3-~3: Ensure spelling is correct
Context: ...y Script implementation of the JAM PVM (64bit).  [Demo](https://todr.me/anan-as)  ## ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~9-~9: There might be a mistake here.
Context: .../todr.me/anan-as)  ## Todo  - [x] Memory - [x] [JAM tests](https://github.com/w3f/j...

(QB_NEW_EN)

---

[grammar] ~10-~10: There might be a mistake here.
Context: ...w3f/jamtestvectors/pull/3) compatibility - [x] 64-bit & new instructions ([GrayPape...

(QB_NEW_EN)

---

[grammar] ~11-~11: There might be a mistake here.
Context: ....5.0](https://graypaper.fluffylabs.dev)) - [x] GP 0.5.4 compatibility (ZBB extensio...

(QB_NEW_EN)

---

[grammar] ~16-~16: There might be a mistake here.
Context: .../en.wikipedia.org/wiki/Ananas) are cool. - [JAM](https://graypaper.com/) is promisin...

(QB_NEW_EN)

---

[grammar] ~80-~80: There might be a mistake here.
Context: ..., you can choose between stable releases and bleeding-edge builds:  ```bash # Lat...

(QB_NEW_EN)

---

[style] ~110-~110: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...calhost:3000).  ```cmd npm run web ```  To run JSON test vectors.  ```cmd npm star...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (4)</summary><blockquote>

<details>
<summary>README.md (4)</summary><blockquote>

`7-7`: **Heading level change looks good**

Promoting “Todo” to H2 improves structure.

---

`14-14`: **Heading level change looks good**

Consistent H2 hierarchy.

---

`20-20`: **Heading level change looks good**

Clearer sectioning.

---

`78-82`: **Version tags section LGTM**

Clear guidance for stable vs. next.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

tomusdrw and others added 2 commits October 16, 2025 17:19
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@DrEverr DrEverr requested a review from tomusdrw October 17, 2025 08:32
@tomusdrw tomusdrw merged commit 5213fac into tomusdrw:main Oct 17, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants