Stream: git-wasmtime

Topic: wasmtime / PR #4409 fuzz: add a single instruction module...

Wasmtime GitHub notifications bot (Jul 07 2022 at 18:45):

abrown opened PR #4409 from single-inst-module to main:

As proposed by @cfallin in #3251, this change adds a way to generate a
Wasm module for a single instruction. It captures the necessary
parameter and result types so that fuzzing can not only choose which
instruction to check but also generate values to pass to the
instruction. Not all instructions are available yet, but a significant
portion of scalar instructions are implemented in this change.

This does not wire the generator up to any fuzz targets.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 07 2022 at 18:53):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 18:53):

fitzgen created PR review comment:

You can use a raw string here to avoid the \n and put things on multiple lines.

Wasmtime GitHub notifications bot (Jul 07 2022 at 18:53):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 19:59):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 19:59):

jameysharp created PR review comment:

I like this set of macros!

I notice none of the rust_ty macro arguments are used currently. As an alternative, what about a helper macro like:

macro_rules! wasm_type {
    (i32) => { ValType::I32 };
    ...
    (f64) => { ValType::F64 };
}

Then use Rust types in all macro arguments, and convert them using wasm_type! only when constructing the parameters and results arrays. That makes the macro invocations less verbose and also means you can enhance the macros to make use of the Rust types later. You could define convert! and compare! in one line each at that point.

You could also collapse all the current macros into one like inst!(F64Sub (f64, f64) -> (f64)):

macro_rules! inst {
    ($inst:ident ($($param_ty:ident),*) -> ($($ret_ty:ident),*)) => {
        SingleInstModule {
            instruction: $inst,
            parameters: &[$(wasm_type!($param_ty)),*],
            results: &[$(wasm_type!($ret_ty)),*],
        }
    }
}

Wasmtime GitHub notifications bot (Jul 07 2022 at 19:59):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 19:59):

jameysharp created PR review comment:

I think it might be an improvement to avoid the use of once_cell in this module by making parameters and results be &'a [ValType] instead. (Or drop the 'a lifetime parameter and just use 'static.) The macros below would then use &[...] instead of vec![...], and the instructions would be declared like:

static INSTRUCTIONS: &[SingleInstModule] = &[
    unary!(I32Clz, i32),
    ...
];

Wasmtime GitHub notifications bot (Jul 07 2022 at 19:59):

jameysharp created PR review comment:

Does the module need to be cloned, or can this be implemented for &SingleInstModule<'_> instead?

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:39):

abrown created PR review comment:

Good suggestion! Done.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:39):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:50):

abrown updated PR #4409 from single-inst-module to main.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:55):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:55):

abrown created PR review comment:

Yeah, thanks for the comment; I cleaned things up a bit but not all the way to the final inst! suggestion. In the project this came from, there was more involved and so this was an exercise in "paring down" the original code. I'll leave it as it is in 28f2636 until I see a bit more clearly how this will be used in the actual fuzz target, since that might change the shape of this again.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:56):

abrown updated PR #4409 from single-inst-module to main.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:56):

abrown has marked PR #4409 as ready for review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:56):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:56):

abrown has enabled auto merge for PR #4409.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:00):

alexcrichton created PR review comment:

One nice test to add here as well might be iterating over super::INSTRUCTIONS and verifying that everything which pops out is a valid module.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:20):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:20):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:20):

jameysharp created PR review comment:

This hardly matters but I'll mention it anyway: locals doesn't need to be a Vec. Since Rust 1.53 there's an impl<T, const N: usize> IntoIterator for [T; N], so you can just call Function::new([]).

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:20):

jameysharp created PR review comment:

I guess the changes to Cargo.lock should be removed too.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:30):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:30):

abrown created PR review comment:

Hm, interesting that this didn't go away....

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:31):

abrown updated PR #4409 from single-inst-module to main.

Wasmtime GitHub notifications bot (Jul 07 2022 at 21:32):

abrown updated PR #4409 from single-inst-module to main.

Wasmtime GitHub notifications bot (Jul 07 2022 at 22:01):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 22:51):

abrown merged PR #4409.

Last updated: Nov 22 2024 at 16:03 UTC