API for resolving function imports by gumb0 · Pull Request #318 · wasmx/fizzy

gumb0 · 2020-05-18T15:03:32Z

No description provided.

axic · 2020-05-18T18:19:11Z

lib/fizzy/execute.cpp

+            std::move(it->function), module.typesec[import.desc.function_type_index]});
+    }
+
+    if (external_functions.size() != imported_functions.size())


Please remove this.

axic · 2020-05-18T18:22:19Z

Can you add some unit tests?

chfast · 2020-05-19T10:46:09Z

Maybe to so much functional, but I was imaging the instantiation workflow to look something like:

auto module = parse(...);
auto builder = InstanceBuilder{module};
builder.add_imported_function("host", "name", func_ptr);
builder.add_imported_function("host", "xxx", func_ptr_xxx);
auto instance = builder.instantiate();

axic · 2020-05-19T11:14:23Z

I think that is an option too, but shouldn't we design that as part of the API discussion topic?

Here my goal would be something quick we can add to test the overhead of host calls.

gumb0 · 2020-05-19T14:51:21Z

Maybe to so much functional, but I was imaging the instantiation workflow to look something like:
auto module = parse(...);
auto builder = InstanceBuilder{module};
builder.add_imported_function("host", "name", func_ptr);
builder.add_imported_function("host", "xxx", func_ptr_xxx);
auto instance = builder.instantiate();

Looks good for C++ API, but maybe this should be a part of higher-level C++ bindings... I'd leave it simpler in this PR.

gumb0 · 2020-05-19T15:31:08Z

lib/fizzy/execute.hpp

 execution_result execute(const Module& module, FuncIdx func_idx, std::vector<uint64_t> args);

+// Function that should be used by instantiate as imports, identified by module and function name.
+struct ImportedFunction


I'm not very happy with the name, maybe NamedFunctionImport or NamedFunction?

gumb0 · 2020-05-19T15:35:43Z

lib/fizzy/execute.cpp

+
+        if (it == imported_functions.end())
+        {
+            throw instantiate_error(


Decided it's not worth making separate resolve_error

axic · 2020-05-19T16:11:02Z

Changelog entry perhaps?

axic · 2020-05-19T18:55:42Z

lib/fizzy/execute.hpp

+{
+    std::string module;
+    std::string name;
+    std::function<execution_result(Instance&, std::vector<uint64_t>, int depth)> function;


I think ImportedFunction should have two more fields:

std::vector<ValType> inputs;

std::optional<ValType> result; (where empty means void)

And check that against the type section. Abort on mismatch.

It can be then

struct NamedExternalFunction { std::string module; std::string name; ExternalFunction external_function; };

Yeah I guess it could, but that's quite a few different structs to address when declaring these. (From a user perspective.)

Alternative is to use a string (similar to what wasm3 and emscripten is doing): parameters followed by colon followed by return types.

e.g. ii:v would mean it takes two 32-bit inputs and returns void.

The same in wasm3 is v(ii) and iiv in emscripten.

axic · 2020-05-20T11:34:58Z

test/unittests/api_test.cpp

+        {"mod1", "foo1", {}, ValType::i32, function_returning_value(0)},
+        {"mod1", "foo2", {ValType::i32}, ValType::i32, function_returning_value(1)},
+        {"mod2", "foo1", {ValType::i32}, ValType::i32, function_returning_value(2)},
+        {"mod2", "foo2", {ValType::i64}, std::nullopt, function_returning_void},


Can you add one test case somewhere which takes 2 inputs?

Made this void function take 2 parameters

axic · 2020-05-20T14:16:53Z

Looks good to me, but as discussed in chat lets do the changelog in a single PR at the release time instead, so please remove the commit :)

gumb0 · 2020-05-20T14:23:00Z

Looks good to me, but as discussed in chat lets do the changelog in a single PR at the release time instead, so please remove the commit :)

Looks good to me, but as discussed in chat lets do the changelog in a single PR at the release time instead, so please remove the commit :)

@chfast though wanted to have it in each PR, as I understood.

gumb0 force-pushed the linking-api branch 2 times, most recently from 1f02f40 to fc6787d Compare May 18, 2020 17:21

gumb0 changed the title ~~instantiate with imported functions referred by import name~~ API for resolving function imports May 18, 2020

axic reviewed May 18, 2020

View reviewed changes

gumb0 force-pushed the linking-api branch from fc6787d to c41443d Compare May 18, 2020 18:20

gumb0 force-pushed the linking-api branch 4 times, most recently from 9a23f3c to 297bcb3 Compare May 19, 2020 15:30

gumb0 commented May 19, 2020

View reviewed changes

gumb0 marked this pull request as ready for review May 19, 2020 15:31

gumb0 requested review from axic and chfast May 19, 2020 15:34

gumb0 commented May 19, 2020

View reviewed changes

axic mentioned this pull request May 19, 2020

Add a test case for using host functions #320

Merged

axic reviewed May 19, 2020

View reviewed changes

gumb0 force-pushed the linking-api branch from 560b746 to a112025 Compare May 20, 2020 11:21

axic reviewed May 20, 2020

View reviewed changes

gumb0 force-pushed the linking-api branch from a112025 to eb6a3b9 Compare May 20, 2020 12:44

axic approved these changes May 20, 2020

View reviewed changes

gumb0 added 3 commits May 20, 2020 17:20

API for resolving imported function names into function types

fddc72d

test: resolve imported functions API

54f3764

Add changelog entry

9fc7388

Add function inputs/ouput type to ImportedFunction and check in resolve

ef200b5

axic force-pushed the linking-api branch from eb6a3b9 to ef200b5 Compare May 20, 2020 16:20

gumb0 merged commit 94320a4 into master May 20, 2020

gumb0 deleted the linking-api branch May 20, 2020 16:26

axic mentioned this pull request May 22, 2020

utils: add signature to WasmEngine::find_function #331

Merged

Conversation

gumb0 commented May 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented May 18, 2020

Uh oh!

chfast commented May 19, 2020

Uh oh!

axic commented May 19, 2020

Uh oh!

gumb0 commented May 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented May 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented May 20, 2020

Uh oh!

gumb0 commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants