feat(c-api) Implement `wasi_get_unordered_imports` #2053

Hywan · 2021-01-25T15:09:47Z

Depend on #2083.

Description

The problem I've with the C API for WASI is the following:

// At some point, we write this:
bool success = wasi_get_imports(store, module, wasi_env, &imports_to_collect);

This function iterates over the module imports, and for each entry, it looks inside the (inner generated) WASI import object if a matching entry exists. If it doesn't, it fails. See:

wasmer/lib/c-api/src/wasm_c_api/wasi/mod.rs

Lines 333 to 356 in 6b02841

    
           let import_object = generate_import_object_from_env(store, wasi_env.inner.clone(), version); 
        
           *imports = module 
        
               .inner 
        
               .imports() 
        
               .map(|import_type| { 
        
                   let export = c_try!(import_object 
        
                       .resolve_by_name(import_type.module(), import_type.name()) 
        
                       .ok_or_else(|| CApiError { 
        
                           msg: format!( 
        
                               "Failed to resolve import \"{}\" \"{}\"", 
        
                               import_type.module(), 
        
                               import_type.name() 
        
                           ), 
        
                       })); 
        
                   let inner = Extern::from_vm_export(store, export); 
        
                   Some(Box::new(wasm_extern_t { 
        
                       instance: None, 
        
                       inner, 
        
                   })) 
        
               }) 
        
               .collect::<Option<Vec<_>>>()? 
        
               .into();

So… if a module requires WASI and WASI only, it works like a charm of course.
But, if a module requires WASI + other imports, it will fail.

And I think it's a common pattern to use WASI + some other imports (like math.random or anything related to I/O etc etc.).

What we need is an API to collect all the WASI imports, in no particular order, by module/namespace and name, so that we can implement an API like ImportObject to re-order the imports when needed (i.e. when passing a vec of externs to wasm_instance_new for example).

Review

Add a short description of the the change to the CHANGELOG.md file

MarkMcCaskey

I like it! But I'm hesitant to add new APIs that users may use without a lot of thought as we'll be stuck with them for a while

lib/c-api/src/wasm_c_api/wasi/mod.rs

Hywan · 2021-01-28T13:17:26Z

I like it! But I'm hesitant to add new APIs that users may use without a lot of thought as we'll be stuck with them for a while

Well, in an ideal world, we would need an ImportObject API in C, but we are stuck with the Wasm C API, so I'm not sure we can do better for the moment. WASI has no standard C API for the moment neither.

MarkMcCaskey

There's a lot to unpack here, this is my initial review; I'll need to do another pass about ownership / see more examples of the ownership in action (maybe we need a test using these new functions to do something)

lib/c-api/src/wasm_c_api/externals/mod.rs

lib/c-api/src/wasm_c_api/types/export.rs

lib/c-api/src/wasm_c_api/types/import.rs

lib/c-api/src/wasm_c_api/wasi/mod.rs

MarkMcCaskey · 2021-01-28T16:28:42Z

lib/c-api/src/wasm_c_api/wasi/mod.rs

+                r#extern: Box::new(wasm_extern_t {
+                    instance: None,
+                    inner: extern_inner,
+                }),


TODO: review instance: None, aren't wasm_extern_ts callable? is instance: None a problem for data being passed to the user?

We don't have an access to Instance here since we read the externs from Module.

wasm_extern_t is defined as:

pub struct wasm_extern_t { // this is how we ensure the instance stays alive pub(crate) instance: Option<Arc<Instance>>, pub(crate) inner: Extern, }

We don't need to ensure that Instance stays alive, it doesn't exist at this step :-).

MarkMcCaskey · 2021-01-28T16:31:13Z

lib/c-api/wasmer.h

