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

test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs #1949

Merged
merged 18 commits into from
Dec 18, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a4effaa
test+doc(c-api) Continue to improve test coverage of the C API.
Hywan Dec 17, 2020
e10ad51
fix(c-api) Fix how `wasm_functype_t` is implemented.
Hywan Dec 17, 2020
75ceb0d
fix(c-api) Fix how `wasm_globaltype_t` is implemented.
Hywan Dec 17, 2020
a3c7a2d
fix(c-api) Fix how `wasm_memorytype_t` is implemented.
Hywan Dec 17, 2020
a0c34fe
fix(c-api) Fix how `wasm_tabletype_t` is implemented.
Hywan Dec 17, 2020
4abf6f8
fix(c-api) Fix how `wasm_frame_t` is implemented.
Hywan Dec 17, 2020
43026e6
feat(c-api) Implement `From<&[T]> for wasm_$name_vec_t` for boxed vec.
Hywan Dec 17, 2020
940dea7
feat(c-api) Simplify code by using new conversion implementations.
Hywan Dec 17, 2020
8aa0822
feat(c-api) `wasm_$name_vec_delete` checks the vec is initialized.
Hywan Dec 17, 2020
96169de
test+doc(c-api) Improve test coverage for the `macros` module, and im…
Hywan Dec 17, 2020
eb03f18
Merge branch 'master' into doc-c-api-2
Hywan Dec 17, 2020
2853bd4
doc(changelog) Add #1949.
Hywan Dec 17, 2020
272b9d1
Merge branch 'master' into doc-c-api-2
Hywan Dec 17, 2020
51fe219
feat(c-api) `wasm_$name_vec_delete` for boxed vec now takes an `Optio…
Hywan Dec 18, 2020
a68a1e6
feat(c-api) Transmute boxed vecs to `Vec<Option<Box<T>>>` when deleting.
Hywan Dec 18, 2020
40fa9c0
fix(c-api) UPdate `wasm_functype_new` according to previous commit.
Hywan Dec 18, 2020
2ca30fe
fix(c-api) Ensure that uninitialized boxed vec are zeroed.
Hywan Dec 18, 2020
b9afb6e
fix(c-api) Remove `Box` from `wasm_functype_new`.
Hywan Dec 18, 2020
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
177 changes: 166 additions & 11 deletions lib/c-api/src/wasm_c_api/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,31 @@
macro_rules! wasm_declare_vec_inner {
($name:ident) => {
paste::paste! {
/// Creates an empty vector of
#[doc = "Creates an empty vector of [`wasm_" $name "_t`]."]
#[doc = "Creates an empty vector of [`wasm_" $name "_t`].

# Example

```rust
# use inline_c::assert_c;
# fn main() {
# (assert_c! {
# #include \"tests/wasmer_wasm.h\"
#
int main() {
// Creates an empty vector of `wasm_" $name "_t`.
wasm_" $name "_vec_t vector;
wasm_" $name "_vec_new_empty(&vector);

// Check that it is empty.
assert(vector.size == 0);

// Free it.
wasm_" $name "_vec_delete(&vector);
}
# })
# .success();
# }
```"]
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_new_empty>](out: *mut [<wasm_ $name _vec_t>]) {
// TODO: actually implement this
Expand All @@ -19,7 +42,37 @@ macro_rules! wasm_declare_vec_inner {
macro_rules! wasm_declare_vec {
($name:ident) => {
paste::paste! {
#[doc = "Represents of a vector of [`wasm_" $name "_t`]."]
#[doc = "Represents a vector of `wasm_" $name "_t`.

Read the documentation of [`wasm_" $name "_t`] to see more concrete examples.

# Example

```rust
# use inline_c::assert_c;
# fn main() {
# (assert_c! {
# #include \"tests/wasmer_wasm.h\"
#
int main() {
// Create a vector of 2 `wasm_" $name "_t`.
wasm_" $name "_t x;
wasm_" $name "_t y;
wasm_" $name "_t* items[2] = {&x, &y};

wasm_" $name "_vec_t vector;
wasm_" $name "_vec_new(&vector, 2, (wasm_" $name "_t*) items);

// Check that it contains 2 items.
assert(vector.size == 2);

// Free it.
wasm_" $name "_vec_delete(&vector);
}
# })
# .success();
# }
```"]
#[derive(Debug)]
#[repr(C)]
pub struct [<wasm_ $name _vec_t>] {
Expand Down Expand Up @@ -54,6 +107,7 @@ macro_rules! wasm_declare_vec {
.into_boxed_slice();
let data = copied_data.as_mut_ptr();
::std::mem::forget(copied_data);

Self {
size,
data,
Expand Down Expand Up @@ -84,31 +138,68 @@ macro_rules! wasm_declare_vec {
}

// TODO: investigate possible memory leak on `init` (owned pointer)
#[doc = "Creates a new vector of [`wasm_" $name "_t`]."]
#[doc = "Creates a new vector of [`wasm_" $name "_t`].

# Example

See the [`wasm_" $name "_vec_t`] type to get an example."]
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_new>](out: *mut [<wasm_ $name _vec_t>], length: usize, init: *mut [<wasm_ $name _t>]) {
let mut bytes: Vec<[<wasm_ $name _t>]> = Vec::with_capacity(length);

for i in 0..length {
bytes.push(::std::ptr::read(init.add(i)));
}

let pointer = bytes.as_mut_ptr();
debug_assert!(bytes.len() == bytes.capacity());

(*out).data = pointer;
(*out).size = length;
::std::mem::forget(bytes);
}

#[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`]."]
#[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`].

