utils: add signature to WasmEngine::find_function #331

axic · 2020-05-22T13:51:59Z

This is required for wamr support. Also it can help us avoid mistakes if we validate signatures in the engines.

Should also add signature type validation to find_function. That helps with both this PR and #329.

axic · 2020-05-22T13:57:23Z

test/bench/bench.cpp

+                std::cout << l << std::endl;
+                if (l.find_first_of(":") != l.find_last_of(":"))
+                    throw std::runtime_error{"Multiple occurances of ':' found in signature"};
+                // Only allow i (i32) I (i64) v (void) as types


Need to decide on a design here, the main question is about return types and no arguments cases:
a) Require colon and void type, e.g. i:v, i:i
b) Allow empty return type, no need for void, e.g. i:
c) Omit colon if no return type, e.g. i
d) Require void on input side if there no inputs, but there's an output, e.g. v:i
e) Allow empty arguments, e.g. :i

I'm not sure what is the best, but leaning towards c) and e).

cc @chfast @gumb0

I like a) because everything is explicit and easier to parse

In the case of a), what happens when there are no inputs? :v and :i ?

If this is based on WAMR I would use what they have.

I would not use v at all to be consistent with wasm types, where it is [].

I would keep : always for parsing. So : is also valid.

It is not based on WAMR. Mentioned it here before: #318 (comment)

wasm3 uses a version of this, but has a reverse notation where return value is first (e.g. v(ii))

emscripten uses a version of this: it has no separation of input vs output (e.g. iiv), but rather assumes the last type is either a type or void

wamr uses a version of this, but unclear what they do for the return value

@chfast are you saying these rules then:

: no input, no output

i: single i32 input, no output

:i single i32 output, no input

etc.

Yes. But if this is something different than anything else, I don't care so much.

We may want to improve/change/use it later on in the external API, then we can come back. All we need right now is just to make the benchmark tool more convenient to use, to support wamr, and to make fizzy-wasi nicer. Any of the solutions accomplish that, but prefer to take the easiest-to-parse route than anything else now.

In the case of a), what happens when there are no inputs? :v and :i ?

v:v, v:i
I guess it's d) or a) + d)

axic · 2020-05-22T14:02:46Z

test/bench/bench.cpp

+                st = InputsReadingState::FuncSignature;
+                break;
+
+            case InputsReadingState::FuncSignature:


@chfast suggested to conflate the signature into the name line, such as keccak256_bnech iii:i

That would be nicer for reading input files, but I don't insist.

axic · 2020-05-22T20:53:22Z

test/utils/fizzy_engine.cpp

 {
-    return fizzy::find_exported_function(m_instance->module, name);
+bool matching_signature(const fizzy::FuncType& func_type, std::string_view signature)


Probably would be nicer to have simple translator instead (sig -> func_type) and equal operator for func_type. Update: we actually already have the equal operator.

test/bench/bench.cpp

test/utils/fizzy_engine.cpp

gumb0 · 2020-05-25T15:15:25Z

test/utils/wasm_engine.hpp

@@ -55,6 +56,17 @@ class WasmEngine
    /// Executes the function of the given index.
    /// Requires instantiate().
    virtual Result execute(FuncRef func_ref, const std::vector<uint64_t>& args) = 0;
+
+    static void validate_function_signature(std::string_view signature)


I would prefer to make it a free function in fizzy::test, but not so important

Would that mean to make inline in the header or creating yet another file, wasm_engine.cpp, to place it?

@chfast any preference?

a) as it is now
b) as a free function in the header
c) as a free function in a new .cpp file

For sanity, it is preferable to move implementation to .cpp file.
For static method / free function I think both options are fine.

gumb0 · 2020-05-25T15:16:12Z

Do you plan to add this validation to all of the engines?

axic · 2020-05-25T15:18:21Z

Do you plan to add this validation to all of the engines?

Initially I thought so, but I think it is fine to only do it in fizzy (it detects problems in all our test cases) and only do the conversion for a given engine when there's need (such as wamr would depend on it for implementation).

codecov · 2020-05-30T15:31:30Z

Codecov Report

Merging #331 into master will decrease coverage by 0.00%.
The diff coverage is 97.72%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
- Coverage   98.86%   98.85%   -0.01%     
==========================================
  Files          40       41       +1     
  Lines       11779    11854      +75     
==========================================
+ Hits        11645    11718      +73     
- Misses        134      136       +2

axic · 2020-05-31T10:29:09Z

Squashed this down quite a bit.

test/utils/fizzy_engine.cpp

axic · 2020-06-02T07:36:26Z

test/utils/wasm_engine.hpp

@@ -37,7 +37,8 @@ class WasmEngine

    /// Finds an exported function in the internal instance.
    /// Requires instantiate().
-    virtual std::optional<FuncRef> find_function(std::string_view name) const = 0;
+    virtual std::optional<FuncRef> find_function(
+        std::string_view name, std::string_view signature) const = 0;


Should this be signature or sig?

signature.

chfast · 2020-06-02T08:12:05Z

test/utils/wasm_engine.cpp

@@ -0,0 +1,20 @@
+// Fizzy: A fast WebAssembly interpreter
+// Copyright 2019-2020 The Fizzy Authors.


