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

Workaround for floating point arguments and return values in DynamicFuncs. #1283

Merged
merged 17 commits into from
Mar 12, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/clif-backend/src/code.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ impl ModuleCodeGenerator<CraneliftFunctionCodeGenerator, Caller, CodegenError>
Ok(())
}

fn feed_import_function(&mut self) -> Result<(), CodegenError> {
fn feed_import_function(&mut self, _sigindex: SigIndex) -> Result<(), CodegenError> {
Ok(())
}

Expand Down
2 changes: 1 addition & 1 deletion lib/llvm-backend/src/code.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8984,7 +8984,7 @@ impl<'ctx> ModuleCodeGenerator<LLVMFunctionCodeGenerator<'ctx>, LLVMBackend, Cod
Ok(())
}

fn feed_import_function(&mut self) -> Result<(), CodegenError> {
fn feed_import_function(&mut self, _sigindex: SigIndex) -> Result<(), CodegenError> {
self.func_import_count += 1;
Ok(())
}
Expand Down
197 changes: 162 additions & 35 deletions lib/runtime-core-tests/tests/imports.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::sync::Arc;
use std::{convert::TryInto, sync::Arc};
use wasmer_runtime_core::{
compile_with,
error::RuntimeError,
Expand All @@ -12,10 +12,11 @@ use wasmer_runtime_core::{
use wasmer_runtime_core_tests::{get_compiler, wat2wasm};

macro_rules! call_and_assert {
($instance:ident, $function:ident, $expected_value:expr) => {
let $function: Func<i32, i32> = $instance.func(stringify!($function)).unwrap();
($instance:ident, $function:ident( $( $inputs:ty ),* ) -> $output:ty, ( $( $arguments:expr ),* ) == $expected_value:expr) => {
#[allow(unused_parens)]
let $function: Func<( $( $inputs ),* ), $output> = $instance.func(stringify!($function)).expect(concat!("Failed to get the `", stringify!($function), "` export function."));

let result = $function.call(1);
let result = $function.call( $( $arguments ),* );

match (result, $expected_value) {
(Ok(value), expected_value) => assert_eq!(
Expand Down Expand Up @@ -75,7 +76,12 @@ fn imported_functions_forms(test: &dyn Fn(&Instance)) {
(import "env" "memory" (memory 1 1))
(import "env" "callback_fn" (func $callback_fn (type $type)))
(import "env" "callback_closure" (func $callback_closure (type $type)))
(import "env" "callback_closure_dynamic" (func $callback_closure_dynamic (type $type)))
(import "env" "callback_fn_dynamic" (func $callback_fn_dynamic (type $type)))
(import "env" "callback_closure_dynamic_0" (func $callback_closure_dynamic_0))
(import "env" "callback_closure_dynamic_1" (func $callback_closure_dynamic_1 (param i32) (result i32)))
(import "env" "callback_closure_dynamic_2" (func $callback_closure_dynamic_2 (param i32 i64) (result i64)))
(import "env" "callback_closure_dynamic_3" (func $callback_closure_dynamic_3 (param i32 i64 f32) (result f32)))
(import "env" "callback_closure_dynamic_4" (func $callback_closure_dynamic_4 (param i32 i64 f32 f64) (result f64)))
(import "env" "callback_closure_with_env" (func $callback_closure_with_env (type $type)))
(import "env" "callback_fn_with_vmctx" (func $callback_fn_with_vmctx (type $type)))
(import "env" "callback_closure_with_vmctx" (func $callback_closure_with_vmctx (type $type)))
Expand All @@ -94,9 +100,34 @@ fn imported_functions_forms(test: &dyn Fn(&Instance)) {
get_local 0
call $callback_closure)

(func (export "function_closure_dynamic") (type $type)
(func (export "function_fn_dynamic") (type $type)
get_local 0
call $callback_closure_dynamic)
call $callback_fn_dynamic)

(func (export "function_closure_dynamic_0")
call $callback_closure_dynamic_0)

(func (export "function_closure_dynamic_1") (param i32) (result i32)
get_local 0
call $callback_closure_dynamic_1)

(func (export "function_closure_dynamic_2") (param i32 i64) (result i64)
get_local 0
get_local 1
call $callback_closure_dynamic_2)

(func (export "function_closure_dynamic_3") (param i32 i64 f32) (result f32)
get_local 0
get_local 1
get_local 2
call $callback_closure_dynamic_3)

(func (export "function_closure_dynamic_4") (param i32 i64 f32 f64) (result f64)
get_local 0
get_local 1
get_local 2
get_local 3
call $callback_closure_dynamic_4)

(func (export "function_closure_with_env") (type $type)
get_local 0
Expand Down Expand Up @@ -154,13 +185,77 @@ fn imported_functions_forms(test: &dyn Fn(&Instance)) {
Ok(n + 1)
}),

"callback_closure_dynamic" => DynamicFunc::new(
// Regular polymorphic function.
"callback_fn_dynamic" => DynamicFunc::new(
Arc::new(FuncSig::new(vec![Type::I32], vec![Type::I32])),
|_, params| -> Vec<Value> {
match params[0] {
Value::I32(x) => vec![Value::I32(x + 1)],
_ => unreachable!()
}
callback_fn_dynamic,
),

// Polymorphic closure “closures”.
"callback_closure_dynamic_0" => DynamicFunc::new(
Arc::new(FuncSig::new(vec![], vec![])),
|_, inputs: &[Value]| -> Vec<Value> {
assert!(inputs.is_empty());

vec![]
}
),
"callback_closure_dynamic_1" => DynamicFunc::new(
Arc::new(FuncSig::new(vec![Type::I32], vec![Type::I32])),
move |vmctx: &mut vm::Ctx, inputs: &[Value]| -> Vec<Value> {
assert_eq!(inputs.len(), 1);

let memory = vmctx.memory(0);
let shift_ = shift + memory.view::<i32>()[0].get();
let n: i32 = (&inputs[0]).try_into().unwrap();

vec![Value::I32(shift_ + n)]
}
),
"callback_closure_dynamic_2" => DynamicFunc::new(
Arc::new(FuncSig::new(vec![Type::I32, Type::I64], vec![Type::I64])),
move |vmctx: &mut vm::Ctx, inputs: &[Value]| -> Vec<Value> {
assert_eq!(inputs.len(), 2);

let memory = vmctx.memory(0);
let shift_ = shift + memory.view::<i32>()[0].get();
let i: i32 = (&inputs[0]).try_into().unwrap();
let j: i64 = (&inputs[1]).try_into().unwrap();

vec![Value::I64(shift_ as i64 + i as i64 + j)]
}
),
"callback_closure_dynamic_3" => DynamicFunc::new(
Arc::new(FuncSig::new(vec![Type::I32, Type::I64, Type::F32], vec![Type::F32])),
move |vmctx: &mut vm::Ctx, inputs: &[Value]| -> Vec<Value> {
assert_eq!(inputs.len(), 3);

let memory = vmctx.memory(0);
let shift_ = shift + memory.view::<i32>()[0].get();
let i: i32 = (&inputs[0]).try_into().unwrap();
let j: i64 = (&inputs[1]).try_into().unwrap();
let k: f32 = (&inputs[2]).try_into().unwrap();

dbg!(i);
dbg!(j);
dbg!(k);
losfair marked this conversation as resolved.
Show resolved Hide resolved

vec![Value::F32(shift_ as f32 + i as f32 + j as f32 + k)]
}
),
"callback_closure_dynamic_4" => DynamicFunc::new(
Arc::new(FuncSig::new(vec![Type::I32, Type::I64, Type::F32, Type::F64], vec![Type::F64])),
move |vmctx: &mut vm::Ctx, inputs: &[Value]| -> Vec<Value> {
assert_eq!(inputs.len(), 4);

let memory = vmctx.memory(0);
let shift_ = shift + memory.view::<i32>()[0].get();
let i: i32 = (&inputs[0]).try_into().unwrap();
let j: i64 = (&inputs[1]).try_into().unwrap();
let k: f32 = (&inputs[2]).try_into().unwrap();
let l: f64 = (&inputs[3]).try_into().unwrap();

vec![Value::F64(shift_ as f64 + i as f64 + j as f64 + k as f64 + l)]
}
),

Expand Down Expand Up @@ -227,6 +322,13 @@ fn callback_fn(n: i32) -> Result<i32, ()> {
Ok(n + 1)
}

fn callback_fn_dynamic(_: &mut vm::Ctx, inputs: &[Value]) -> Vec<Value> {
match inputs[0] {
Value::I32(x) => vec![Value::I32(x + 1)],
_ => unreachable!(),
}
}

fn callback_fn_with_vmctx(vmctx: &mut vm::Ctx, n: i32) -> Result<i32, ()> {
let memory = vmctx.memory(0);
let shift_: i32 = memory.view()[0].get();
Expand All @@ -246,57 +348,82 @@ fn callback_fn_trap_with_vmctx(vmctx: &mut vm::Ctx, n: i32) -> Result<i32, Strin
}

macro_rules! test {
($test_name:ident, $function:ident, $expected_value:expr) => {
($test_name:ident, $function:ident( $( $inputs:ty ),* ) -> $output:ty, ( $( $arguments:expr ),* ) == $expected_value:expr) => {
#[test]
fn $test_name() {
imported_functions_forms(&|instance| {
call_and_assert!(instance, $function, $expected_value);
call_and_assert!(instance, $function( $( $inputs ),* ) -> $output, ( $( $arguments ),* ) == $expected_value);
});
}
};
}

test!(test_fn, function_fn, Ok(2));
test!(test_closure, function_closure, Ok(2));
test!(test_closure_dynamic, function_closure_dynamic, Ok(2));
test!(test_fn, function_fn(i32) -> i32, (1) == Ok(2));
test!(test_closure, function_closure(i32) -> i32, (1) == Ok(2));
test!(test_fn_dynamic, function_fn_dynamic(i32) -> i32, (1) == Ok(2));
test!(
test_closure_dynamic_0,
function_closure_dynamic_0(()) -> (),
() == Ok(())
);
test!(
test_closure_dynamic_1,
function_closure_dynamic_1(i32) -> i32,
(1) == Ok(1 + shift + SHIFT)
);
test!(
test_closure_dynamic_2,
function_closure_dynamic_2(i32, i64) -> i64,
(1, 2) == Ok(1 + 2 + shift as i64 + SHIFT as i64)
);
test!(
test_closure_dynamic_3,
function_closure_dynamic_3(i32, i64, f32) -> f32,
(1, 2, 3.) == Ok(1. + 2. + 3. + shift as f32 + SHIFT as f32)
);
test!(
test_closure_dynamic_4,
function_closure_dynamic_4(i32, i64, f32, f64) -> f64,
(1, 2, 3., 4.) == Ok(1. + 2. + 3. + 4. + shift as f64 + SHIFT as f64)
);
test!(
test_closure_with_env,
function_closure_with_env,
Ok(2 + shift + SHIFT)
function_closure_with_env(i32) -> i32,
(1) == Ok(2 + shift + SHIFT)
);
test!(test_fn_with_vmctx, function_fn_with_vmctx, Ok(2 + SHIFT));
test!(test_fn_with_vmctx, function_fn_with_vmctx(i32) -> i32, (1) == Ok(2 + SHIFT));
test!(
test_closure_with_vmctx,
function_closure_with_vmctx,
Ok(2 + SHIFT)
function_closure_with_vmctx(i32) -> i32,
(1) == Ok(2 + SHIFT)
);
test!(
test_closure_with_vmctx_and_env,
function_closure_with_vmctx_and_env,
Ok(2 + shift + SHIFT)
function_closure_with_vmctx_and_env(i32) -> i32,
(1) == Ok(2 + shift + SHIFT)
);
test!(
test_fn_trap,
function_fn_trap,
Err(RuntimeError(Box::new(format!("foo {}", 2))))
function_fn_trap(i32) -> i32,
(1) == Err(RuntimeError(Box::new(format!("foo {}", 2))))
);
test!(
test_closure_trap,
function_closure_trap,
Err(RuntimeError(Box::new(format!("bar {}", 2))))
function_closure_trap(i32) -> i32,
(1) == Err(RuntimeError(Box::new(format!("bar {}", 2))))
);
test!(
test_fn_trap_with_vmctx,
function_fn_trap_with_vmctx,
Err(RuntimeError(Box::new(format!("baz {}", 2 + SHIFT))))
function_fn_trap_with_vmctx(i32) -> i32,
(1) == Err(RuntimeError(Box::new(format!("baz {}", 2 + SHIFT))))
);
test!(
test_closure_trap_with_vmctx,
function_closure_trap_with_vmctx,
Err(RuntimeError(Box::new(format!("qux {}", 2 + SHIFT))))
function_closure_trap_with_vmctx(i32) -> i32,
(1) == Err(RuntimeError(Box::new(format!("qux {}", 2 + SHIFT))))
);
test!(
test_closure_trap_with_vmctx_and_env,
function_closure_trap_with_vmctx_and_env,
Err(RuntimeError(Box::new(format!("! {}", 2 + shift + SHIFT))))
function_closure_trap_with_vmctx_and_env(i32) -> i32,
(1) == Err(RuntimeError(Box::new(format!("! {}", 2 + shift + SHIFT))))
);
2 changes: 1 addition & 1 deletion lib/runtime-core/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ pub trait ModuleCodeGenerator<FCG: FunctionCodeGenerator<E>, RM: RunnableModule,
Ok(())
}
/// Adds an import function.
fn feed_import_function(&mut self) -> Result<(), E>;
fn feed_import_function(&mut self, _sigindex: SigIndex) -> Result<(), E>;
/// Sets the signatures.
fn feed_signatures(&mut self, signatures: Map<SigIndex, FuncSig>) -> Result<(), E>;
/// Sets function signatures.
Expand Down
65 changes: 36 additions & 29 deletions lib/runtime-core/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,46 @@ pub fn read_module<
let mut namespace_builder = Some(StringTableBuilder::new());
let mut name_builder = Some(StringTableBuilder::new());
let mut func_count: usize = 0;
let mut mcg_signatures_fed = false;
let mut mcg_info_fed = false;

loop {
use wasmparser::ParserState;
let state = parser.read();

// Feed signature and namespace information as early as possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this change? These would have the same behaviour if they were cases in the match *state that follows, right? If this is about code reuse, maybe create a closure?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes sense to use the Wasmparser types that let us be more specific about what exactly we want: https://docs.rs/wasmparser/0.51.4/wasmparser/struct.CodeSectionReader.html . They have a bunch of section-specific parsers, there aren't validating versions of these parsers though, so we'd have to do a validation pass first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this block borrows mcg, there would be lifetime issues if this is made into a closure. And yeah this is about code reuse. The same code was already duplicated twice for BeginFunctionBody and EndWasm, and this change will add one more duplication if done "in place".

Copy link
Contributor

Choose a reason for hiding this comment

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

mcg could be passed as an argument here to make closures work

match *state {
ParserState::BeginFunctionBody { .. }
| ParserState::ImportSectionEntry { .. }
| ParserState::EndWasm => {
if !mcg_signatures_fed {
mcg_signatures_fed = true;
let info_read = info.read().unwrap();
mcg.feed_signatures(info_read.signatures.clone())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
}
}
_ => {}
}
match *state {
ParserState::BeginFunctionBody { .. } | ParserState::EndWasm => {
if !mcg_info_fed {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use separate match statements, moving the if !mcg_*_fed { checks outside the match *state will probably have better performance due to better branch prediction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a kind of simple optimization that can be left to the compiler?

Copy link
Contributor

@nlewycky nlewycky Mar 11, 2020

Choose a reason for hiding this comment

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

Depends whether the compiler can prove *state is side-effect free. I'm sorta torn on this one, I think we should write whatever is clearest to the reader, but I'm not sure that pulling this part of the match out in advance is actually clearer. I'm happy to let you and Mark decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make it into a closure.

mcg_info_fed = true;
{
let mut info_write = info.write().unwrap();
info_write.namespace_table = namespace_builder.take().unwrap().finish();
info_write.name_table = name_builder.take().unwrap().finish();
}
let info_read = info.read().unwrap();
mcg.feed_function_signatures(info_read.func_assoc.clone())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
mcg.check_precondition(&info_read)
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
}
}
_ => {}
}

match *state {
ParserState::Error(ref err) => return Err(err.clone().into()),
ParserState::TypeSectionEntry(ref ty) => {
Expand All @@ -136,7 +171,7 @@ pub fn read_module<
let sigindex = SigIndex::new(sigindex as usize);
info.write().unwrap().imported_functions.push(import_name);
info.write().unwrap().func_assoc.push(sigindex);
mcg.feed_import_function()
mcg.feed_import_function(sigindex)
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
}
ImportSectionEntryType::Table(table_ty) => {
Expand Down Expand Up @@ -218,22 +253,6 @@ pub fn read_module<
}
ParserState::BeginFunctionBody { range } => {
let id = func_count;
if !mcg_info_fed {
mcg_info_fed = true;
{
let mut info_write = info.write().unwrap();
info_write.namespace_table = namespace_builder.take().unwrap().finish();
info_write.name_table = name_builder.take().unwrap().finish();
}
let info_read = info.read().unwrap();
mcg.feed_signatures(info_read.signatures.clone())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
mcg.feed_function_signatures(info_read.func_assoc.clone())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
mcg.check_precondition(&info_read)
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
}

let fcg = mcg
.next_function(
Arc::clone(&info),
Expand Down Expand Up @@ -432,18 +451,6 @@ pub fn read_module<
info.write().unwrap().globals.push(global_init);
}
ParserState::EndWasm => {
// TODO Consolidate with BeginFunction body if possible
if !mcg_info_fed {
info.write().unwrap().namespace_table =
namespace_builder.take().unwrap().finish();
info.write().unwrap().name_table = name_builder.take().unwrap().finish();
mcg.feed_signatures(info.read().unwrap().signatures.clone())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
mcg.feed_function_signatures(info.read().unwrap().func_assoc.clone())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
mcg.check_precondition(&info.read().unwrap())
.map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
}
break;
}
_ => {}
Expand Down
Loading