# Example

```rust
# use inline_c::assert_c;
# fn main() {
# (assert_c! {
# #include \"tests/wasmer_wasm.h\"
#
int main() {
// Creates an empty vector of `wasm_" $name "_t`.
wasm_" $name "_vec_t vector;
wasm_" $name "_vec_new_uninitialized(&vector, 3);

// Check that it contains 3 items.
assert(vector.size == 3);

// Free it.
wasm_" $name "_vec_delete(&vector);
}
# })
# .success();
# }
```"]
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_new_uninitialized>](out: *mut [<wasm_ $name _vec_t>], length: usize) {
let mut bytes: Vec<[<wasm_ $name _t>]> = Vec::with_capacity(length);
let pointer = bytes.as_mut_ptr();

(*out).data = pointer;
(*out).size = length;

::std::mem::forget(bytes);
}

#[doc = "Deletes a vector of [`wasm_" $name "_t`]."]
#[doc = "Deletes a vector of [`wasm_" $name "_t`].

# Example

See the [`wasm_" $name "_vec_t`] type to get an example."]
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_delete>](ptr: Option<&mut [<wasm_ $name _vec_t>]>) {
if let Some(vec) = ptr {
Expand All @@ -130,7 +221,9 @@ macro_rules! wasm_declare_vec {
macro_rules! wasm_declare_boxed_vec {
($name:ident) => {
paste::paste! {
#[doc = "Represents of a vector of [`wasm_" $name "_t`]."]
#[doc = "Represents a vector of `wasm_" $name "_t`.

Read the documentation of [`wasm_" $name "_t`] to see more concrete examples."]
#[derive(Debug)]
#[repr(C)]
pub struct [<wasm_ $name _vec_t>] {
Expand All @@ -153,6 +246,27 @@ macro_rules! wasm_declare_boxed_vec {
}
}

impl<'a, T: Into<[<wasm_ $name _t>]> + Clone> From<&'a [T]> for [<wasm_ $name _vec_t>] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such implementation exists for regular vecs. Now it exists for boxed vecs too.

fn from(other: &'a [T]) -> Self {
let size = other.len();
let mut copied_data = other
.iter()
.cloned()
.map(Into::into)
.map(Box::new)
.map(Box::into_raw)
.collect::<Vec<*mut [<wasm_ $name _t>]>>()
.into_boxed_slice();
let data = copied_data.as_mut_ptr();
::std::mem::forget(copied_data);

Self {
size,
data,
}
}
}

// TODO: do this properly
impl [<wasm_ $name _vec_t>] {
pub unsafe fn into_slice(&self) -> Option<&[Box<[<wasm_ $name _t>]>]>{
Expand All @@ -171,33 +285,75 @@ macro_rules! wasm_declare_boxed_vec {
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_new>](out: *mut [<wasm_ $name _vec_t>], length: usize, init: *const *mut [<wasm_ $name _t>]) {
let mut bytes: Vec<*mut [<wasm_ $name _t>]> = Vec::with_capacity(length);

for i in 0..length {
bytes.push(*init.add(i));
}

let pointer = bytes.as_mut_ptr();
debug_assert!(bytes.len() == bytes.capacity());

(*out).data = pointer;
(*out).size = length;

::std::mem::forget(bytes);
}

#[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`]."]
#[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`].

# Example

```rust
# use inline_c::assert_c;
# fn main() {
# (assert_c! {
# #include \"tests/wasmer_wasm.h\"
#
int main() {
// Creates an empty vector of `wasm_" $name "_t`.
wasm_" $name "_vec_t vector;
wasm_" $name "_vec_new_uninitialized(&vector, 3);

// Check that it contains 3 items.
assert(vector.size == 3);

// Free it.
wasm_" $name "_vec_delete(&vector);
}
# })
# .success();
# }
```"]
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_new_uninitialized>](out: *mut [<wasm_ $name _vec_t>], length: usize) {
let mut bytes: Vec<*mut [<wasm_ $name _t>]> = Vec::with_capacity(length);
let pointer = bytes.as_mut_ptr();

(*out).data = pointer;
(*out).size = length;

::std::mem::forget(bytes);
}

#[doc = "Deletes a vector of [`wasm_" $name "_t`]."]
#[doc = "Deletes a vector of [`wasm_" $name "_t`].

# Example

See the [`wasm_" $name "_vec_t`] type to get an example."]
#[no_mangle]
pub unsafe extern "C" fn [<wasm_ $name _vec_delete>](ptr: *mut [<wasm_ $name _vec_t>]) {
Hywan marked this conversation as resolved.
Show resolved Hide resolved
let vec = &mut *ptr;
Hywan marked this conversation as resolved.
Show resolved Hide resolved

if !vec.data.is_null() {
let data: Vec<*mut [<wasm_ $name _t>]> = Vec::from_raw_parts(vec.data, vec.size, vec.size);
let _data: Vec<Box<[<wasm_ $name _t>]>> = ::std::mem::transmute(data);

// If the vector has been initialized (we check
// only the first item), we can transmute items to
// `Box`es.
if vec.size > 0 && !data[0].is_null() {
let _data: Vec<Box<[<wasm_ $name _t>]>> = ::std::mem::transmute(data);
Hywan marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes allows to delete an uninitialized boxed vec. Should we check for all items or checking the first is safe enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

For handling uninitialized, I think we just have to make a representation of uninit that makes sense... I think the code as-is will break in real code (for example partially initialized vectors. I'll get back to you with more details after finishing the review and looking at the code locally


vec.data = ::std::ptr::null_mut();
vec.size = 0;
}
Expand All @@ -222,7 +378,6 @@ macro_rules! wasm_declare_ref_base {
}

// TODO: finish this...

}
};
}
Expand Down
2 changes: 1 addition & 1 deletion lib/c-api/src/wasm_c_api/types/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,4 @@ pub unsafe extern "C" fn wasm_frame_module_offset(frame: &wasm_frame_t) -> usize
frame.info.module_offset()
}

wasm_declare_vec!(frame);
wasm_declare_boxed_vec!(frame);
47 changes: 4 additions & 43 deletions lib/c-api/src/wasm_c_api/types/function.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
use super::{
wasm_externtype_t, wasm_valtype_t, wasm_valtype_vec_delete, wasm_valtype_vec_t, WasmExternType,
};
use std::mem;
use super::{wasm_externtype_t, wasm_valtype_vec_delete, wasm_valtype_vec_t, WasmExternType};
use wasmer::{ExternType, FunctionType, ValType};

#[derive(Debug)]
Expand All @@ -13,44 +10,8 @@ pub(crate) struct WasmFunctionType {

impl WasmFunctionType {
pub(crate) fn new(function_type: FunctionType) -> Self {
let params = {
let mut valtypes = function_type
.params()
.iter()
.cloned()
.map(Into::into)
.map(Box::new)
.map(Box::into_raw)
.collect::<Vec<*mut wasm_valtype_t>>();

let valtypes_vec = Box::new(wasm_valtype_vec_t {
size: valtypes.len(),
data: valtypes.as_mut_ptr(),
});

mem::forget(valtypes);

valtypes_vec
};
let results = {
let mut valtypes = function_type
.results()
.iter()
.cloned()
.map(Into::into)
.map(Box::new)
.map(Box::into_raw)
.collect::<Vec<*mut wasm_valtype_t>>();

let valtypes_vec = Box::new(wasm_valtype_vec_t {
size: valtypes.len(),
data: valtypes.as_mut_ptr(),
});

mem::forget(valtypes);

valtypes_vec
};
let params: Box<wasm_valtype_vec_t> = Box::new(function_type.params().into());
let results: Box<wasm_valtype_vec_t> = Box::new(function_type.results().into());

Self {
function_type,
Expand Down Expand Up @@ -90,7 +51,7 @@ impl wasm_functype_t {
}
}

wasm_declare_vec!(functype);
wasm_declare_boxed_vec!(functype);

#[no_mangle]
pub unsafe extern "C" fn wasm_functype_new(
Expand Down
2 changes: 1 addition & 1 deletion lib/c-api/src/wasm_c_api/types/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ impl wasm_globaltype_t {
}
}

wasm_declare_vec!(globaltype);
wasm_declare_boxed_vec!(globaltype);

#[no_mangle]
pub unsafe extern "C" fn wasm_globaltype_new(
Expand Down
2 changes: 1 addition & 1 deletion lib/c-api/src/wasm_c_api/types/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl wasm_memorytype_t {
}
}

wasm_declare_vec!(memorytype);
wasm_declare_boxed_vec!(memorytype);

#[no_mangle]
pub unsafe extern "C" fn wasm_memorytype_new(limits: &wasm_limits_t) -> Box<wasm_memorytype_t> {
Expand Down
2 changes: 1 addition & 1 deletion lib/c-api/src/wasm_c_api/types/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl wasm_tabletype_t {
}
}

wasm_declare_vec!(tabletype);
wasm_declare_boxed_vec!(tabletype);

#[no_mangle]
pub unsafe extern "C" fn wasm_tabletype_new(
Expand Down