-#if defined(WASMER_EMSCRIPTEN_ENABLED)
-/**
- * Convenience function for setting up arguments and calling the Emscripten
- * main function.
- *
- * WARNING:
- *
- * Do not call this function on untrusted code when operating without
- * additional sandboxing in place.
- * Emscripten has access to many host system calls and therefore may do very
- * bad things.
- */
-wasmer_result_t wasmer_emscripten_call_main(wasmer_instance_t *instance,
-                                            const wasmer_byte_array *args,
-                                            unsigned int args_len);
-#endif
-
-#if defined(WASMER_EMSCRIPTEN_ENABLED)
-/**
- * Destroy `wasmer_emscrpten_globals_t` created by
- * `wasmer_emscripten_get_emscripten_globals`.
- */
-void wasmer_emscripten_destroy_globals(wasmer_emscripten_globals_t *globals);
-#endif
-
-#if defined(WASMER_EMSCRIPTEN_ENABLED)
-/**
- * Create a `wasmer_import_object_t` with Emscripten imports, use
- * `wasmer_emscripten_get_emscripten_globals` to get a
- * `wasmer_emscripten_globals_t` from a `wasmer_module_t`.
- *
- * WARNING:
- *
- * This `import_object_t` contains thin-wrappers around host system calls.
- * Do not use this to execute untrusted code without additional sandboxing.
- */
-wasmer_import_object_t *wasmer_emscripten_generate_import_object(wasmer_emscripten_globals_t *globals);
-#endif
-
-#if defined(WASMER_EMSCRIPTEN_ENABLED)
-/**
- * Create a `wasmer_emscripten_globals_t` from a Wasm module.
- */
-wasmer_emscripten_globals_t *wasmer_emscripten_get_globals(const wasmer_module_t *module);
-#endif
-
-#if defined(WASMER_EMSCRIPTEN_ENABLED)
-/**
- * Execute global constructors (required if the module is compiled from C++)
- * and sets up the internal environment.
- *
- * This function sets the data pointer in the same way that
- * [`wasmer_instance_context_data_set`] does.
- */
-wasmer_result_t wasmer_emscripten_set_up(wasmer_instance_t *instance,
-                                         wasmer_emscripten_globals_t *globals);
-#endif
-


Do we want to commit these deletions?

To be honest, I didn't know why they were still here. As far as I know, our C API no longer ship with emscripten, is that right?

lib/c-api/wasmer.hh

MarkMcCaskey · 2021-01-28T16:33:00Z

lib/c-api/wasmer_wasm.h

+ * Non-standard function to get the module name of a
+ * `wasm_named_extern_t`.
+ */
+const wasm_name_t *wasm_named_extern_module(const wasm_named_extern_t *named_extern);


This may be a wider problem but we need to mark things as own in our extension to the Wasm C API, otherwise it's ~~really hard~~ impossible to know what needs to be freed by who

Yup. I just don't know how to add attributes inside the C headers from Rust. I'm not even sure if it's doable.

I'll explain that in the documentation.

(in this case, ownership isn't transferred by the way)

While documenting onwership, I found a memory leak. We said in wasi_get_imports and in wasi_get_unordered_imports that the ownership of the wasi_env_t parameter is taken by the function, which wasn't the case. It was memory leak.

It's not fixed.

Edit: see 9e63ba9.

lib/c-api/wasmer_wasm.h

…nwrap`.

…API.

This patch also moves `wasm_extern_delete` into the correct Rust module.

In `wasi_get_imports` and `wasi_get_unordered_imports`, we said the ownership of `wasi_env_t` was taken by the function, but it wasn't the case. This patch changes the type from `&wasi_env_t` to `Box<wasi_env_t>` to take ownership of it. The rest of the patch updates the documentation, and improves null protections with `Option<T>`.

MarkMcCaskey

Alright, if there are any problems with this I'm not able to see them right now!

I did become aware again that we're using unsafe in a lot of places that we don't need to, but we can fix that separately

lib/c-api/build.rs

lib/c-api/src/wasm_c_api/wasi/mod.rs

2074: feat(package) Be consistent on Windows regarding `libwasmer` name. r=jubianchi a=Hywan # Description Windows package libraries aren't named consistently regarding other platforms. This patch fixes that. # Review - [ ] Add a short description of the the change to the CHANGELOG.md file 2083: doc(c-api) Remove inline documentation in `wasmer_wasm.h`, clarification about stability… r=Hywan a=Hywan # Description As suggested in #2053 (comment), this PR removes the automatically generated documentation when building `wasmer_wasm.h`. It also clarifies our position regarding stability, and add a section about the documentation. This PR takes also the opportunity to mark `wasi_env_set_(instance|memory)` as deprecated functions. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Ivan Enderlin <[email protected]>

Hywan · 2021-02-01T14:50:09Z

bors r+

2053: feat(c-api) Implement `wasi_get_unordered_imports` r=Hywan a=Hywan Depend on #2083. # Description The problem I've with the C API for WASI is the following: ```c // At some point, we write this: bool success = wasi_get_imports(store, module, wasi_env, &imports_to_collect); ``` This function iterates over the module imports, and for each entry, it looks inside the (inner generated) WASI import object if a matching entry exists. If it doesn't, it fails. See: https://github.com/wasmerio/wasmer/blob/6b028410c23da088d62a6b6919e2c086931ad101/lib/c-api/src/wasm_c_api/wasi/mod.rs#L333-L356 So… if a module requires WASI and WASI only, it works like a charm of course. But, if a module requires WASI + other imports, it will fail. And I think it's a common pattern to use WASI + some other imports (like `math.random` or anything related to I/O etc etc.). What we need is an API to collect all the WASI imports, in no particular order, by module/namespace and name, so that we can implement an API like `ImportObject` to re-order the imports when needed (i.e. when passing a vec of externs to `wasm_instance_new` for example). # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Ivan Enderlin <[email protected]>

bors · 2021-02-01T14:57:47Z

Build failed:

Code lint

2083: doc(c-api) Remove inline documentation in `wasmer_wasm.h`, clarification about stability… r=Hywan a=Hywan # Description As suggested in #2053 (comment), this PR removes the automatically generated documentation when building `wasmer_wasm.h`. It also clarifies our position regarding stability, and add a section about the documentation. This PR takes also the opportunity to mark `wasi_env_set_(instance|memory)` as deprecated functions. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Ivan Enderlin <[email protected]>

… nightly. So. Let's explain a dirty hack. `cbindgen` reads the code and collects symbols. What symbols do we need? None of the one declared in `wasm.h`, but for non-standard API, we need to collect all of them. The problem is that `wasm_named_extern_t` is the only non-standard type where extra symbols are generated by a macro (`wasm_declare_boxed_vec!`). If we want those macro-generated symbols to be collected by `cbindgen`, we need to _expand_ the crate (i.e. running something like `rustc -- -Zunstable-options --pretty=expanded`). Expanding code is unstable and available only on nightly compiler. We _don't want_ to use a nightly compiler only for that. So how can we help `cbindgen` to _see_ those symbols? First solution: We write the C code directly in a file, which is then included in the generated header file with the `cbindgen` API. Problem, it's super easy to get it outdated, and it makes the build process more complex. Second solution: We write those symbols in a custom module, that is just here for `cbindgen`, never used by our Rust code (otherwise it's duplicated code), with no particular implementation. And that's why we have the following `cbindgen_hack` module. But this module must not be compiled by `rustc`. How to force `rustc` to ignore a module? With conditional compilation. Because `cbindgen` does not support conditional compilation, it will always _ignore_ the `#[cfg]` attribute, and will always read the content of the module. Sorry.

