fix: handle typeberry assertion on high-address faults#6
Merged
Conversation
Typeberry's getStartPageIndex() uses signed << without >>> 0, causing assertion errors when programs fault at addresses >= 0x80000000. The adapter now catches these and reports fault status, matching Ananas. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for pvm-debugger ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughTypeberrySyncInterpreter is enhanced to catch Typeberry assertion errors during step execution, converting them into a synthetic fault status instead of propagating exceptions. The fault status is managed through load and reset lifecycle methods. Supporting test suite and documentation added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-worker/src/adapters/typeberry.ts`:
- Around line 127-145: The step(n: number) method must short-circuit when a
synthetic fault is already latched: check this.assertionFaultStatus ===
TYPEBERRY_STATUS_FAULT at the start of step and immediately return { finished:
true } (without calling this.adapter.nextStep() or this.adapter.nSteps(n)); keep
the existing assertion-fault detection logic that sets this.assertionFaultStatus
in the catch block but ensure subsequent step() calls do not delegate into the
adapter.
In `@packages/runtime-worker/src/index.test.ts`:
- Around line 855-871: The loop currently allows 10 iterations to complete
without asserting that a terminal fault was reached; update the test to track
termination by introducing a boolean (e.g., observedTermination) set to true
when either tbResult.finished or anResult.finished is true (inside the existing
if block where you already assert tbStatus and anStatus are "fault"), and after
the for loop add an assertion expect(observedTermination).toBe(true) to fail the
test if neither interpreter ever reaches the terminal fault; reference
typeberry.step, ananas.step, typeberry.getStatus, ananas.getStatus and mapStatus
to locate where to set the flag and add the final assertion.
In `@spec/ui/sprint-48-typeberry-assertion-fault-workaround.md`:
- Around line 43-45: The page-alignment example is incorrect: the expression
`(0xFFFFF >>> 12) << 12` and its claimed result `0xFFFFF000` (signed -4096) is
wrong and misleading; update the prose and example around getStartPageIndex and
tryAsMemoryIndex to show the correct bit-shift behavior and resulting aligned
address (use the correct original address that leads to `0xFFFFF000` or adjust
the shifted result to match the shown input such as using `0xFFFFFFF8` =>
alignment to `0xFFFFFFF000`), and explain that getStartPageIndex aligns
addresses by zeroing lower 12 bits so tryAsMemoryIndex will receive the aligned
(unsigned) page address rather than -4096. Ensure references to
getStartPageIndex and tryAsMemoryIndex remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a05ae99-c26f-410e-a8c3-6fa95b47e74e
📒 Files selected for processing (4)
packages/runtime-worker/src/adapters/typeberry.tspackages/runtime-worker/src/index.test.tsspec/005-runtime-worker-and-adapters.mdspec/ui/sprint-48-typeberry-assertion-fault-workaround.md
Comment on lines
127
to
+145
| step(n: number): { finished: boolean } { | ||
| if (n === 1) { | ||
| const running = this.adapter.nextStep(); | ||
| try { | ||
| if (n === 1) { | ||
| const running = this.adapter.nextStep(); | ||
| return { finished: !running }; | ||
| } | ||
| const running = this.adapter.nSteps(n); | ||
| return { finished: !running }; | ||
| } catch (err) { | ||
| // Typeberry has a known bug where its fault-handling code path throws | ||
| // assertion errors (e.g. getStartPageIndex uses signed << on high | ||
| // addresses, producing negative values that fail tryAsMemoryIndex). | ||
| // The instruction itself was correctly identified as a fault, but the | ||
| // error-reporting code crashed. Treat this as a fault. | ||
| if (err instanceof Error && err.message.includes("Assertion failure")) { | ||
| this.assertionFaultStatus = TYPEBERRY_STATUS_FAULT; | ||
| return { finished: true }; | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
Make the synthetic fault terminal across subsequent step() calls.
Once assertionFaultStatus is latched, step() still delegates into Typeberry on the next call. That can re-enter the same broken path or mutate state behind a permanently forced "fault" status. Short-circuit immediately when the synthetic fault is already set.
Suggested fix
step(n: number): { finished: boolean } {
+ if (this.assertionFaultStatus !== null) {
+ return { finished: true };
+ }
+
try {
if (n === 1) {
const running = this.adapter.nextStep();
return { finished: !running };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| step(n: number): { finished: boolean } { | |
| if (n === 1) { | |
| const running = this.adapter.nextStep(); | |
| try { | |
| if (n === 1) { | |
| const running = this.adapter.nextStep(); | |
| return { finished: !running }; | |
| } | |
| const running = this.adapter.nSteps(n); | |
| return { finished: !running }; | |
| } catch (err) { | |
| // Typeberry has a known bug where its fault-handling code path throws | |
| // assertion errors (e.g. getStartPageIndex uses signed << on high | |
| // addresses, producing negative values that fail tryAsMemoryIndex). | |
| // The instruction itself was correctly identified as a fault, but the | |
| // error-reporting code crashed. Treat this as a fault. | |
| if (err instanceof Error && err.message.includes("Assertion failure")) { | |
| this.assertionFaultStatus = TYPEBERRY_STATUS_FAULT; | |
| return { finished: true }; | |
| } | |
| throw err; | |
| step(n: number): { finished: boolean } { | |
| if (this.assertionFaultStatus !== null) { | |
| return { finished: true }; | |
| } | |
| try { | |
| if (n === 1) { | |
| const running = this.adapter.nextStep(); | |
| return { finished: !running }; | |
| } | |
| const running = this.adapter.nSteps(n); | |
| return { finished: !running }; | |
| } catch (err) { | |
| // Typeberry has a known bug where its fault-handling code path throws | |
| // assertion errors (e.g. getStartPageIndex uses signed << on high | |
| // addresses, producing negative values that fail tryAsMemoryIndex). | |
| // The instruction itself was correctly identified as a fault, but the | |
| // error-reporting code crashed. Treat this as a fault. | |
| if (err instanceof Error && err.message.includes("Assertion failure")) { | |
| this.assertionFaultStatus = TYPEBERRY_STATUS_FAULT; | |
| return { finished: true }; | |
| } | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/runtime-worker/src/adapters/typeberry.ts` around lines 127 - 145,
The step(n: number) method must short-circuit when a synthetic fault is already
latched: check this.assertionFaultStatus === TYPEBERRY_STATUS_FAULT at the start
of step and immediately return { finished: true } (without calling
this.adapter.nextStep() or this.adapter.nSteps(n)); keep the existing
assertion-fault detection logic that sets this.assertionFaultStatus in the catch
block but ensure subsequent step() calls do not delegate into the adapter.
Comment on lines
+855
to
+871
| for (let i = 0; i < 10; i++) { | ||
| const tbResult = typeberry.step(1); | ||
| const anResult = ananas.step(1); | ||
|
|
||
| const tbStatus = mapStatus(typeberry.getStatus()); | ||
| const anStatus = mapStatus(ananas.getStatus()); | ||
|
|
||
| expect(tbResult.finished).toBe(anResult.finished); | ||
| expect(tbStatus).toBe(anStatus); | ||
|
|
||
| if (tbResult.finished || anResult.finished) { | ||
| // Both should agree it's a fault | ||
| expect(tbStatus).toBe("fault"); | ||
| expect(anStatus).toBe("fault"); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail the test if neither interpreter ever reaches the terminal fault.
This loop can exit after 10 iterations with no finished state and still pass, which masks the regression this test is supposed to catch. Track whether termination was observed and assert it after the loop.
Suggested fix
it("both PVMs agree on fault after store to unmapped memory", () => {
typeberry.load(doomBytes, doomState);
ananas.load(doomBytes, doomState);
// Step until one terminates (doom faults on step 2 due to unmapped memory store)
+ let sawTerminalState = false;
for (let i = 0; i < 10; i++) {
const tbResult = typeberry.step(1);
const anResult = ananas.step(1);
@@
if (tbResult.finished || anResult.finished) {
+ sawTerminalState = true;
// Both should agree it's a fault
expect(tbStatus).toBe("fault");
expect(anStatus).toBe("fault");
break;
}
}
+
+ expect(sawTerminalState).toBe(true);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/runtime-worker/src/index.test.ts` around lines 855 - 871, The loop
currently allows 10 iterations to complete without asserting that a terminal
fault was reached; update the test to track termination by introducing a boolean
(e.g., observedTermination) set to true when either tbResult.finished or
anResult.finished is true (inside the existing if block where you already assert
tbStatus and anStatus are "fault"), and after the for loop add an assertion
expect(observedTermination).toBe(true) to fail the test if neither interpreter
ever reaches the terminal fault; reference typeberry.step, ananas.step,
typeberry.getStatus, ananas.getStatus and mapStatus to locate where to set the
flag and add the final assertion.
Comment on lines
+43
to
+45
| 5. In the **fault-handling code path** (not the address calculation), `getStartPageIndex(tryAsMemoryIndex(storeResult.error.address))` is called to compute the page index for the error report. | ||
| 6. `getStartPageIndex` aligns the error address to a page boundary: `(0xFFFFF >>> 12) << 12` = `0xFFFFF000` as unsigned, but `-4096` as signed 32-bit. | ||
| 7. `tryAsMemoryIndex(-4096)` throws `Assertion failure: Incorrect memory index: -4096!`. |
There was a problem hiding this comment.
Correct the page-alignment example.
The arithmetic here is off: 0xFFFFF >>> 12 cannot produce 0xFFFFF000 after shifting back. The example result matches an address like 0xFFFFFFF8, so the current text is misleading.
Suggested fix
-5. In the **fault-handling code path** (not the address calculation), `getStartPageIndex(tryAsMemoryIndex(storeResult.error.address))` is called to compute the page index for the error report.
-6. `getStartPageIndex` aligns the error address to a page boundary: `(0xFFFFF >>> 12) << 12` = `0xFFFFF000` as unsigned, but `-4096` as signed 32-bit.
+5. In the **fault-handling code path** (not the address calculation), `getStartPageIndex(tryAsMemoryIndex(storeResult.error.address))` is called to compute the page index for the error report.
+6. `getStartPageIndex` aligns the error address to a page boundary: `(0xFFFFFFF8 >>> 12) << 12` = `0xFFFFF000` as unsigned, but `-4096` as signed 32-bit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/ui/sprint-48-typeberry-assertion-fault-workaround.md` around lines 43 -
45, The page-alignment example is incorrect: the expression `(0xFFFFF >>> 12) <<
12` and its claimed result `0xFFFFF000` (signed -4096) is wrong and misleading;
update the prose and example around getStartPageIndex and tryAsMemoryIndex to
show the correct bit-shift behavior and resulting aligned address (use the
correct original address that leads to `0xFFFFF000` or adjust the shifted result
to match the shown input such as using `0xFFFFFFF8` => alignment to
`0xFFFFFFF000`), and explain that getStartPageIndex aligns addresses by zeroing
lower 12 bits so tryAsMemoryIndex will receive the aligned (unsigned) page
address rather than -4096. Ensure references to getStartPageIndex and
tryAsMemoryIndex remain consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
faultstatus@typeberry/lib—getStartPageIndex()uses signed<<without>>> 0, producing negative values for addresses >=0x80000000in the fault-handling code pathTypeberrySyncInterpreter.step()catches assertion errors and reports synthetic fault status (code 2), cleared on load/resetTest plan
faultstatus at step 2 (unmapped memory store)typeberry.step(100)returns{ finished: true }without throwingreset()clears synthetic fault status back took🤖 Generated with Claude Code