Merge #1783

1783: feat(c-api) Handle initialized but empty results in `wasm_func_call` r=Hywan a=Hywan # Description 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. This pattern can be found in the `global.c` example of the Wasm C API: https://github.com/wasmerio/wasmer/blob/591691bc17795e0f547a06bd8227ecc37f9aec39/lib/c-api/tests/wasm_c_api/wasm-c-api/example/global.c#L40-L47 Without this patch, this test fails. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Ivan Enderlin <[email protected]> Co-authored-by: jubianchi <[email protected]>
wasmerio · Nov 2, 2020 · 0ca87b8 · 0ca87b8
2 parents a5281ea + c2602ab
commit 0ca87b8
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 19 deletions.
diff --git 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
 

diff --git 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::<Result<Vec<Val>, _>>()
-                .expect("Argument conversion failed")
+                .expect("Arguments conversion failed")
         })
         .unwrap_or_default();
 
     match func.inner.call(&params) {
         Ok(wasm_results) => {
-            *results = wasm_results
+            let vals = wasm_results
                 .into_iter()
                 .map(TryInto::try_into)
                 .collect::<Result<Vec<wasm_val_t>, _>>()
-                .expect("Argument conversion failed")
-                .into();
+                .expect("Results conversion failed");
+
+            // `results` is an uninitialized vector. Set a new value.
+            if results.is_uninitialized() {
+                *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
         }

diff --git a/lib/c-api/src/wasm_c_api/macros.rs b/lib/c-api/src/wasm_c_api/macros.rs
@@ -56,12 +56,24 @@ macro_rules! wasm_declare_vec {
 
             impl [<wasm_ $name _vec_t>] {
                 pub unsafe fn into_slice(&self) -> Option<&[[<wasm_ $name _t>]]>{
-                    if self.data.is_null() {
+                    if self.is_uninitialized() {
                         return None;
                     }
 
                     Some(::std::slice::from_raw_parts(self.data, self.size))
                 }
+
+                pub unsafe fn into_slice_mut(&mut self) -> Option<&mut [[<wasm_ $name _t>]]>{
+                    if self.is_uninitialized() {
+                        return None;
+                    }
+
+                    Some(::std::slice::from_raw_parts_mut(self.data, self.size))
+                }
+
+                pub fn is_uninitialized(&self) -> bool {
+                    self.data.is_null()
+                }
             }
 
             // TODO: investigate possible memory leak on `init` (owned pointer)

diff --git a/lib/c-api/tests/wasm_c_api/CMakeLists.txt b/lib/c-api/tests/wasm_c_api/CMakeLists.txt
@@ -4,10 +4,10 @@ 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)
+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)
@@ -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})
@@ -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})