Fix resolving imported functions when importing the same function more than once #639
Conversation
| @@ -408,8 +408,7 @@ std::vector<ExternalFunction> resolve_imported_functions( | |||
| " output type doesn't match imported function in module"}; | |||
| } | |||
|
|
|||
| external_functions.emplace_back( | |||
| ExternalFunction{std::move(it->function), module_func_type}); | |||
| external_functions.emplace_back(ExternalFunction{it->function, module_func_type}); | |||
There was a problem hiding this comment.
Now input imported_functions could be passed by const reference, not sure it's worth to change.
Codecov Report
@@ Coverage Diff @@
## master #639 +/- ##
=======================================
Coverage 98.37% 98.37%
=======================================
Files 69 69
Lines 9658 9682 +24
=======================================
+ Hits 9501 9525 +24
Misses 157 157 |
| { | ||
| /* wat2wasm | ||
| (func (import "mod1" "foo1") (param i32) (result i32)) | ||
| (func (import "mod1" "foo1") (param i32) (result i32)) |
There was a problem hiding this comment.
Hmm, are there such test cases in spectests?
There was a problem hiding this comment.
Also can such a test be added to the C++ part as well?
There was a problem hiding this comment.
Hmm, are there such test cases in spectests?
I found at least this one case of importing the same function twice:
https://github.com/wasmx/wasm-spec/blob/7526564b56c30250b66504fe795e9c1e88a938af/test/core/imports.wast#L30-L33
Our spec test runner doesn't use resolve function, so it wouldn't be affected by this.
There was a problem hiding this comment.
Also there's this curious case of importing overloaded functions with different types, which is expected to be valid but fail to instantiate:
https://github.com/wasmx/wasm-spec/blob/7526564b56c30250b66504fe795e9c1e88a938af/test/core/imports.wast#L568
Perhaps resolve should allow several functions with the same name and different types, too. I would fix this in another PR.
There was a problem hiding this comment.
Also can such a test be added to the C++ part as well?
I assume you mean to C API part, because this is C++ API test.
There was a problem hiding this comment.
Also can such a test be added to the C++ part as well?
I assume you mean to C API part, because this is C++ API test.
Ah you do seem to have it in both.
There was a problem hiding this comment.
Our spec test runner doesn't use resolve function, so it wouldn't be affected by this.
Hm, should it?
There was a problem hiding this comment.
Our spec test runner doesn't use resolve function, so it wouldn't be affected by this.
Hm, should it?
I think maybe not necessary, and in current form it would be really inconvenient.
To me it seems that resolve API suits more to import host functions - because of convenience of flattened ImportedFunction struct and other Imported* from #637. Whereas importing from one module to another requires External* structs - which are returned from find_exported_* functions - that's what spectest runner uses.
There was a problem hiding this comment.
Also there's this curious case of importing overloaded functions with different types, which is expected to be valid but fail to instantiate:
https://github.com/wasmx/wasm-spec/blob/7526564b56c30250b66504fe795e9c1e88a938af/test/core/imports.wast#L568Perhaps
resolveshould allow several functions with the same name and different types, too. I would fix this in another PR.
Also an issue for this #647
gumb0
force-pushed
the
resolve-imported-function-twice
branch
from
0c5d63c to
cc52b35
Compare
No description provided.