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 |
| 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 😄
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.
test/reduce/imports.wast.txt
Outdated
| (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.
There was a problem hiding this comment.
To close here, the diff here was because with the import validation, the un-reduced module was trapping on instantiation because the import doesn't exist, so the reducer was able to reduce it to a trivial module that imports (and traps). I guess this is bad because in the fuzzer we generally want to ignore imports and possibly even assume a convenient value for imports (e.g. imported functions are assumed to be no-op and can be optimized out link).
To fix this and ctor-eval, I changed this PR to only enable import validation for wasm-shell / spec tests. Once we have some improvements to the way that importing logic is handled, we can remove this flag and run the validation logic unconditionally.
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.