Suggested change

// Copyright 2019-2020 The Fizzy Authors.

// Copyright 2020 The Fizzy Authors.

Also introduce validate_function_signature helper.

And validate its format.

axic mentioned this pull request May 22, 2020

Add wamr to benchmarking #327

Closed

axic commented May 22, 2020

View reviewed changes

axic force-pushed the func-sig branch from a013674 to adce478 Compare May 22, 2020 17:46

axic commented May 22, 2020

View reviewed changes

axic changed the title ~~Utils: add signature to WasmEngine::find_function~~ utils: add signature to WasmEngine::find_function May 25, 2020

axic force-pushed the func-sig branch from 8c22a96 to 18c3f82 Compare May 25, 2020 13:28

axic marked this pull request as ready for review May 25, 2020 13:30

axic commented May 25, 2020

View reviewed changes

test/bench/bench.cpp Outdated Show resolved Hide resolved

axic requested review from gumb0 and chfast May 25, 2020 13:37

gumb0 reviewed May 25, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 25, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 25, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 25, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 25, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 25, 2020

View reviewed changes

axic force-pushed the func-sig branch from 5311b5f to d9b7092 Compare May 25, 2020 15:24

axic force-pushed the func-sig branch 2 times, most recently from 099da55 to abee111 Compare May 28, 2020 22:45

axic mentioned this pull request May 29, 2020

refactor: move FuncType comparisons operators to types.hpp #360

Merged

axic force-pushed the func-sig branch from abee111 to 47ef88c Compare May 30, 2020 15:22

axic force-pushed the func-sig branch 4 times, most recently from 3d7c85f to 1739212 Compare May 31, 2020 09:55

axic requested a review from gumb0 May 31, 2020 10:28

axic force-pushed the func-sig branch from 1739212 to 5481fe7 Compare May 31, 2020 10:28

axic mentioned this pull request Jun 1, 2020

bench: Fix execution pre-validation #340

Merged

axic commented Jun 1, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

chfast approved these changes Jun 1, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

axic requested a review from chfast June 1, 2020 20:47

chfast approved these changes Jun 1, 2020

View reviewed changes

axic commented Jun 2, 2020

View reviewed changes

axic force-pushed the func-sig branch from 8978a73 to 7bf187a Compare June 2, 2020 07:44

axic requested a review from chfast June 2, 2020 07:47

axic force-pushed the func-sig branch from cb98be6 to b8d2980 Compare June 2, 2020 08:03

chfast approved these changes Jun 2, 2020

View reviewed changes

axic force-pushed the func-sig branch from b8d2980 to c4fb625 Compare June 2, 2020 08:13

chfast approved these changes Jun 2, 2020

View reviewed changes

axic added 3 commits June 2, 2020 09:23

utils: add signature to WasmEngine::find_function

99302d1

Also introduce validate_function_signature helper.

bench: require function signature in inputs

68a6480

And validate its format.

benchmarks: add function signatures

606da0a

axic force-pushed the func-sig branch from c4fb625 to 2c2d0ec Compare June 2, 2020 08:23

tests: implement sigature verification in fizzy_engine

83247e3

axic force-pushed the func-sig branch from 2c2d0ec to 83247e3 Compare June 2, 2020 08:34

axic merged commit 7c45309 into master Jun 2, 2020

axic deleted the func-sig branch June 2, 2020 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utils: add signature to WasmEngine::find_function #331

utils: add signature to WasmEngine::find_function #331

axic commented May 22, 2020 •

edited

Loading

axic May 22, 2020

gumb0 May 22, 2020

axic May 22, 2020

chfast May 22, 2020

axic May 22, 2020

axic May 22, 2020

chfast May 22, 2020

axic May 22, 2020 •

edited

Loading

gumb0 May 22, 2020

axic May 22, 2020

chfast Jun 1, 2020

axic May 22, 2020 •

edited

Loading

gumb0 May 25, 2020

axic May 30, 2020

axic Jun 1, 2020

chfast Jun 2, 2020

gumb0 commented May 25, 2020

axic commented May 25, 2020

codecov bot commented May 30, 2020 •

edited

Loading

axic commented May 31, 2020

axic Jun 2, 2020

chfast Jun 2, 2020

chfast Jun 2, 2020

		@@ -0,0 +1,20 @@
		// Fizzy: A fast WebAssembly interpreter
		// Copyright 2019-2020 The Fizzy Authors.

	// Copyright 2019-2020 The Fizzy Authors.
	// Copyright 2020 The Fizzy Authors.

utils: add signature to WasmEngine::find_function #331

utils: add signature to WasmEngine::find_function #331

Conversation

axic commented May 22, 2020 • 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

axic May 22, 2020 • 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

axic May 22, 2020 • 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

gumb0 commented May 25, 2020

axic commented May 25, 2020

codecov bot commented May 30, 2020 • edited Loading

Codecov Report

axic commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented May 22, 2020 •

edited

Loading

axic May 22, 2020 •

edited

Loading

axic May 22, 2020 •

edited

Loading

codecov bot commented May 30, 2020 •

edited

Loading