actually call near-vm when using VMKind::NearVm #8886
Merged
Conversation
This file contains 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
nagisa
approved these changes
Apr 6, 2023
| multi_memory: WASM_FEATURES.multi_memory, | ||
| memory64: WASM_FEATURES.memory64, | ||
| exceptions: WASM_FEATURES.exceptions, | ||
| mutable_global: true, | ||
| saturating_float_to_int: true, | ||
| sign_extension: true, |
There was a problem hiding this comment.
We went over this before, but double-checking these ideally happens before we roll something out (you mentioned believing these should be true, I still haven't double checked it, though.)
There was a problem hiding this comment.
Got it! We don’t have the protocol change yet and I’ll be asking you for review on it before we roll anything out; I’ll try to remember reminding you that this needs the double-check before we land it :)
There was a problem hiding this comment.
(basically, my recollection of my checking that these need to be true is more along the lines of "tests wouldn’t pass without these to true" than a spec-based checking; which made sense to me as we want to keep the same behavior, but we should also spec-based check as you mention)
There was a problem hiding this comment.
This message seems to confirm that these were on-by-default in previous versions of wasmparser: bytecodealliance/wasm-tools#482
| @@ -533,59 +533,144 @@ impl Wasmer2VM { | |||
| } | |||
| } | |||
|
|
|||
| impl wasmer_vm::Tunables for &Wasmer2VM { | |||
| impl near_vm_vm::Tunables for &NearVmVM { | |||
There was a problem hiding this comment.
So this is a separate crate, that was copy-pasted from wasmer_vm ; if we rename it it’ll collide with the main near_vm crate (that was copy-pasted from the wasmer crate).
So yes, but it’ll have to wait until we merge the near-vm crates all together, as the crate split doesn’t make much sense for us
near-bulldozer bot
pushed a commit
that referenced
this pull request
Apr 18, 2023
This closes up the near-vm work. I believe #8886 (comment) is the only remaining point to be checked in former PRs, in addition to all the code added in this PR. Note that there are still things in #8323 that are not in this PR, because either I noticed they actually should not be merged (eg. the wasmparser bump would be a protocol upgrade, so will wait until we get limited replayability), or because they are actually orthogonal to this landing.
nikurt
pushed a commit
that referenced
this pull request
Apr 18, 2023
* actually call near-vm when using VMKind::NearVm * handle review comments * cargo fmt
nikurt
pushed a commit
that referenced
this pull request
Apr 18, 2023
This closes up the near-vm work. I believe #8886 (comment) is the only remaining point to be checked in former PRs, in addition to all the code added in this PR. Note that there are still things in #8323 that are not in this PR, because either I noticed they actually should not be merged (eg. the wasmparser bump would be a protocol upgrade, so will wait until we get limited replayability), or because they are actually orthogonal to this landing.
near-bulldozer bot
pushed a commit
that referenced
this pull request
Apr 20, 2023
These are not currently supported and any decision to enable them should be a intentional decision, not an accidental one. Overall these changes are no-op since the deserialization will fail earlier when trying to deserialize with pwasm deserializer, but if and when we eventually get rid of it, it would be nice if these extensions didn't accidentally get enabled. This puts to rest the question raised in #8886 (comment).
nikurt
pushed a commit
that referenced
this pull request
Apr 25, 2023
This closes up the near-vm work. I believe #8886 (comment) is the only remaining point to be checked in former PRs, in addition to all the code added in this PR. Note that there are still things in #8323 that are not in this PR, because either I noticed they actually should not be merged (eg. the wasmparser bump would be a protocol upgrade, so will wait until we get limited replayability), or because they are actually orthogonal to this landing.
nikurt
pushed a commit
that referenced
this pull request
Apr 25, 2023
These are not currently supported and any decision to enable them should be a intentional decision, not an accidental one. Overall these changes are no-op since the deserialization will fail earlier when trying to deserialize with pwasm deserializer, but if and when we eventually get rid of it, it would be nice if these extensions didn't accidentally get enabled. This puts to rest the question raised in #8886 (comment).
nikurt
pushed a commit
that referenced
this pull request
Apr 28, 2023
* actually call near-vm when using VMKind::NearVm * handle review comments * cargo fmt
nikurt
pushed a commit
that referenced
this pull request
Apr 28, 2023
This closes up the near-vm work. I believe #8886 (comment) is the only remaining point to be checked in former PRs, in addition to all the code added in this PR. Note that there are still things in #8323 that are not in this PR, because either I noticed they actually should not be merged (eg. the wasmparser bump would be a protocol upgrade, so will wait until we get limited replayability), or because they are actually orthogonal to this landing.
nikurt
pushed a commit
that referenced
this pull request
Apr 28, 2023
These are not currently supported and any decision to enable them should be a intentional decision, not an accidental one. Overall these changes are no-op since the deserialization will fail earlier when trying to deserialize with pwasm deserializer, but if and when we eventually get rid of it, it would be nice if these extensions didn't accidentally get enabled. This puts to rest the question raised in #8886 (comment).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.