Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ecalli handling for other pvms #285

Merged
merged 16 commits into from
Jan 17, 2025
Merged

Add ecalli handling for other pvms #285

merged 16 commits into from
Jan 17, 2025

Conversation

krystian50
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for pvm-debugger ready!

Name Link
🔨 Latest commit cf876e0
🔍 Latest deploy log https://app.netlify.com/sites/pvm-debugger/deploys/678a463e71a25700085f7bce
😎 Deploy Preview https://deploy-preview-285--pvm-debugger.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@krystian50 krystian50 force-pushed the host-calls-multi-pvms branch from 8614d41 to 130fe5e Compare January 15, 2025 20:15
@krystian50 krystian50 marked this pull request as ready for review January 15, 2025 20:23
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

I can't test if it works, because ananas and polkavm are already updated to 0.5.4 (see #291)
but let's merge as-is and test later.

src/packages/web-worker/command-handlers/host-call.ts Outdated Show resolved Hide resolved
src/packages/web-worker/command-handlers/host-call.ts Outdated Show resolved Hide resolved
src/packages/web-worker/command-handlers/host-call.ts Outdated Show resolved Hide resolved
src/packages/web-worker/command-handlers/host-call.ts Outdated Show resolved Hide resolved
}

storeFrom(address: MemoryIndex, bytes: Uint8Array) {
this.pvm.setMemory(address, bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.pvm.setMemory(address, bytes);
// TODO [ToDr] Either change the API to require handling multi-page writes or change this code to split the write into multiple pages.
this.pvm.setMemory(address, bytes);

payload: { hostCallIdentifier: worker.exitArg as HostCallIdentifiers },
});
if (
resp.payload.hostCallIdentifier === HostCallIdentifiers.WRITE &&
Copy link
Contributor

Choose a reason for hiding this comment

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

These HostCallIndentifiers should rather come from host calls impls (new Write().index) to avoid duplicating this stuff in case it ever changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I'm seeing it's only exported as

import { write } from "@typeberry/jam-host-calls";
new write.Write().index

which is fine, but it requires account param. Maybe you could add index as a static property?

@krystian50 krystian50 merged commit 7d522d4 into main Jan 17, 2025
6 checks passed
@krystian50 krystian50 deleted the host-calls-multi-pvms branch January 17, 2025 12:12
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