Stream: git-wasmtime

Topic: wasmtime / PR #9008 Introduce the `pulley-interpreter` crate

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 23:17):

fitzgen requested alexcrichton for a review on PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 23:17):

fitzgen requested wasmtime-default-reviewers for a review on PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 23:17):

fitzgen opened PR #9008 from fitzgen:add-pulley-interpreter-crate to bytecodealliance:main:

This commit is the first step towards implementing https://github.com/bytecodealliance/rfcs/pull/35

This commit introduces the pulley-interpreter crate which contains the Pulley bytecode definition, encoder, decoder, disassembler, and interpreter.

This is still very much a work in progress! It is expected that we will tweak encodings and bytecode definitions, that we will overhaul the interpreter (to, for example, optionally support the unstable Rust explicit_tail_calls feature), and otherwise make large changes. This is just a starting point to get the ball rolling.

Subsequent commits and pull requests will do things like add the Cranelift backend to produce Pulley bytecode from Wasm as well as the runtime integration to run the Pulley interpreter inside Wasmtime.

Dedicated CI for Pulley, ensuring that it builds and runs on things like 32-bit nostd platforms, will also come in follow ups.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton submitted PR review:

Looks great to me, thanks again for pushing on this!

For some background I've pre-reviewed much of this with Nick ahead of time hence the light comments here as many of my otherwise-comments have already been addressed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

Is it actually possible to do Self::SP => write!(f, "sp") here? I forget what we can do with constants-in-patterns...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton submitted PR review:

Looks great to me, thanks again for pushing on this!

For some background I've pre-reviewed much of this with Nick ahead of time hence the light comments here as many of my otherwise-comments have already been addressed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

Stray fn main?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

One thing I might recommend is #[allow(unused_imports)] at the crate root with a cfg_attr for the "unusual cfgs" or something like that? Along the lines of "default builds have no warnings" but less-than-default builds can have warnings suppressed for ease of use.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

Mind adding a small test that the registers above all assert!(thing.is_special())?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

This won't get executed on oss-fuzz, how do you feel about that?

If you're ok with it I might recommend a wasm-tools-style approach where the fuzz crate here isn't actually a cargo fuzz crate but has a single entrypoint that takes fuzz input and then uses the input to dictate which fuzzer is internally run. Then we'd add a single fuzzer at the crate root for pulley which would internally run all of pulley's fuzzers.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

Mind adding #![cfg_attr(docsrs, feature(doc_auto_cfg))] to the crate here too?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

Perhaps all-features = true instead?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

Reading over this again, WDYT about something like:

fn advance<const N: usize>(&mut self) -> Result<[u8; N], Self::Error>;

It feels a little weird below that in SafeBytecodeStream the advance method looks like it can cause panics but I think it probably never does in practice. By folding everything into one method though it might be reasonable to make this more obvious that the safe stream never panics?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 15:59):

alexcrichton created PR review comment:

This seems a bit dangerous, should this perhaps be omitted? Or maybe it always generates 0 for the time being?

(seems like the "real fix" is a higher-level generator that generates semantically correct offsets)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:03):

alexcrichton commented on PR #9008:

Dedicated CI for Pulley

One thing I'd also recommend here is to run all tests for pulley in miri as well (or otherwise just add it to our list of tests-in-miri)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:08):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:08):

fitzgen created PR review comment:

Tried that first, unfortunately it isn't allowed :(

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:08):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:08):

fitzgen created PR review comment:

Good eye

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:14):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:14):

fitzgen created PR review comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:15):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:15):

fitzgen created PR review comment:

Turns out this import isn't used at all anymore...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:21):

fitzgen created PR review comment:

Changed this to always choose zero.

Either way, whoever is working with the arbitrary ops is going to have to clean them up somehow if they are interpreting them. In the existing fuzz target for the interpreter we simply filter out instructions that we can't safely interpret, such as branches.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:21):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:25):

fitzgen updated PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:41):

fitzgen updated PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:41):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:41):

fitzgen created PR review comment:

This worked out nicely.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 16:52):

fitzgen updated PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 17:33):

fitzgen updated PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 17:34):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 17:34):

fitzgen created PR review comment:

Alright, I merged the fuzz targets together into one, like wasm-tools.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 17:34):

fitzgen commented on PR #9008:

One thing I'd also recommend here is to run all tests for pulley in miri as well (or otherwise just add it to our list of tests-in-miri)

Will do as a follow up, with the rest of pulley-specific CI.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 17:34):

fitzgen has enabled auto merge for PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 20:06):

fitzgen updated PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 20:06):

fitzgen has enabled auto merge for PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 21:40):

fitzgen requested elliottt for a review on PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 21:40):

fitzgen updated PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 21:40):

fitzgen requested wasmtime-fuzz-reviewers for a review on PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 21:40):

fitzgen has enabled auto merge for PR #9008.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 22:04):

fitzgen merged PR #9008.

Last updated: Nov 22 2024 at 16:03 UTC