api: resolving imported globals #637

gumb0 · 2020-11-10T19:25:31Z

~~This extends resolve_imported_functions function into supporting all kinds of imports.~~

I decided to leave here only support for imported globals. Resolving imported table and memory doesn't seem very useful, becasue there's always at most one of each. I suggest to postpone that till we see there's real need to resolve them.

C API part will be in another PR.

codecov · 2020-11-11T11:41:26Z

Codecov Report

Merging #637 (570e8ef) into master (4e4d86f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #637   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files          72       72           
  Lines       10470    10542   +72     
=======================================
+ Hits        10398    10470   +72     
  Misses         72       72

Flag	Coverage Δ
spectests	`90.84% <0.00%> (-0.66%)`	⬇️
unittests	`99.31% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/fizzy/instantiate.hpp	`100.00% <ø> (ø)`
lib/fizzy/instantiate.cpp	`100.00% <100.00%> (ø)`
test/unittests/api_test.cpp	`100.00% <100.00%> (ø)`

gumb0 · 2020-11-12T11:09:41Z

@chfast @axic Please check that it's an ok direction for resolve API.

I will add tests and header comments next.

lib/fizzy/instantiate.hpp

chfast · 2020-11-12T14:33:03Z

@chfast @axic Please check that it's an ok direction for resolve API.

Add description first.

gumb0 · 2020-11-12T14:35:05Z

@chfast @axic Please check that it's an ok direction for resolve API.

Add description first.

Added.

chfast

Looks ok, but I'm confused how this was done previously.

chfast · 2020-11-12T14:50:41Z

lib/fizzy/instantiate.hpp

+// Create vectors of External* ready to be passed to instantiate.
+// imported_* may be in any order,
+// but must contain imports for all of the import names defined in the module.
+Externals resolve_imports(const Module& module, std::vector<ImportedFunction> imported_functions,


The std::span should be used as the parameters type, but I'm not sure how good our implementation is.

I tried it, and span doesn't look suitable here, because it's supposed to be created over existing container (it doesn't have an initializer_list constructor, because initializer_list is not a container, but just a temporary), so the vector would need to be explicitly created on the caller side. Whereas now it's possible to pass just an initializer list.

Now:

auto imports = fizzy::resolve_imported_functions( *module, { {"env", "adler32", {fizzy::ValType::i32, fizzy::ValType::i32}, fizzy::ValType::i32, env_adler32}, });

With span:

auto imports = fizzy::resolve_imported_functions( *module, std::vector<ImportedFunction>{ {"env", "adler32", {fizzy::ValType::i32, fizzy::ValType::i32}, fizzy::ValType::i32, env_adler32}, });

I think I'll leave it as vector const reference.

axic · 2020-11-12T20:26:40Z

lib/fizzy/instantiate.hpp

+// Create vectors of External* ready to be passed to instantiate.
+// imported_* may be in any order,
+// but must contain imports for all of the import names defined in the module.
+Externals resolve_imports(const Module& module, std::vector<ImportedFunction> imported_functions,


I think it would be nice to structure this differently, from a consumer side:

Externals externals(module); externals.resolve_global(ExternalGlobal); externals.resolve_globals(...); // with a span

At what point would it check that all required imports are provided? In a separate bool is_complete() method?

I thinks I'm still more in favor of simpler functional API like vector<External*> resolve_imported_*(Module, span<Imported*>) because the rest of the API is mostly functional.

(Otherwise it seems better to make this a full-fledged builder object with instantiate method in the end, but then there'll probably be some complications around module and externals being consumed by instantiate...)

I will split resolve_imports into 4 functions for now and will try to use span.

I'm not convinced this is good design, but I don't think this is important right now. Good API is hard to do on paper, so I will accept whatever forks for Rust currently.

Better is to observe how this works in some real-life examples. We have two of these right now: fizzy-wasi and FizzyEngine.

In general I'm not content either with any of the options for resolve proposed so far, and would just pick something good enough.

(fizzy-wasi and FizzyEngine are quite simplistic use cases, as they import only functions. I think any of the options would look fine in them)

My point is we should start with something "good enough" and the tune it based on example integrations, not on intuition nor unit tests.

gumb0 · 2020-11-18T18:35:36Z

lib/fizzy/instantiate.cpp

+        throw instantiate_error{"global " + module + "." + name +
+                                " mutability doesn't match imported global in module"};
+    }
+


Should we check here also that it->value != nullptr ?
(note that it's checked in instantiate anyway)

I think it is fine as is. Perhaps a comment here could be useful to say that instantiate will validate the presence of a value.

Added a comment.

gumb0 · 2021-01-05T15:27:08Z

Rebased.

axic · 2021-01-27T10:12:30Z

lib/fizzy/instantiate.cpp

-        }
-
-        external_functions.emplace_back(ExternalFunction{it->function, module_func_type});
+        external_functions.emplace_back(find_imported_function(


Is this refactoring changing behaviour? i.e. it would have stopped with instantiate_error in certain cases, but now it will catch an assert?

No, it just changes the order, the checks for instantiate_error are moved into find_imported_function.

Assertion is guaranteed to be true by parser.

Yes I see the checks are moved to find_imported_function, but now they are executed post assertions.

They are independent - if is checking import's name is present in imported_functions argument, and assertion is checking that import's function index is valid (must be already ensured by validation).

axic · 2021-01-27T10:16:48Z

lib/fizzy/instantiate.hpp

+{
+    std::string module;
+    std::string name;
+    Value* value = nullptr;


Hmm, ~~module/name~~/value does not seem to be used ye?

module/name is used in find_imported_global, but where is value used?

find_imported_global puts it into returned ExternalGlobal

Ah yes I missed the return value.

axic · 2021-01-27T11:19:15Z

test/unittests/api_test.cpp

+    EXPECT_THROW_MESSAGE(resolve_imported_globals(*module, imported_globals_invalid_type1),
+        instantiate_error, "global mod1.g1 value type doesn't match imported global in module");
+
+    const std::vector<ImportedGlobal> imported_globals_invalid_type2 = {


Suggested change

const std::vector<ImportedGlobal> imported_globals_invalid_type2 = {

const std::vector<ImportedGlobal> imported_globals_invalid_mutability = {

axic · 2021-01-27T11:19:31Z

test/unittests/api_test.cpp

+    EXPECT_THROW_MESSAGE(resolve_imported_globals(*module, imported_globals_missing),
+        instantiate_error, "imported global mod2.g2 is required");
+
+    const std::vector<ImportedGlobal> imported_globals_invalid_type1 = {


Suggested change

const std::vector<ImportedGlobal> imported_globals_invalid_type1 = {

const std::vector<ImportedGlobal> imported_globals_invalid_type = {

They need to be copied, so there's no point in passing by value.

axic · 2021-01-27T11:47:32Z

lib/fizzy/instantiate.hpp

+};
+
+// Create vector of ExternalGlobals ready to be passed to instantiate.
+// imported_globals may be in any order, but must contain globals for all of the imported global


Just realised these comments are no doxygen, neither for resolve_imported_functions. If we want to doxygenize them, lets do it in a new PR.

gumb0 force-pushed the resolve-imports branch 4 times, most recently from a4f6818 to e466b25 Compare November 11, 2020 11:15

gumb0 force-pushed the resolve-imports branch 4 times, most recently from 16202d6 to 11cf068 Compare November 12, 2020 11:02

gumb0 requested review from chfast and axic November 12, 2020 11:09

gumb0 commented Nov 12, 2020

View reviewed changes

lib/fizzy/instantiate.hpp Outdated Show resolved Hide resolved

chfast reviewed Nov 12, 2020

View reviewed changes

axic reviewed Nov 12, 2020

View reviewed changes

gumb0 mentioned this pull request Nov 16, 2020

Fix resolving imported functions when importing the same function more than once #639

Merged

gumb0 force-pushed the resolve-imports branch 5 times, most recently from 841a720 to af4d025 Compare November 17, 2020 13:27

gumb0 mentioned this pull request Nov 17, 2020

fizzy-spectests doesn't use import resolve API #645

Open

gumb0 force-pushed the resolve-imports branch 4 times, most recently from 4996a05 to 84c6837 Compare November 18, 2020 18:08

gumb0 changed the title ~~api: resolving imports~~ api: resolving impored globals Nov 18, 2020

gumb0 force-pushed the resolve-imports branch from 84c6837 to 94fab2d Compare November 18, 2020 18:21

gumb0 commented Nov 18, 2020

View reviewed changes

gumb0 marked this pull request as ready for review November 18, 2020 18:35

gumb0 requested review from axic and chfast November 18, 2020 18:37

gumb0 force-pushed the resolve-imports branch from 874b9ba to 5a8e3a0 Compare November 24, 2020 11:54

chfast approved these changes Nov 24, 2020

View reviewed changes

axic mentioned this pull request Dec 21, 2020

capi: resolve imported globals #660

Merged

axic changed the title ~~api: resolving impored globals~~ api: resolving imported globals Jan 4, 2021

gumb0 force-pushed the resolve-imports branch 3 times, most recently from 29b4829 to 14e1a39 Compare January 5, 2021 15:26

axic reviewed Jan 27, 2021

View reviewed changes

gumb0 added 2 commits January 27, 2021 12:34

api: Pass functions by const reference to resolve_imported_functions

c32cb3d

They need to be copied, so there's no point in passing by value.

refactor: Extract finding ImportedFunction by name into helper function

a7812d2

gumb0 force-pushed the resolve-imports branch from 14e1a39 to 73938d1 Compare January 27, 2021 11:38

gumb0 added 2 commits January 27, 2021 12:43

api: Add function for resolving imported globals

9321b18

test: resolving imported globals

570e8ef

gumb0 force-pushed the resolve-imports branch from 73938d1 to 570e8ef Compare January 27, 2021 11:44

axic reviewed Jan 27, 2021

View reviewed changes

axic approved these changes Jan 27, 2021

View reviewed changes

axic merged commit db8f714 into master Jan 27, 2021

axic deleted the resolve-imports branch January 27, 2021 12:17

axic mentioned this pull request Jan 27, 2021

Alternative approach for import resolution in C API #698

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: resolving imported globals #637

api: resolving imported globals #637

gumb0 commented Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

gumb0 commented Nov 12, 2020 •

edited

Loading

chfast commented Nov 12, 2020

gumb0 commented Nov 12, 2020

chfast left a comment

chfast Nov 12, 2020

gumb0 Nov 17, 2020

gumb0 Nov 17, 2020

axic Nov 12, 2020

gumb0 Nov 16, 2020

gumb0 Nov 16, 2020

chfast Nov 17, 2020

gumb0 Nov 17, 2020

chfast Nov 17, 2020

gumb0 Nov 18, 2020

axic Jan 27, 2021

gumb0 Jan 27, 2021

gumb0 commented Jan 5, 2021

axic Jan 27, 2021

gumb0 Jan 27, 2021 •

edited

Loading

axic Jan 27, 2021

gumb0 Jan 27, 2021 •

edited

Loading

axic Jan 27, 2021 •

edited

Loading

gumb0 Jan 27, 2021

axic Jan 27, 2021

axic Jan 27, 2021

gumb0 Jan 27, 2021

axic Jan 27, 2021

gumb0 Jan 27, 2021

axic Jan 27, 2021

	const std::vector<ImportedGlobal> imported_globals_invalid_type2 = {
	const std::vector<ImportedGlobal> imported_globals_invalid_mutability = {

api: resolving imported globals #637

api: resolving imported globals #637

Conversation

gumb0 commented Nov 10, 2020 • edited Loading

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

gumb0 commented Nov 12, 2020 • edited Loading

chfast commented Nov 12, 2020

gumb0 commented Nov 12, 2020

chfast 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

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

gumb0 commented Jan 5, 2021

Choose a reason for hiding this comment

gumb0 Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

axic Jan 27, 2021 • edited Loading

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

gumb0 commented Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

gumb0 commented Nov 12, 2020 •

edited

Loading

gumb0 Jan 27, 2021 •

edited

Loading

gumb0 Jan 27, 2021 •

edited

Loading

axic Jan 27, 2021 •

edited

Loading