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.
src/wasm.h
Outdated
| return true; | ||
| } | ||
|
|
||
| bool isSubType(const Table& other) { return Table::isSubType(*this, other); } |
There was a problem hiding this comment.
I would prefer to put logic like this in ir/, so that we can keep wasm.h shorter. This might fit in memory-utils.h, table-utils.h etc?
There was a problem hiding this comment.
Could we split out the Memory class to its own file in that case? I'm thinking it's good to avoid too many free functions especially since we have to differentiate them by name e.g. MemoryIsSubType, TableIsSubType, etc. The memory class seems like the right place to keep this function.
There was a problem hiding this comment.
Ah, I see where you're coming from, but it would be a large change in our style.
Our current approach is that the core IR is in very simple classes, all in wasm.h. We do have methods on those classes, but just very basic things that are useful universally. And we make an effort to split out things that are useful in more narrow contexts, to ir/*-utils.h and other helpers.
So concretely, we already have e.g. MemoryUtils::flatten(), and I think we can add MemoryUtils::isSubType() alongside that?
There was a problem hiding this comment.
Oh, and to explain the current style, the main motivation is to keep wasm.h small, and other headers that are used universally. That makes it easier to read wasm.h etc. and get a general idea of the shape of things, and also helps compile times.
There was a problem hiding this comment.
Got it, thanks for the explanation! Moved them to the utils headers.
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.
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.