Hywan · 2021-02-02T11:17:20Z

bors try

bors · 2021-02-02T11:41:07Z

try

Build failed:

Test on macos-x64

We said that `wasi_get_imports` was taking ownership of `wasi_env_t`. It wasn't. In 9e63ba9, we have fixed this. But it creates another bug when `wasi_env_t` is used _after_ for example when calling `wasi_env_read_stdout`. So this patch reverts the bug fix. And we will discuss about how to fix that later.

Hywan · 2021-02-02T12:45:10Z

bors r+

bors · 2021-02-02T13:18:55Z

Build succeeded:

Hywan added 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api labels Jan 25, 2021

Hywan requested a review from MarkMcCaskey January 25, 2021 15:09

Hywan self-assigned this Jan 25, 2021

MarkMcCaskey reviewed Jan 25, 2021

View reviewed changes

lib/c-api/src/wasm_c_api/wasi/mod.rs Outdated Show resolved Hide resolved

lib/c-api/src/wasm_c_api/wasi/mod.rs Outdated Show resolved Hide resolved

lib/c-api/src/wasm_c_api/wasi/mod.rs Outdated Show resolved Hide resolved

Hywan mentioned this pull request Jan 25, 2021

feat(wasi) Implement WASI wasmerio/wasmer-go#178

Merged

Hywan changed the title ~~feat(c-api) Implement wasi_get_unordered_imports~~ feat(c-api) Implement wasi_get_unordered_imports (TASK-4) Jan 25, 2021

Hywan marked this pull request as ready for review January 26, 2021 12:26

Hywan requested a review from jubianchi as a code owner January 26, 2021 12:26

Hywan changed the title ~~feat(c-api) Implement wasi_get_unordered_imports (TASK-4)~~ feat(c-api) Implement wasi_get_unordered_imports Jan 26, 2021

Hywan added a commit to Hywan/wasmer that referenced this pull request Jan 28, 2021

doc(changelog) Add wasmerio#2053.

8d842f7

MarkMcCaskey reviewed Jan 28, 2021

View reviewed changes

This was referenced Jan 29, 2021

fix(scripts) Fix publish script with wasmer-c-api Hywan/wasmer#2

Closed

fix(scripts) Fix publish script regarding wasmer-c-api #2076

Merged

doc(c-api) Remove inline documentation in wasmer_wasm.h, clarification about stability… #2083

Merged

Hywan added 14 commits February 1, 2021 11:59

feat(c-api) Implement wasi_get_unordered_imports.

dc0cf7d

chore(c-api) Reorder field for the sake of consistency.

00bf74c

chore(c-api) Update headers.

0ad4d31

