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

Implement polymorphic testing against WASMTime #16

Closed

Conversation

george-cosma
Copy link
Collaborator

@george-cosma george-cosma commented Jul 1, 2024

Pull Request Overview

This PR (mainly) adds an abstraction layer over WASMTime, and created some functions to (slightly) simplify running the code.

I'm just going to showcase two test cases, rewritten with this abstraction layer in-place:

/// Two simple methods for storing and loading an i32 from the first slot in linear memory.
#[test_log::test]
fn basic_memory() {
    use wasm::{validate, RuntimeInstance};

    let wat = r#"
    (module
        (memory 1)
        (func (export "store_num") (param $x i32)
            i32.const 0
            local.get $x
            i32.store)
        (func (export "load_num") (result i32)
            i32.const 0
            i32.load)
    )
    "#;

    let wasm_bytes = wat::parse_str(wat).unwrap();
    let validation_info = validate(&wasm_bytes).expect("validation failed");
    let instance = RuntimeInstance::new(&validation_info).expect("instantiation failed");
    let wasmtime_instance =
        WASMTimeRunner::new(wat, ()).expect("wasmtime runner failed to instantiate");

    let mut runners = [instance.into(), wasmtime_instance.into()];

    // poly_test(params, expected_output, function_id, function_name, runners);
    poly_test_once(42, (), 0, "store_num", &mut runners);
    poly_test_once((), 42, 1, "load_num", &mut runners);
}

And for a function in which a specific function is tested multiple times:

/// A simple function to add 1 to an i32 and return the result
#[test_log::test]
fn add_one() {
    use wasm::{validate, RuntimeInstance};

    let wat = r#"
    (module
        (func (export "add_one") (param $x i32) (result i32)
            local.get $x
            i32.const 1
            i32.add)
    )
    "#;
    let wasm_bytes = wat::parse_str(wat).unwrap();
    let validation_info = validate(&wasm_bytes).expect("validation failed");
    let instance = RuntimeInstance::new(&validation_info).expect("instantiation failed");
    let wasmtime_instance =
        WASMTimeRunner::new(wat, ()).expect("wasmtime runner failed to instantiate");

    let mut runners = [
        FunctionRunner::new(instance.into(), 0, "add_one"),
        FunctionRunner::new(wasmtime_instance.into(), 0, "add_one"),
    ];

    // poly_test(params, expected, runners)
    poly_test(11, 12, &mut runners);
    poly_test(0, 1, &mut runners);
    poly_test(-5, -4, &mut runners);
}

So, to recap what the new shiny things are:

  1. WASMTimeRunner - the aforementioned abstraction layer over wasmtime. The two parameters it has is (unparsed) the wat, and initial value for the Store.
  2. Runner - a simple enum that basically provides a common-ish interface for executing a function. Not going to lie, now that I am writing this this could've been a trait rather than an enum.
  3. FunctionRunner - a wrapper around a runner, but meant to run only a single function, Useful to avoid writing again and again the function id and function name when doing poly_test_once
  4. poly_test_once - run a function on all the runners using the same parameters and expecting the same thing from every single one of them and does an assert_eq!
  5. poly_test - same, but for FunctionRunner(s)

Testing Strategy

This pull request was tested by running cargo test and making the tests fail, and ensuring that they fail.

TODO or Help Wanted

This proposal isn't 100% functioning. For example, I am struggling to make the tests/arithmetic/multiply.rs test be able to import the common utilities from tests/common/.... Since all tests are compiled stand-alone as their own crate, and since multiply.rs is nested inside, it can't find the common elements. I'm open for suggestions on how to tackle this.

Another aspect is the utility of all of these layers. Do we need FunctionRunner or am I overengineering things? Similarly, when making the code I've tried to keep it easy to extend with another runtime. In the original issue wasmi and wasmtime were both mentioned as possible runtimes to check against. Why not do both? Why do both? Adding wasmi to the pipeline shouldn't be that difficult, though I'd like some feedback on this aspect.

Formatting

  • Ran cargo fmt
  • Ran cargo check
  • Ran cargo build

Github Issue

This pull request closes #9

Author

Signed-off-by: George Cosma [email protected]

@george-cosma
Copy link
Collaborator Author

I've made this a draft PR since I still want to refactor some things. Though I'd still like to receive some feedback on the code as-is.

@wucke13
Copy link
Collaborator

wucke13 commented Jul 3, 2024

@george-cosma This is indeed a lot, maybe some slight over-engineering. Maybe this code could be simplified by using a macro_rules macro, which just expands to some code like the crufty example below.

