Add validations for imports during instantiation #8086
Conversation
stevenfontanella
force-pushed
the
import-validation
branch
from
4c82930 to
9c0a77c
Compare
stevenfontanella
force-pushed
the
multithread
branch
from
44c5834 to
62b613b
Compare
stevenfontanella
force-pushed
the
import-validation
branch
from
9c0a77c to
760c35a
Compare
stevenfontanella
force-pushed
the
multithread
branch
from
62b613b to
bb0d612
Compare
stevenfontanella
force-pushed
the
import-validation
branch
2 times, most recently
from
3920f0c to
766d753
Compare
stevenfontanella
force-pushed
the
import-validation
branch
6 times, most recently
from
1f212af to
1368f6a
Compare
stevenfontanella
force-pushed
the
import-validation
branch
from
1368f6a to
cb635ed
Compare
stevenfontanella
force-pushed
the
import-validation
branch
from
cb635ed to
35d3f13
Compare
stevenfontanella
force-pushed
the
import-validation
branch
from
35d3f13 to
abb3c33
Compare
stevenfontanella
force-pushed
the
import-validation
branch
from
abb3c33 to
08cdd58
Compare
|
Will run this through the fuzzer as well before merging |
src/shell-interface.h
Outdated
| trap((std::stringstream() | ||
| << "importGlobals: unknown import: " << import->module.str << "." | ||
| << import->name.str) | ||
| .str()); |
There was a problem hiding this comment.
I did not know about this nice pattern for stringstream 😄
src/wasm-interpreter.h
Outdated
|
|
||
| virtual void trap(const char* why) { WASM_UNREACHABLE("unimp"); } | ||
|
|
||
| virtual void trap(std::string_view why) { WASM_UNREACHABLE("unimp"); } |
There was a problem hiding this comment.
Do we really need two trap forms? This does not need to be fast. How about making them both std::string?
There was a problem hiding this comment.
If we remove the const char* overload then we'd need to change it in a few other places that override it e.g. in ConstantExpressionRunner link, so for this PR I was just going to introduce the "better" overload without removing the old one right away. I mostly just want to avoid C-strings and I think string_view is the best parameter as long as we're changing it. I'll move this to a new PR instead.
ctor-eval doesn't have access to imports and previously didn't have any special handling for this. Globals would fail to import (https://github.com/WebAssembly/binaryen/blob/main/src/tools/wasm-ctor-eval.cpp#L247), which would prevent further evaluation (https://github.com/WebAssembly/binaryen/blob/main/src/tools/wasm-ctor-eval.cpp#L1451). This is why global-get-init.wast didn't optimize away even though it does nothing. We already stubbed out imports to "env". Change the code to create a stub module for all modules named in imports, so that instantiation is valid and evaluation can continue. This also unblocks #8086 which checks imports and requires them to exist during instantiation.
| (i32.const 5678) | ||
| ) | ||
| (type $0 (func)) | ||
| (import "env" "func" (func $fimport$0)) |
There was a problem hiding this comment.
Good point, this looks wrong. I might have added this while I tested it and forgot to revert. I'm guessing something traps during instantiation in this test because of the import which doesn't exist as far as wasm-reduce knows. Taking a look now.
exact-func-import.wastfrom the testsuite repo, which now passes.ref_func.wastandtags.wastwhich also otherwise fail with the new validations.imports0,imports2,linking0,linking3and partially fixesimports,imports3andmemory64.As followups, we should improve the handling of the "spectest" module by implementing the implicit definition described by the spec, and we should add tracking for exact types of function re-exports so that we can enable import-time type checking for function types.