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(runtime): Wasmer 1.0 runner #3799

Merged
merged 60 commits into from
Feb 23, 2021
Merged

feat(runtime): Wasmer 1.0 runner #3799

merged 60 commits into from
Feb 23, 2021

Conversation

ailisp
Copy link
Member

@ailisp ailisp commented Jan 12, 2021

Add Wasmer 1.0 runner, enabled by wasmer1_vm features (similar to wasmtime_vm). Expose wasmer 1.0 runner and wasmtime runner available to neard and vm standalone.

Test Plan

All near-vm-runner tests pass. compatible behavior as wasmer 0.17. neard compiles and work with wasmer 1.0 runner turns on

@ailisp ailisp requested a review from olonho January 12, 2021 01:49
@ailisp ailisp requested a review from willemneal January 12, 2021 02:07
@ailisp ailisp marked this pull request as ready for review January 15, 2021 00:08
runtime/near-vm-runner/src/cache.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/errors.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/imports.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/runner.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/runner.rs Outdated Show resolved Hide resolved
Comment on lines +127 to +128
let func = info.functions.get(index.clone()).unwrap();
let sig = info.signatures.get(func.clone()).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is unwrap safe here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not answered

Copy link
Member Author

Choose a reason for hiding this comment

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

It is safe, this function is almost identical to wasmer_runner::check_method, just wasmer_runtime_core::module::ExportIndex::Func => wasmer_types::ExportIndex::Function refactor done by wasmer. as i checked wasmer code, when Function(index) is in module.info().exports, corresponding index always exist in info.functions, say it is func, and func is always in info.signatures

runtime/near-vm-runner/src/wasmer1_runner.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

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

Please address nit comments first

@@ -306,6 +314,19 @@ impl From<PrepareError> for VMError {
}
}

impl From<&VMLogicError> for VMError {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: why we use &VMLogicError instead of VMLogicError?

Copy link
Member Author

Choose a reason for hiding this comment

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

There're two calls this with e.into(), one e is a &VMLogicError, another is VMLogicError, so make a From<&VMLogicError> works for both

runtime/near-vm-runner/src/errors.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/imports.rs Show resolved Hide resolved
Comment on lines +127 to +128
let func = info.functions.get(index.clone()).unwrap();
let sig = info.signatures.get(func.clone()).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not answered

@ailisp ailisp merged commit 79f1732 into master Feb 23, 2021
@ailisp ailisp deleted the wasmer-1.0 branch February 23, 2021 01:38
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.

7 participants