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

replace parity_wasm #23

Merged
merged 23 commits into from
Apr 19, 2023
Merged

replace parity_wasm #23

merged 23 commits into from
Apr 19, 2023

Conversation

shishkin-pavel
Copy link
Collaborator

@shishkin-pavel shishkin-pavel commented Mar 30, 2023

paritytech/wasm-instrument#3 (comment)

wasm_translate.rs was initially copied from https://github.com/bytecodealliance/wasm-tools/blob/main/crates/wasm-mutate/src/mutators/translate.rs
and murdered after that

there is many things to be redone properly, but general direction (is quite straightforward) will probably remain similar

main pain-points:

  • poor api for mutating builder
  • todos in parse func (mostly because of wasm components or non-standard wasm features)
  • not finalized translation in some cases (like Translator::const_expr & ConstExprKind)

@shishkin-pavel shishkin-pavel marked this pull request as ready for review March 30, 2023 15:48
@shishkin-pavel shishkin-pavel requested a review from birchmd March 30, 2023 15:48
@shishkin-pavel
Copy link
Collaborator Author

seems like near runtime cannot parse that wasm module, will fix tomorrow
but it works in wasmtime

Copy link
Collaborator

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

LGTM! Nice job!

Just a few comments for some nits.

@@ -195,7 +185,7 @@ impl Compiler {
/// contract's ABI, enabling users to directly call a contract method
/// without going through the low-level `execute` EVM dispatcher.
pub fn emit_abi_methods(
self: &mut Compiler,
self: &mut Compiler<'a>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to this PR, but I think we could write this simply as &mut self

}
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This continue doesn't seem needed. It's at the bottom of the loop body, so the only thing left to do it continue to the next iteration.

}
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

}
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

for tag in tag_section {
let tag = tag?;
let _tag_type = tag_type(&tag)?;
panic!("unspecified section (spec doesnt have that one! WTF)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should make an issue to come back to this when the spec is updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as far as i understand, its near wasm runtime limitation at the moment
wasm-core-2 actually support data count section
and tag section appears to be supported in exception proposal
so probably we should follow near runtime instead of wasm spec itself

}
}
Payload::DataCountSection { count: _, range: _ } => {
panic!("unspecified section (spec doesnt have that one! WTF)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

@shishkin-pavel shishkin-pavel merged commit b1c8e70 into master Apr 19, 2023
shishkin-pavel added a commit that referenced this pull request Jul 31, 2023
obsolete parity_wasm lib replaced with wasm-encoder + wasmparser
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