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

feat: parse PVM program blob #43

Merged
merged 4 commits into from
Aug 23, 2024
Merged

feat: parse PVM program blob #43

merged 4 commits into from
Aug 23, 2024

Conversation

danielvladco
Copy link
Member

@danielvladco danielvladco commented Aug 14, 2024

Parse the program binary.

This initial iteration adds the program parsing component, it takes in a bytes reader and decodes each section of the program into jump table, a set of instructions, import and export functions.

The logic is based on https://github.com/koute/polkavm

@danielvladco danielvladco requested a review from a team August 14, 2024 15:13
@danielvladco danielvladco self-assigned this Aug 14, 2024
@danielvladco danielvladco force-pushed the feat/parse-pvm-program branch from 4b57e7f to aadb98b Compare August 14, 2024 15:14
@danielvladco danielvladco force-pushed the feat/parse-pvm-program branch from aadb98b to 24fc849 Compare August 14, 2024 15:35
@danielvladco danielvladco mentioned this pull request Aug 14, 2024
13 tasks
internal/polkavm/program.go Outdated Show resolved Hide resolved
internal/polkavm/program.go Outdated Show resolved Hide resolved
@danielvladco danielvladco force-pushed the feat/parse-pvm-program branch from a69a248 to 967f457 Compare August 19, 2024 15:49
@asmie
Copy link
Member

asmie commented Aug 21, 2024

The overall comment - maybe we should introduce a division into Polka Host - so something that's reading the blob, preparing all the data and structures, and runs the second part - so the PVM itself.

internal/polkavm/program.go Outdated Show resolved Hide resolved
internal/polkavm/program.go Show resolved Hide resolved
internal/polkavm/program.go Show resolved Hide resolved
internal/polkavm/program.go Show resolved Hide resolved
@danielvladco danielvladco force-pushed the feat/parse-pvm-program branch from e6bd29b to 5ad5d0c Compare August 22, 2024 14:55
@danielvladco danielvladco force-pushed the feat/parse-pvm-program branch from 5ad5d0c to b04ff15 Compare August 22, 2024 15:02
@danielvladco danielvladco requested review from asmie and pantrif August 22, 2024 15:05
@aranw
Copy link

aranw commented Aug 23, 2024

Most of this all looks okay to me from what I can tell. But something that does stand out is the number of panics? Do we want to instead return errors rather than panic in this code? I realise you've marked a few of these as unreachable but just wanted to raise it and discuss it

Copy link
Member

@asmie asmie left a comment

Choose a reason for hiding this comment

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

instructions.go seems to be not tied with parsing program blob but let's merge it and go on with the instruction decoder.

@danielvladco
Copy link
Member Author

danielvladco commented Aug 23, 2024

@aranw , the panics are there for cases that should never happen, this is why I put panics as we expect those code parts to be "unreachable" and if we get there something went terribly wrong, in rust they use assert and unreachable macro for this I think

@danielvladco danielvladco merged commit ca9ea1c into main Aug 23, 2024
2 checks passed
@danielvladco danielvladco deleted the feat/parse-pvm-program branch August 23, 2024 11:58
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.

4 participants