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

Begin adding WASI support #72

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Begin adding WASI support #72

merged 4 commits into from
Nov 29, 2019

Conversation

MarkMcCaskey
Copy link
Contributor

Very much a work in progress. Probably will require updating more of the existing code toward ImportObject and away from existing imports, though I'm not sure.

This PR is not in a state where it can be merged and won't be until we ship WASI support in the C API. Before doing so we want to verify, by implementing it in a language extension, that the updates to the C API are sufficient.

@Hywan
Copy link
Contributor

Hywan commented Oct 7, 2019

Thanks!

Can you rebase on top of master please? Since #73, Wasmer 0.8.0 is used, which will be easier for everyone.

@Hywan Hywan self-assigned this Oct 7, 2019
@Hywan Hywan added 🎉 enhancement New feature or request 📦 component-extension About the Go extension labels Oct 7, 2019
wasmer/import.go Outdated Show resolved Hide resolved
wasmer/instance.go Outdated Show resolved Hide resolved
@MarkMcCaskey MarkMcCaskey marked this pull request as ready for review October 29, 2019 17:44
@robinvanemden
Copy link

How far along is this branch? We currently have completed a machine learning C++ application that compiles nicely using both clang and emscripten standalone, and now runs on both Wasmer and WAVM. But it fails to instantiate the wasm using go-ext-wasm, dueto some missing WASI imports. Would be super if we can make this work, as it would be the final step towards completing our framework.

@Hywan
Copy link
Contributor

Hywan commented Nov 29, 2019

I'm literally working on this right now. Expect to be merged in a couple of hours if everything goes smoothly.

@robinvanemden
Copy link

Incredible! The timing couldn't be any better. Thanks!

wasmer-runtime-c-api = { path = "../wasmer/lib/runtime-c-api", version = "0.8.0", features = ["wasi"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just require wasmer 0.11.0, which has wasi as a default feature.

@@ -56,6 +58,109 @@ func cNewWasmerImportT(moduleName string, importName string, function *cWasmerIm
return (cWasmerImportT)(importedFunction)
}

func cGetParamsForImportFunc(function *cWasmerImportFuncT) []cWasmerValueTag {
var arity C.uint32_t = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

0 is the value, it's suggested to remove it.

}

func cGetReturnsForImportFunc(function *cWasmerImportFuncT) []cWasmerValueTag {
var arity C.uint32_t = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

0 is the default value, it's suggested to remove it.

message string
}

// A type that owns a set of imports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation must start by the name of the type or the name of the function. So it becomes:

ImportObject owns a set of imports.

It's a Go rule.

if params == nil {
return imports, NewImportedFunctionError(importName, fmt.Sprintf("could not get parameters for %s %s", namespace, importName))
}
returns := cGetParamsForImportFunc(wasmerImportFunc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we suppose to use cGetReturnsForImportfunc here?

}

// Adds the given imports to the exsiting import object
func (importObject *ImportObject) Extend(imports Imports) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think to turn this API to:

func (importObject *ImportObject) Extend(incomingImportObject ImportObject) error;

It will help to slowly deprecate the Imports API in the future if we need to.

}

var extendResult = cWasmerImportObjectExtend(importObject.inner,
(*cWasmerImportT)(unsafe.Pointer(&cImports[0])),
Copy link
Contributor

Choose a reason for hiding this comment

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

We must check that importFunctionNth is greater than 0 before reading &cImports[0].

return &ImportObjectError{message}
}

// ImportedFunctionError is an actual error. The `Error` function
Copy link
Contributor

Choose a reason for hiding this comment

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

- ImportedFunctionError
+ ImportObjectError

@@ -192,6 +192,38 @@ func (module *Module) InstantiateWithImports(imports *Imports) (Instance, error)
)
}

// ImportInstantiate creates a new instance of a WebAssembly module with an
Copy link
Contributor

Choose a reason for hiding this comment

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

- ImportInstantiate
+ InstantiateWithImportObject

;; Copied from https://github.com/CraneStation/wasmtime/blob/master/docs/WASI-tutorial.md and then modified
;; Licensed under Apache 2.0; see https://github.com/CraneStation/wasmtime/blob/master/LICENSE
(module
(import "wasi_unstable" "fd_write" (func $fd_write (param i32 i32 i32 i32) (result i32)))
Copy link
Contributor

Choose a reason for hiding this comment

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

It must be wasi_snapshot_prevew1 now.

@Hywan Hywan merged commit b6a7732 into master Nov 29, 2019
@bors bors bot deleted the feature/wasi-support branch November 29, 2019 14:12
@Hywan
Copy link
Contributor

Hywan commented Nov 29, 2019

Review feedbacks fixed by c07c101, b91099d and bf837c2 (the last one fixes WASI itself, since the namespace has changed from wasi_unstable to wasi_snapshot_preview1.

Thanks!

@Hywan
Copy link
Contributor

Hywan commented Nov 29, 2019

The test prints hello world as expected, but I wish we can find a solution to test the output to avoid regression in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 component-extension About the Go extension 🎉 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants