C API execute #533

gumb0 · 2020-09-17T19:27:05Z

Possible options to deal with module-instance relationship.

PR currently uses 2.b approach.

Copy module during instantiate

FizzyInstance* instance = fizzy_instantiate(module, NULL, 0);
// can use module, instance has it's own copy
...
fizzy_free_module(module);
fizzy_free_instance(instance);

Invalidate module in instantiate
2.a. fizzy_free_module required after invalidation, so that any module allocated by fizzy_parse must be freed in the end by the user.

FizzyInstance* instance = fizzy_instantiate(module, NULL, 0);
// can't use module
...
fizzy_free_module(module);
fizzy_free_instance(instance);

2.b. instatiate calls fizzy_free_module, so it must not be called by user afterwards.

FizzyInstance* instance = fizzy_instantiate(module, NULL, 0);
// can't use module
...
fizzy_free_instance(instance);

Reference counting

FizzyInstance* instance = fizzy_instantiate(module, NULL, 0);
// can use module, instance has a reference to the same module
...
fizzy_free_module(module);
fizzy_free_instance(instance);

Different ways to implement this on the C++ side:
3.a. double indirection

struct FizzyModule
{
    shared_ptr<fizzy::Module> module;
};

FizzyInstance* fizzy_instantiate(FizzyModule* cmod, ...)
{
  return fizzy::instantiate(cmod->module, ...)
}

3.b. Module with enabled shared_from_this:

class Module : public std::enable_shared_from_this<Module> { ... };

fizzy::Module* unwrap(FizzyModule* cmod) { return reinterpret_cast<fizzy::Module*>(cmod); }

FizzyInstance* fizzy_instantiate(FizzyModule* cmod, ...)
{
  return fizzy::instantiate(unwrap(cmod)->shared_from_this(), ...)
}

codecov · 2020-09-21T09:17:21Z

Codecov Report

Merging #533 into master will increase coverage by 0.00%.
The diff coverage is 98.76%.

@@           Coverage Diff            @@
##           master     #533    +/-   ##
========================================
  Coverage   98.23%   98.24%            
========================================
  Files          62       62            
  Lines        9023     9185   +162     
========================================
+ Hits         8864     9024   +160     
- Misses        159      161     +2

lib/fizzy/capi.cpp

test/unittests/capi_test.cpp

axic · 2020-09-21T22:35:46Z

lib/fizzy/capi.cpp