Also I'd suggest that we might generate split test cases, one where we compare wasmtime and our, one where compare wasmi and our result.

let wat = r#"
    (module
        (import "host" "host_func" (func $host_hello (param i32)))

        (func (export "hello")
            i32.const 3
            call $host_hello)
    )
"#;

// wasmtime
let wasmtime_result = {
// Modules can be compiled through either the text or binary format
    let engine = Engine::default();
    let module = Module::new(&engine, wat)?;

    // Create a `Linker` which will be later used to instantiate this module.
    // Host functionality is defined by name within the `Linker`.
    let mut linker = Linker::new(&engine);
    linker.func_wrap("host", "host_func", |caller: Caller<'_, u32>, param: i32| {
        println!("Got {} from WebAssembly", param);
        println!("my host state is: {}", caller.data());
    })?;

    // All wasm objects operate within the context of a "store". Each
    // `Store` has a type parameter to store host-specific data, which in
    // this case we're using `4` for.
    let mut store = Store::new(&engine, 4);
    let instance = linker.instantiate(&mut store, &module)?;
    let hello = instance.get_typed_func::<(), ()>(&mut store, "hello")?;

    // And finally we can call the wasm!
    hello.call(&mut store, ())?
}

// wasmi
let wasmi_result = {
   // First step is to create the Wasm execution engine with some config.
    // In this example we are using the default configuration.
    let engine = Engine::default();
    // Wasmi does not yet support parsing `.wat` so we have to convert
    // out `.wat` into `.wasm` before we compile and validate it.
    let wasm = wat::parse_str(&wat)?;
    let module = Module::new(&engine, &mut &wasm[..])?;

    // All Wasm objects operate within the context of a `Store`.
    // Each `Store` has a type parameter to store host-specific data,
    // which in this case we are using `42` for.
    type HostState = u32;
    let mut store = Store::new(&engine, 42);
    let host_hello = Func::wrap(&mut store, |caller: Caller<'_, HostState>, param: i32| {
        println!("Got {param} from WebAssembly");
        println!("My host state is: {}", caller.data());
    });

    // In order to create Wasm module instances and link their imports
    // and exports we require a `Linker`.
    let mut linker = <Linker<HostState>>::new(&engine);
    // Instantiation of a Wasm module requires defining its imports and then
    // afterwards we can fetch exports by name, as well as asserting the
    // type signature of the function with `get_typed_func`.
    //
    // Also before using an instance created this way we need to start it.
    linker.define("host", "hello", host_hello)?;
    let instance = linker
        .instantiate(&mut store, &module)?
        .start(&mut store)?;
    let hello = instance.get_typed_func::<(), ()>(&store, "hello")?;

    // And finally we can call the wasm!
    hello.call(&mut store, ())?
}

// our wasm interpreter
let our_result = {
  // ..
}

assert_eq!(our_result, wasmtime_result);
assert_eq!(our_result, wasmi_result);

@george-cosma
Copy link
Collaborator Author

@wucke13 I think I understand you, but how would this macro call look? Something like this?

fn add_one() {
  let wat = r#"
  (module
      (func (export "add_one") (param $x i32) (result i32)
          local.get $x
          i32.const 1
          i32.add)
  )
  "#;

  // test_eq!(wat, func_name, input, expected_output)
  test_eq!(wat, "add_one", 1, 2);
  test_eq!(wat, "add_one", -10, -9);
  ...
}

This approach is fine, but it is missing any sort of versatility in terms of testing (eg. no shared state between asserts, only assert_eq). Other specialized could be made to address these shortcomings (test_neq!, some variations to also handle imports and store).

Or perhaps something like this to maintain state?

fn add_one() {
  let wat = r#"
    (module
        (memory 1)
        (func (export "store_num") (param $x i32)
            i32.const 0
            local.get $x
            i32.store)
        (func (export "load_num") (result i32)
            i32.const 0
            i32.load)
    )
    "#;

  test_init!(wat, wasmtime, wasmi, our) // maybe some optional stuff for store & imports here

  test_eq!("store_num", 42, (), wasmtime, wasmi, our) // the order of runtimes would be important
  test_eq!("load_num", (), 42, wasmtime, wasmi, our)
  test_neq!("load_num", (), 0, wasmtime, wasmi, our)
  ...
}

I quickly implemented a variant of the second variant over here. Should we go doin this path?

@wucke13
Copy link
Collaborator

wucke13 commented Jul 3, 2024

@george-cosma
Copy link
Collaborator Author

I will close this PR as it doesn't satisfy any of our needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement plymorphic property testing with an existing interpreter
2 participants