From 92e4cf64c1312e12da909a89a353ac0983c24948 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 13:29:28 +0100 Subject: [PATCH 1/8] feat(c-api) Implement `wasm_$name_vec_t::into_slice_mut`. --- lib/c-api/src/wasm_c_api/macros.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/c-api/src/wasm_c_api/macros.rs b/lib/c-api/src/wasm_c_api/macros.rs index afa81957de2..b7c8db87c90 100644 --- a/lib/c-api/src/wasm_c_api/macros.rs +++ b/lib/c-api/src/wasm_c_api/macros.rs @@ -64,6 +64,16 @@ macro_rules! wasm_declare_vec { } } + impl [] { + pub unsafe fn into_slice_mut(&self) -> Option<&mut [[]]>{ + if self.data.is_null() { + return None; + } + + Some(::std::slice::from_raw_parts_mut(self.data, self.size)) + } + } + // TODO: investigate possible memory leak on `init` (owned pointer) #[no_mangle] pub unsafe extern "C" fn [](out: *mut [], length: usize, init: *mut []) { From 758bc9bb78772102ca6ec717cf9ada56f189d8bb Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 13:30:14 +0100 Subject: [PATCH 2/8] feat(c-api) Handle initialized but empty results in `wasm_func_call`. Our implementation of `wasm_func_call` was correct for C code as follows: ```c wasm_val_vec_t arguments = WASM_EMPTY_VEC; wasm_val_vec_t results = WASM_EMPTY_VEC; wasm_func_call(func, &arguments, &results); ``` However, for a C code such as: ```c wasm_val_t vals[1]; wasm_val_vec_t arguments = WASM_EMPTY_VEC; wasm_val_vec_t results = WASM_ARRAY_VEC(vals); wasm_func_call(func, &arguments, &results); ``` the `vals` array were kept empty/unchanged. Why? Because `wasm_func_call` was replacing the value of `results` by a new `wasm_val_vec_t`. It is correct when `results` is an empty vector, but it is incorrect when `results` is initialized with empty values. This patch tries to detect this pattern: If `results.data` is `null`, it means the vector is empty/uninitialized, and we can set a new `wasm_val_vec_t`, otherwise it means the vector is initialized with empty values, and we need to update each item individually. --- .../src/wasm_c_api/externals/function.rs | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/externals/function.rs b/lib/c-api/src/wasm_c_api/externals/function.rs index c7b2a3c666c..1bffbda29de 100644 --- a/lib/c-api/src/wasm_c_api/externals/function.rs +++ b/lib/c-api/src/wasm_c_api/externals/function.rs @@ -154,18 +154,34 @@ pub unsafe extern "C" fn wasm_func_call( .into_iter() .map(TryInto::try_into) .collect::, _>>() - .expect("Argument conversion failed") + .expect("Arguments conversion failed") }) .unwrap_or_default(); match func.inner.call(¶ms) { Ok(wasm_results) => { - *results = wasm_results + let vals = wasm_results .into_iter() .map(TryInto::try_into) .collect::, _>>() - .expect("Argument conversion failed") - .into(); + .expect("Results conversion failed"); + + // `results` is an uninitialized vector. Set a new value. + if results.size == 0 || results.data.is_null() { + *results = vals.into(); + } + // `results` is an initialized but empty vector. Fill it + // item per item. + else { + let slice = results + .into_slice_mut() + .expect("`wasm_func_call`, results' size is greater than 0 but data is NULL"); + + for (result, value) in slice.iter_mut().zip(vals.iter()) { + (*result).kind = value.kind; + (*result).of = value.of; + } + } None } From b1e875903ac6cda3886052c11e6a29db1122963a Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 13:40:52 +0100 Subject: [PATCH 3/8] test(c-api) Enable the global test example. --- lib/c-api/tests/wasm_c_api/CMakeLists.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/c-api/tests/wasm_c_api/CMakeLists.txt b/lib/c-api/tests/wasm_c_api/CMakeLists.txt index 1ac9b3eaf53..2ce6014bf9a 100644 --- a/lib/c-api/tests/wasm_c_api/CMakeLists.txt +++ b/lib/c-api/tests/wasm_c_api/CMakeLists.txt @@ -4,7 +4,7 @@ project (WasmerWasmCApiTests) # Examples as tests from the `wasm-c-api` repository. add_executable(wasm-c-api-callback wasm-c-api/example/callback.c) #add_executable(wasm-c-api-finalize wasm-c-api/example/finalize.c) -#add_executable(wasm-c-api-global wasm-c-api/example/global.c) +add_executable(wasm-c-api-global wasm-c-api/example/global.c) add_executable(wasm-c-api-hello wasm-c-api/example/hello.c) #add_executable(wasm-c-api-hostref wasm-c-api/example/hostref.c) #add_executable(wasm-c-api-memory wasm-c-api/example/memory.c) @@ -57,12 +57,12 @@ add_test(NAME wasm-c-api-callback # WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/ #) -#target_link_libraries(wasm-c-api-global general ${WASMER_LIB}) -#target_compile_options(wasm-c-api-global PRIVATE ${COMPILER_OPTIONS}) -#add_test(NAME wasm-c-api-global -# COMMAND wasm-c-api-global -# WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/ -#) +target_link_libraries(wasm-c-api-global general ${WASMER_LIB}) +target_compile_options(wasm-c-api-global PRIVATE ${COMPILER_OPTIONS}) +add_test(NAME wasm-c-api-global + COMMAND wasm-c-api-global + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/ +) target_link_libraries(wasm-c-api-hello general ${WASMER_LIB}) target_compile_options(wasm-c-api-hello PRIVATE ${COMPILER_OPTIONS}) From 525fa5472f57abc3de37411b66fec265e795892e Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 13:44:24 +0100 Subject: [PATCH 4/8] doc(changelog) Add #1783. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 746759a843e..e8218b703d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## **[Unreleased]** - [#1710](https://github.com/wasmerio/wasmer/pull/1710) Memory for function call trampolines is now owned by the Artifact. +- [#1783](https://github.com/wasmerio/wasmer/pull/1783) Handle initialized but empty results in `wasm_func_call` in the Wasm C API. ### Added From ae9ee50baf6519274627669390cac3a53347a3b2 Mon Sep 17 00:00:00 2001 From: jubianchi Date: Fri, 30 Oct 2020 14:07:41 +0100 Subject: [PATCH 5/8] test(c-api) Enable the memory test example. --- lib/c-api/tests/wasm_c_api/CMakeLists.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/c-api/tests/wasm_c_api/CMakeLists.txt b/lib/c-api/tests/wasm_c_api/CMakeLists.txt index b66cead6869..98c39b49f31 100644 --- a/lib/c-api/tests/wasm_c_api/CMakeLists.txt +++ b/lib/c-api/tests/wasm_c_api/CMakeLists.txt @@ -7,7 +7,7 @@ add_executable(wasm-c-api-callback wasm-c-api/example/callback.c) add_executable(wasm-c-api-global wasm-c-api/example/global.c) add_executable(wasm-c-api-hello wasm-c-api/example/hello.c) #add_executable(wasm-c-api-hostref wasm-c-api/example/hostref.c) -#add_executable(wasm-c-api-memory wasm-c-api/example/memory.c) +add_executable(wasm-c-api-memory wasm-c-api/example/memory.c) #add_executable(wasm-c-api-multi wasm-c-api/example/multi.c) add_executable(wasm-c-api-reflect wasm-c-api/example/reflect.c) add_executable(wasm-c-api-serialize wasm-c-api/example/serialize.c) @@ -78,12 +78,12 @@ add_test(NAME wasm-c-api-hello # WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/ #) -#target_link_libraries(wasm-c-api-memory general ${WASMER_LIB}) -#target_compile_options(wasm-c-api-memory PRIVATE ${COMPILER_OPTIONS}) -#add_test(NAME wasm-c-api-memory -# COMMAND wasm-c-api-memory -# WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/ -#) +target_link_libraries(wasm-c-api-memory general ${WASMER_LIB}) +target_compile_options(wasm-c-api-memory PRIVATE ${COMPILER_OPTIONS}) +add_test(NAME wasm-c-api-memory + COMMAND wasm-c-api-memory + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/ +) #target_link_libraries(wasm-c-api-multi general ${WASMER_LIB}) #target_compile_options(wasm-c-api-multi PRIVATE ${COMPILER_OPTIONS}) From 77be1040f69b75b1d7c320ff7fd4cc4b69341901 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 14:11:09 +0100 Subject: [PATCH 6/8] feat(c-api) Implement `wasm_$name_vec_t::is_uninitialized()`. --- lib/c-api/src/wasm_c_api/macros.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/macros.rs b/lib/c-api/src/wasm_c_api/macros.rs index b7c8db87c90..bbefe848569 100644 --- a/lib/c-api/src/wasm_c_api/macros.rs +++ b/lib/c-api/src/wasm_c_api/macros.rs @@ -56,22 +56,24 @@ macro_rules! wasm_declare_vec { impl [] { pub unsafe fn into_slice(&self) -> Option<&[[]]>{ - if self.data.is_null() { + if self.is_unitialized() { return None; } Some(::std::slice::from_raw_parts(self.data, self.size)) } - } - impl [] { pub unsafe fn into_slice_mut(&self) -> Option<&mut [[]]>{ - if self.data.is_null() { + if self.is_unitialized() { return None; } Some(::std::slice::from_raw_parts_mut(self.data, self.size)) } + + pub fn is_unitialized(&self) -> bool { + self.data.is_null() + } } // TODO: investigate possible memory leak on `init` (owned pointer) From 477dcd576cb34ddc167f4ea63dfa1308a6997d5f Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 14:11:59 +0100 Subject: [PATCH 7/8] chore(c-api) Use `wasm_$name_vec_t::is_uninitialized()`. --- lib/c-api/src/wasm_c_api/externals/function.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/c-api/src/wasm_c_api/externals/function.rs b/lib/c-api/src/wasm_c_api/externals/function.rs index 1bffbda29de..cf7ff59c721 100644 --- a/lib/c-api/src/wasm_c_api/externals/function.rs +++ b/lib/c-api/src/wasm_c_api/externals/function.rs @@ -167,7 +167,7 @@ pub unsafe extern "C" fn wasm_func_call( .expect("Results conversion failed"); // `results` is an uninitialized vector. Set a new value. - if results.size == 0 || results.data.is_null() { + if results.is_uninitialized() { *results = vals.into(); } // `results` is an initialized but empty vector. Fill it From d2a5b8e1010a6e4093e3f4aa936d748c31f5ec56 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 30 Oct 2020 14:11:59 +0100 Subject: [PATCH 8/8] chore(c-api) Use `wasm_$name_vec_t::is_uninitialized()`. --- lib/c-api/src/wasm_c_api/externals/function.rs | 2 +- lib/c-api/src/wasm_c_api/macros.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/externals/function.rs b/lib/c-api/src/wasm_c_api/externals/function.rs index 1bffbda29de..cf7ff59c721 100644 --- a/lib/c-api/src/wasm_c_api/externals/function.rs +++ b/lib/c-api/src/wasm_c_api/externals/function.rs @@ -167,7 +167,7 @@ pub unsafe extern "C" fn wasm_func_call( .expect("Results conversion failed"); // `results` is an uninitialized vector. Set a new value. - if results.size == 0 || results.data.is_null() { + if results.is_uninitialized() { *results = vals.into(); } // `results` is an initialized but empty vector. Fill it diff --git a/lib/c-api/src/wasm_c_api/macros.rs b/lib/c-api/src/wasm_c_api/macros.rs index bbefe848569..77b196ec564 100644 --- a/lib/c-api/src/wasm_c_api/macros.rs +++ b/lib/c-api/src/wasm_c_api/macros.rs @@ -56,22 +56,22 @@ macro_rules! wasm_declare_vec { impl [] { pub unsafe fn into_slice(&self) -> Option<&[[]]>{ - if self.is_unitialized() { + if self.is_uninitialized() { return None; } Some(::std::slice::from_raw_parts(self.data, self.size)) } - pub unsafe fn into_slice_mut(&self) -> Option<&mut [[]]>{ - if self.is_unitialized() { + pub unsafe fn into_slice_mut(&mut self) -> Option<&mut [[]]>{ + if self.is_uninitialized() { return None; } Some(::std::slice::from_raw_parts_mut(self.data, self.size)) } - pub fn is_unitialized(&self) -> bool { + pub fn is_uninitialized(&self) -> bool { self.data.is_null() } }