feat(c-api) Expand macros of the crate when generating C bindings.

ffd07bc

fixup

8b29156

chore(c-api) Update headers.

5de7d71

feat(c-api) Rename wasm_named_extern_extern to `wasm_named_extern_u…

5abac74

…nwrap`.

chore(c-api) Exclude more functions from Wasm C API for the Wasmer C …

79b5d5c

…API.

feat(c-api) Implement wasm_extern_copy.

18761ca

This patch also moves `wasm_extern_delete` into the correct Rust module.

chore(c-api) Update headers.

1042dad

chore(c-api) Update headers.

5fc3e9f

feat(c-api) Implement From<String> for wasm_name_t.

83c69a5

chore(c-api) Merge _CBINDGEN_IS_RUNNING with DOCS_RS.

e379fd7

chore(c-api) Simplify code.

37aee2d

Hywan added 2 commits February 1, 2021 12:03

doc(changelog) Add wasmerio#2053.

b640a8c

Fix rebase.

0861b73

Hywan force-pushed the feat-c-api-wasi-unordered-imports branch from 8d842f7 to 0861b73 Compare February 1, 2021 12:34

Hywan added 6 commits February 1, 2021 13:38

test(c-api) Move a doctest into a test.

bbbbe53

chore(c-api) Make type explicit for the sake of simplicity.

b1dc041

doc(c-api) Fix documentation.

8ab82cf

chore(c-api) Update headers.

d48bc1e

doc(c-api) Remove incorrect link.

9a835a4

MarkMcCaskey approved these changes Feb 1, 2021

View reviewed changes

lib/c-api/build.rs Outdated Show resolved Hide resolved

lib/c-api/build.rs Outdated Show resolved Hide resolved

lib/c-api/src/wasm_c_api/wasi/mod.rs Show resolved Hide resolved

lib/c-api/src/wasm_c_api/wasi/mod.rs Outdated Show resolved Hide resolved

chore(c-api) Remove useless unsafe keywords.

98bff43

Hywan added 2 commits February 2, 2021 12:05

Merge branch 'master' into feat-c-api-wasi-unordered-imports

e5d5303

bors bot added a commit that referenced this pull request Feb 2, 2021

Try #2053:

bcbaa74

bors bot merged commit dd5196e into wasmerio:master Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(c-api) Implement `wasi_get_unordered_imports` #2053

feat(c-api) Implement `wasi_get_unordered_imports` #2053

Hywan commented Jan 25, 2021 •

edited

Loading

MarkMcCaskey left a comment

Hywan commented Jan 28, 2021

MarkMcCaskey left a comment

MarkMcCaskey Jan 28, 2021

Hywan Feb 1, 2021

MarkMcCaskey Jan 28, 2021

Hywan Jan 29, 2021

MarkMcCaskey Jan 28, 2021

Hywan Jan 29, 2021

Hywan Feb 1, 2021

Hywan Feb 1, 2021

Hywan Feb 1, 2021 •

edited

Loading

MarkMcCaskey left a comment

Hywan commented Feb 1, 2021

bors bot commented Feb 1, 2021

Hywan commented Feb 2, 2021

bors bot commented Feb 2, 2021

Hywan commented Feb 2, 2021

bors bot commented Feb 2, 2021

	let import_object = generate_import_object_from_env(store, wasi_env.inner.clone(), version);

	*imports = module
	.inner
	.imports()
	.map(\|import_type\| {
	let export = c_try!(import_object
	.resolve_by_name(import_type.module(), import_type.name())
	.ok_or_else(\|\| CApiError {
	msg: format!(
	"Failed to resolve import \"{}\" \"{}\"",
	import_type.module(),
	import_type.name()
	),
	}));
	let inner = Extern::from_vm_export(store, export);

	Some(Box::new(wasm_extern_t {
	instance: None,
	inner,
	}))
	})
	.collect::<Option<Vec<_>>>()?
	.into();

feat(c-api) Implement wasi_get_unordered_imports #2053

feat(c-api) Implement wasi_get_unordered_imports #2053

Conversation

Hywan commented Jan 25, 2021 • edited Loading

Description

Review

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Hywan commented Jan 28, 2021

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Hywan commented Feb 1, 2021

bors bot commented Feb 1, 2021

Hywan commented Feb 2, 2021

bors bot commented Feb 2, 2021

try

Hywan commented Feb 2, 2021

bors bot commented Feb 2, 2021

feat(c-api) Implement `wasi_get_unordered_imports` #2053

feat(c-api) Implement `wasi_get_unordered_imports` #2053

Hywan commented Jan 25, 2021 •

edited

Loading

Hywan Feb 1, 2021 •

edited

Loading