Merge wasmerio#2208

2208: Implement to/from casting for `extern` w/ no allocation r=MarkMcCaskey a=MarkMcCaskey Resolves wasmerio#2197 This should be good to go now. The only question is how we want to deal with a breaking change this big. @Hywan and I previously discussed this, it seems other implementers of the wasm C API may have the same bug this is fixing [New Changelog rendered](https://github.com/wasmerio/wasmer/blob/fix/c-api-allocation/lib/c-api/CHANGELOG.md) [Primary Changelog rendered](https://github.com/wasmerio/wasmer/blob/fix/c-api-allocation/CHANGELOG.md) Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
Earthmark · Apr 6, 2021 · 640d9bb · 640d9bb
2 parents eff77c4 + 269df82
commit 640d9bb
Show file tree

Hide file tree

Showing 18 changed files with 289 additions and 130 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,10 +4,13 @@
 
 [Keep a Changelog]: http://keepachangelog.com/en/1.0.0/
 
+Looking for changes that affect our C API? See the [C API Changelog](lib/c-api/CHANGELOG.md).
+
 
 ## **[Unreleased]**
 
 ### Added
+- [#2208](https://github.com/wasmerio/wasmer/pull/2208) Add a new CHANGELOG.md specific to our C API to make it easier for users primarily consuming our C API to keep up to date with changes that affect them.
 - [#2103](https://github.com/wasmerio/wasmer/pull/2103) Add middleware (incl. metering) in the C API.
 - [#2003](https://github.com/wasmerio/wasmer/pull/2003) Wasmer works with musl, and is built, tested and packaged for musl.
 - [#2116](https://github.com/wasmerio/wasmer/pull/2116) Add a package for Windows that is not an installer, but all the `lib` and `include` files as for macOS and Linux.
@@ -27,6 +30,7 @@
 - [#2149](https://github.com/wasmerio/wasmer/pull/2144) `wasmer-engine-native` looks for clang-11 instead of clang-10.
 
 ### Fixed
+- [#2208](https://github.com/wasmerio/wasmer/pull/2208) Fix ownership in Wasm C API of `wasm_extern_as_func`, `wasm_extern_as_memory`, `wasm_extern_as_table`, `wasm_extern_as_global`, `wasm_func_as_extern`, `wasm_memory_as_extern`, `wasm_table_as_extern`, and `wasm_global_as_extern`. These functions no longer allocate memory and thus their results should not be freed. This is a breaking change to align more closely with the Wasm C API's stated ownership.
 - [#2210](https://github.com/wasmerio/wasmer/pull/2210) Fix a memory leak in the Wasm C API in the strings used to identify imports and exports coming from user code.
 - [#2108](https://github.com/wasmerio/wasmer/pull/2108) The Object Native Engine generates code that now compiles correctly with C++.
 - [#2125](https://github.com/wasmerio/wasmer/pull/2125) Fix RUSTSEC-2021-0023.

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/lib/c-api/CHANGELOG.md b/lib/c-api/CHANGELOG.md
@@ -0,0 +1,22 @@
+# C API Changelog
+
+*The format is based on [Keep a Changelog].*
+
+[Keep a Changelog]: http://keepachangelog.com/en/1.0.0/
+
+Looking for changes to the Wasmer CLI and the Rust API? See our [Primary Changelog](../../CHANGELOG.md)
+
+
+## **[Unreleased]**
+
+### Added
+- [#2208](https://github.com/wasmerio/wasmer/pull/2208) Add a new CHANGELOG.md specific to our C API to make it easier for users primarily consuming our C API to keep up to date with changes that affect them.
+
+### Changed
+
+### Fixed
+- [#2208](https://github.com/wasmerio/wasmer/pull/2208) Fix ownership in Wasm C API of `wasm_extern_as_func`, `wasm_extern_as_memory`, `wasm_extern_as_table`, `wasm_extern_as_global`, `wasm_func_as_extern`, `wasm_memory_as_extern`, `wasm_table_as_extern`, and `wasm_global_as_extern`. These functions no longer allocate memory and thus their results should not be freed. This is a breaking change to align more closely with the Wasm C API's stated ownership.
+
+## Changes before 2020-04-06
+
+See the [Primary Changelog](../../CHANGELOG.md).
diff --git a/lib/c-api/Cargo.toml b/lib/c-api/Cargo.toml
@@ -38,6 +38,7 @@ typetag = { version = "0.1", optional = true }
 paste = "1.0"
 
 [dev-dependencies]
+field-offset = "0.3.3"
 inline-c = "0.1.5"
 
 [features]

diff --git a/lib/c-api/examples/exports-function.c b/lib/c-api/examples/exports-function.c
@@ -77,7 +77,6 @@ int main(int argc, const char* argv[]) {
 
     printf("Results of `sum`: %d\n", results_val[0].of.i32);
 
-    wasm_func_delete(sum_func);
     wasm_module_delete(module);
     wasm_instance_delete(instance);
     wasm_extern_vec_delete(&exports);

diff --git a/lib/c-api/examples/exports-global.c b/lib/c-api/examples/exports-global.c
@@ -109,8 +109,6 @@ int main(int argc, const char* argv[]) {
     wasm_global_set(some, &some_set_value);
     printf("`some` value: %.1f\n", some_value.of.f32);
 
-    wasm_global_delete(some);
-    wasm_global_delete(one);
     wasm_module_delete(module);
     wasm_extern_vec_delete(&exports);
     wasm_instance_delete(instance);

diff --git a/lib/c-api/examples/imports-exports.c b/lib/c-api/examples/imports-exports.c
@@ -124,10 +124,6 @@ int main(int argc, const char* argv[]) {
 
     printf("Got the exported memory: %p\n", memory);
 
-    wasm_func_delete(func);
-    wasm_global_delete(global);
-    wasm_table_delete(table);
-    wasm_memory_delete(memory);
     wasm_module_delete(module);
     wasm_extern_vec_delete(&exports);
     wasm_instance_delete(instance);

diff --git a/lib/c-api/examples/memory.c b/lib/c-api/examples/memory.c
@@ -99,10 +99,6 @@ int main(int argc, const char* argv[]) {
 
     printf("Value at 0x%04x: %d\n", mem_addr, get_at_results_val[0].of.i32);
 
-    wasm_memory_delete(memory);
-    wasm_func_delete(mem_size);
-    wasm_func_delete(set_at);
-    wasm_func_delete(get_at);
     wasm_extern_vec_delete(&exports);
     wasm_module_delete(module);
     wasm_instance_delete(instance);

diff --git a/lib/c-api/src/wasm_c_api/externals/function.rs b/lib/c-api/src/wasm_c_api/externals/function.rs
@@ -2,17 +2,27 @@ use super::super::store::wasm_store_t;
 use super::super::trap::wasm_trap_t;
 use super::super::types::{wasm_functype_t, wasm_valkind_enum};
 use super::super::value::{wasm_val_inner, wasm_val_t, wasm_val_vec_t};
+use super::CApiExternTag;
 use std::convert::TryInto;
 use std::ffi::c_void;
 use std::sync::Arc;
-use wasmer::{Function, Instance, RuntimeError, Val};
+use wasmer::{Function, RuntimeError, Val};
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 #[allow(non_camel_case_types)]
+#[repr(C)]
 pub struct wasm_func_t {
-    pub(crate) inner: Function,
-    // this is how we ensure the instance stays alive
-    pub(crate) instance: Option<Arc<Instance>>,
+    pub(crate) tag: CApiExternTag,
+    pub(crate) inner: Box<Function>,
+}
+
+impl wasm_func_t {
+    pub(crate) fn new(function: Function) -> Self {
+        Self {
+            tag: CApiExternTag::Function,
+            inner: Box::new(function),
+        }
+    }
 }
 
 #[allow(non_camel_case_types)]
@@ -80,10 +90,7 @@ pub unsafe extern "C" fn wasm_func_new(
     };
     let function = Function::new(&store.inner, func_sig, inner_callback);
 
-    Some(Box::new(wasm_func_t {
-        instance: None,
-        inner: function,
-    }))
+    Some(Box::new(wasm_func_t::new(function)))
 }
 
 #[no_mangle]
@@ -171,10 +178,7 @@ pub unsafe extern "C" fn wasm_func_new_with_env(
         trampoline,
     );
 
-    Some(Box::new(wasm_func_t {
-        instance: None,
-        inner: function,
-    }))
+    Some(Box::new(wasm_func_t::new(function)))
 }
 
 #[no_mangle]

diff --git a/lib/c-api/src/wasm_c_api/externals/global.rs b/lib/c-api/src/wasm_c_api/externals/global.rs
@@ -1,14 +1,26 @@
 use super::super::store::wasm_store_t;
 use super::super::types::wasm_globaltype_t;
 use super::super::value::wasm_val_t;
+use super::CApiExternTag;
 use crate::error::update_last_error;
 use std::convert::TryInto;
 use wasmer::{Global, Val};
 
 #[allow(non_camel_case_types)]
+#[repr(C)]
+#[derive(Clone, Debug)]
 pub struct wasm_global_t {
-    // maybe needs to hold onto instance
-    pub(crate) inner: Global,
+    pub(crate) tag: CApiExternTag,
+    pub(crate) inner: Box<Global>,
+}
+
+impl wasm_global_t {
+    pub(crate) fn new(global: Global) -> Self {
+        Self {
+            tag: CApiExternTag::Global,
+            inner: Box::new(global),
+        }
+    }
 }
 
 #[no_mangle]
@@ -30,7 +42,7 @@ pub unsafe extern "C" fn wasm_global_new(
         Global::new(store, wasm_val)
     };
 
-    Some(Box::new(wasm_global_t { inner: global }))
+    Some(Box::new(wasm_global_t::new(global)))
 }
 
 #[no_mangle]
@@ -40,9 +52,7 @@ pub unsafe extern "C" fn wasm_global_delete(_global: Option<Box<wasm_global_t>>)
 #[no_mangle]
 pub unsafe extern "C" fn wasm_global_copy(global: &wasm_global_t) -> Box<wasm_global_t> {
     // do shallow copy
-    Box::new(wasm_global_t {
-        inner: global.inner.clone(),
-    })
+    Box::new(wasm_global_t::new((&*global.inner).clone()))
 }
 
 #[no_mangle]

diff --git a/lib/c-api/src/wasm_c_api/externals/memory.rs b/lib/c-api/src/wasm_c_api/externals/memory.rs
@@ -1,12 +1,24 @@
 use super::super::store::wasm_store_t;
 use super::super::types::wasm_memorytype_t;
+use super::CApiExternTag;
 use std::mem;
 use wasmer::{Memory, Pages};
 
 #[allow(non_camel_case_types)]
+#[repr(C)]
+#[derive(Clone, Debug)]
 pub struct wasm_memory_t {
-    // maybe needs to hold onto instance
-    pub(crate) inner: Memory,
+    pub(crate) tag: CApiExternTag,
+    pub(crate) inner: Box<Memory>,
+}
+
+impl wasm_memory_t {
+    pub(crate) fn new(memory: Memory) -> Self {
+        Self {
+            tag: CApiExternTag::Memory,
+            inner: Box::new(memory),
+        }
+    }
 }
 
 #[no_mangle]
@@ -20,7 +32,7 @@ pub unsafe extern "C" fn wasm_memory_new(
     let memory_type = memory_type.inner().memory_type.clone();
     let memory = c_try!(Memory::new(&store.inner, memory_type));
 
-    Some(Box::new(wasm_memory_t { inner: memory }))
+    Some(Box::new(wasm_memory_t::new(memory)))
 }
 
 #[no_mangle]
@@ -30,9 +42,7 @@ pub unsafe extern "C" fn wasm_memory_delete(_memory: Option<Box<wasm_memory_t>>)
 #[no_mangle]
 pub unsafe extern "C" fn wasm_memory_copy(memory: &wasm_memory_t) -> Box<wasm_memory_t> {
     // do shallow copy
-    Box::new(wasm_memory_t {
-        inner: memory.inner.clone(),
-    })
+    Box::new(wasm_memory_t::new((&*memory.inner).clone()))
 }
 
 #[no_mangle]