+    delete instance;
+}
+
+fizzy_execution_result fizzy_execute(fizzy_instance* instance, uint32_t func_idx,


I don't think depth should be part of the public API, @chfast ?

I think it must be. My plan is to convert it to a more generic opaque "ThreadExecutionContext". This is a sneak peak: 5f08ee2.

Of course this is up to discussion.

Yeah, we discussed it a bit when introducing depth in C++ API. My argument for it is that without it host functions don't have an option to respect the call depth limit (calls to imported functions would reset depth to 0). For example, this test would fail (go into infinite recursion):

fizzy/test/unittests/execute_call_test.cpp

Line 506 in 84abe56

TEST(execute_call, call_imported_infinite_recursion)

axic · 2020-09-22T15:12:56Z

@gumb0 can you rebase this on #530 ?

lib/fizzy/capi.cpp

axic · 2020-09-22T15:56:09Z

We need to think about a similar feature as "resolve imports" in C++. wabt/wasm3 have something called"add host function".

include/fizzy/fizzy.h

chfast · 2020-09-24T14:40:39Z

lib/fizzy/capi.cpp

+
+        auto instance = fizzy::instantiate(std::move(module->module), std::move(functions));
+
+        auto cinstance = std::make_unique<fizzy_instance>();


In case C++ function returns a pointer, it will be better to just convert it to opaque type pointer.
reinterpret_cast<fizzy_instance*>(instance.release()). This is correct, although looks ugly. You can additionally add wrap/unwrap functions to have ugliness under control.

fizzy_instance* wrap(Instance* x) { return reinterpret_cast<fizzy_instance*>(x); } Instance* unwrap(fizzy_instance* x) { return reinterpret_cast<instance*>(x); }

Or alternatively,

fizzy_instance* wrap(std::unique_ptr<Instance> x) { return reinterpret_cast<fizzy_instance*>(x.release()); } std::unique_ptr<Instance> unwrap(fizzy_instance* x) { return {reinterpret_cast<instance*>(x)}; }

The second one looks better to me as it is clear that after unwrap() you own the Instance.

Obviously, the Module is an exception here, so I'm ok with inspecting it some other day.

Yeah this is better. For some reason I thought having Instance& in other parts of C++ API would make this not possible, but now I think it should work.

lib/fizzy/capi.cpp

chfast · 2020-09-24T14:46:57Z

test/compilation/compilation_test.c

@@ -4,9 +4,86 @@

 #include <fizzy/fizzy.h>

-bool dummy(void);
+bool validate(const uint8_t* binary, size_t size);


For the future, the "compilation test" is to check if the code in the header is valid C99, C11. The is no need to add code to it.

I know, but I can imagine the case when the header compiles, but then calling the function doesn't compile in C like you imagine it would.

There are unittests for that.

The ones compiled in C99 and C11?

chfast · 2020-09-24T14:47:36Z

test/unittests/capi_test.cpp

+TEST(capi, parse)
+{
+    uint8_t wasm_prefix[]{0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00};
+    auto module = fizzy_parse(wasm_prefix, sizeof(wasm_prefix));


Suggested change

auto module = fizzy_parse(wasm_prefix, sizeof(wasm_prefix));

auto module = fizzy_parse(wasm_prefix, std::size(wasm_prefix));

Why is this better?

Counts number of elements, not number of bytes. Here it is the same, so it does not matter.

test/unittests/capi_test.cpp

chfast · 2020-09-24T14:51:56Z

include/fizzy/fizzy.h

+/// @param args_size   Size of the  argument array.
+/// @param depth        Call stack depth.
+typedef struct fizzy_execution_result (*fizzy_external_fn)(void* context,
+    struct fizzy_instance* instance, const union fizzy_value* args, uint32_t args_size, int depth);


The uint32_t will be annoying to use, let's use size_t for array sizes for now (as in fizzy_validate, fizzy_parse, etc).

Suggested change

struct fizzy_instance* instance, const union fizzy_value* args, uint32_t args_size, int depth);

struct fizzy_instance* instance, const union fizzy_value* args, size_t args_size, int depth);

axic · 2020-09-30T14:41:57Z

include/fizzy/fizzy.h

+
+/// Free resources associated with the module.
+///
+/// Should be called only unless @p module was passed to fizzy_instantiate.


Opt 1:

Suggested change

/// Should be called only unless @p module was passed to fizzy_instantiate.

/// Should be called unless @p module was passed to fizzy_instantiate.

Opt 2:

Suggested change

/// Should be called only unless @p module was passed to fizzy_instantiate.

/// Should be called only if @p module was passed to fizzy_instantiate.

axic · 2020-09-30T14:42:32Z

include/fizzy/fizzy.h

+/// @param args         Pointer to the argument array.
+/// @param args_size    Size of the argument array.
+/// @param depth        Call stack depth.
+FizzyExecutionResult fizzy_execute(struct FizzyInstance* instance, uint32_t func_idx,


Why is depth signed?

In C++ we chose it because it's not defined in the spec, int is a natural type for value involving some arithmetics. Also UBSan can detect overflows for it in tests.

It might be more natural / user-friendly to make it unsigned ot size_t in C.

axic · 2020-09-30T19:42:02Z

This needs to add something like find_function from utils/fizzy_engine.cpp.

chfast · 2020-10-01T13:53:01Z

include/fizzy/fizzy.h

+
+/// Pointer to external function.
+///
+/// @param context      Opaque pointer to execution context.


Suggested change

/// @param context Opaque pointer to execution context.

/// @param context Opaque pointer to execution context.

lib/fizzy/capi.cpp

axic · 2020-10-01T14:03:57Z

include/fizzy/fizzy.h

+///
+/// @param context      Opaque pointer to execution context.
+/// @param instance     Pointer to module instance.
+/// @param args         Pointer to the argument array.


Can args be null?

axic · 2020-10-01T14:04:04Z

include/fizzy/fizzy.h

+/// @param context      Opaque pointer to execution context.
+/// @param instance     Pointer to module instance.
+/// @param args         Pointer to the argument array.
+/// @param args_size   Size of the  argument array.


Suggested change

/// @param args_size Size of the argument array.

/// @param args_size Size of the argument array.

axic · 2020-10-01T14:04:38Z

include/fizzy/fizzy.h

+/// Execute module function.
+///
+/// @param instance     Pointer to module instance.
+/// @param args         Pointer to the argument array.


Can args be null?

gumb0 · 2020-10-01T14:06:11Z

lib/fizzy/capi.cpp


 extern "C" {
+struct FizzyModule
+{
+    fizzy::Module module;


I realize now, that this is not going to work for the case when we want to access already instantiated module, i.e. get FizzyModule* from FizzyInstance* which owns it.

Module has to be dynamically allocated on the C++ side, then on C side module and instance will be represented similarly.

gumb0 · 2020-10-12T12:53:55Z

Closing in favor of #576

gumb0 force-pushed the capi_execute branch 3 times, most recently from 5def5eb to 7537734 Compare September 21, 2020 09:05

gumb0 force-pushed the capi_execute branch 5 times, most recently from cc3402d to a2d0e67 Compare September 21, 2020 14:01

axic reviewed Sep 21, 2020

View reviewed changes

lib/fizzy/capi.cpp Show resolved Hide resolved

axic reviewed Sep 21, 2020

View reviewed changes

test/unittests/capi_test.cpp Show resolved Hide resolved

axic reviewed Sep 21, 2020

View reviewed changes

test/unittests/capi_test.cpp Outdated Show resolved Hide resolved

axic mentioned this pull request Sep 21, 2020

Rust bindings (validate only) #538

Merged

4 tasks

axic reviewed Sep 21, 2020

View reviewed changes

axic mentioned this pull request Sep 22, 2020

Public C API (validation only) #530

Merged

gumb0 force-pushed the capi_execute branch 2 times, most recently from 22ba04d to 7fcde82 Compare September 22, 2020 14:59

axic changed the base branch from master to public_capi September 22, 2020 15:08

gumb0 force-pushed the capi_execute branch from 7fcde82 to d798a18 Compare September 22, 2020 15:23

axic reviewed Sep 22, 2020

View reviewed changes

lib/fizzy/capi.cpp Outdated Show resolved Hide resolved

gumb0 force-pushed the capi_execute branch from d798a18 to 0f35db4 Compare September 22, 2020 15:28

gumb0 force-pushed the capi_execute branch 2 times, most recently from e78a350 to f23345a Compare September 23, 2020 14:06

chfast force-pushed the public_capi branch from d7f8c95 to 1386148 Compare September 23, 2020 20:31

gumb0 force-pushed the capi_execute branch 3 times, most recently from 345af61 to 8ffb157 Compare September 24, 2020 12:53

chfast reviewed Sep 24, 2020

View reviewed changes

chfast force-pushed the public_capi branch from 98a135c to 8f5317f Compare September 30, 2020 12:49

Base automatically changed from public_capi to master September 30, 2020 13:30

gumb0 force-pushed the capi_execute branch from a9b4164 to c14d1d9 Compare September 30, 2020 13:33

axic reviewed Sep 30, 2020

View reviewed changes

gumb0 force-pushed the capi_execute branch from ea47ca0 to c14d1d9 Compare September 30, 2020 16:19

axic force-pushed the capi_execute branch from c14d1d9 to d44b309 Compare September 30, 2020 18:56

gumb0 force-pushed the capi_execute branch 2 times, most recently from 6871e47 to c517410 Compare October 1, 2020 13:05

axic force-pushed the capi_execute branch from c517410 to 815497a Compare October 1, 2020 13:47

chfast reviewed Oct 1, 2020

View reviewed changes

axic reviewed Oct 1, 2020

View reviewed changes

gumb0 commented Oct 1, 2020

View reviewed changes

gumb0 mentioned this pull request Oct 1, 2020

C API instantiate with resolving imported functions #565

Closed

gumb0 marked this pull request as draft October 2, 2020 12:48

gumb0 force-pushed the capi_execute branch 2 times, most recently from dfdbfbf to 6525b9d Compare October 5, 2020 11:17

gumb0 mentioned this pull request Oct 5, 2020

C API execute with dynamically allocated module #576

Merged

gumb0 added 3 commits October 8, 2020 18:08

ci: add capi_test.cpp to wat2wasm4cpp check

34c8e95

capi: Add functions for parse, instantiate, execute

06e0f20

test: compilation of fizzy_parse, fizzy_instantiate, fizzy_execute

2a3d5c7

gumb0 force-pushed the capi_execute branch from 6525b9d to 2a3d5c7 Compare October 8, 2020 16:13

gumb0 closed this Oct 12, 2020

axic deleted the capi_execute branch October 12, 2020 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C API execute #533

C API execute #533

gumb0 commented Sep 17, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

axic Sep 21, 2020

chfast Sep 22, 2020

gumb0 Sep 22, 2020

axic commented Sep 22, 2020

axic commented Sep 22, 2020

chfast Sep 24, 2020

gumb0 Sep 24, 2020

chfast Sep 24, 2020

gumb0 Sep 24, 2020

chfast Sep 24, 2020

gumb0 Sep 24, 2020

chfast Sep 24, 2020

gumb0 Sep 25, 2020

chfast Sep 25, 2020

chfast Sep 24, 2020

axic Sep 30, 2020

axic Sep 30, 2020

gumb0 Sep 30, 2020

axic commented Sep 30, 2020

chfast Oct 1, 2020

axic Oct 1, 2020

axic Oct 1, 2020 •

edited

Loading

axic Oct 1, 2020

gumb0 Oct 1, 2020

gumb0 commented Oct 12, 2020


		auto instance = fizzy::instantiate(std::move(module->module), std::move(functions));

		auto cinstance = std::make_unique<fizzy_instance>();

	auto module = fizzy_parse(wasm_prefix, sizeof(wasm_prefix));
	auto module = fizzy_parse(wasm_prefix, std::size(wasm_prefix));

	struct fizzy_instance* instance, const union fizzy_value* args, uint32_t args_size, int depth);
	struct fizzy_instance* instance, const union fizzy_value* args, size_t args_size, int depth);

	/// Should be called only unless @p module was passed to fizzy_instantiate.
	/// Should be called unless @p module was passed to fizzy_instantiate.

	/// Should be called only unless @p module was passed to fizzy_instantiate.
	/// Should be called only if @p module was passed to fizzy_instantiate.

	/// @param context Opaque pointer to execution context.
	/// @param context Opaque pointer to execution context.

	/// @param args_size Size of the argument array.
	/// @param args_size Size of the argument array.

C API execute #533

C API execute #533

Conversation

gumb0 commented Sep 17, 2020 • edited Loading

codecov bot commented Sep 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Sep 22, 2020

axic commented Sep 22, 2020

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

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

axic commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Oct 12, 2020

gumb0 commented Sep 17, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

axic Oct 1, 2020 •

edited

Loading