-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wip: allow to specify module's interface in wasm-smith #286
Conversation
@fitzgen this turned out to be a bit more compliaceted than I though, for two related reasons. First, when we generate arbitrary imports, we want to pick a type from an already existing set. So, I solved this problem by moving the bulk of the logic to the point where we generate arbitrary types, such that, at the point where we generate the import, we can basically re-use the same logic, and just pick a type from a more restricted set Second, the The PR in the current state implements the minimal amount of work to make just functions work. Opening early to get design feedback. |
Regarding the first point: I think we generally want to avoid mixing up the code paths for arbitrary imports vs available imports together too much. I think it will make everything much harder to reason about. To be specific, when we are generating imports from an available set, rather than limiting ourselves to things for which we already have a compatible type definition, we can choose an available import, add its type definition if it is not already defined, and then add the import referencing the maybe-just-defined type. Having all this in a separate code path/method from arbitrary imports should make implementation and maintenance easier for both paths. Regarding the second point:
I think the answer is "yes" here. It is annoying to type out, but that only has to happen once, and we don't expect these definitions to change frequently, so the maintenance/syncing overhead is low. This approach will provide the best user experience for folks writing custom |
This all makes sense to me, @fitzgen! Ptal, is this closer to what you have in mind? Want to get 👍 on the design before I get into modules and instances :-) One specific question:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
should I remove the import from the set of choices after it is used?
If module linking is enabled (checkable on the config), then you'll have to, because module linking disallows duplicate imports.
Additionally, with module linking, a series of two-layer imports implicitly create a one-layer import of an instance:
(import "env" "x" ...)
(import "env" "y" ...)
;; ==>
(import "env" (instance (export "x" ...) (export "y" ...)))
But this means that the following is invalid:
(import "env" "x" ...)
(import "blah" ...)
(import "env" "y" ...)
because the "env"
imports aren't back-to-back and so it desugars into multiple conflicting imports of an "env"
instance.
So after you add an import with a different module name, you need to remove any imports from the previous module name from the available set.
If module linking is not enabled, then the imports may be reused (although we can leave this as a TODO for now, if you prefer, so that you don't have to implement both code paths right now).
crates/wasm-smith/src/config.rs
Outdated
/// By default, returns `None` which means that any arbitrary import can be generated. | ||
/// | ||
/// To only allow imports from a specific set, override this to return a vec of | ||
/// `(module name, field name, entity type)` describing each available import. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// `(module name, field name, entity type)` describing each available import. | |
/// each available import. |
crates/wasm-smith/src/config.rs
Outdated
/// TODO | ||
#[non_exhaustive] | ||
#[derive(Debug)] | ||
pub enum ImportDesc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep all the publicly exposed AST types in the same module, rather than split between src/lib.rs
and src/config.rs
, so either:
- move everything to
src/lib.rs
- move everything to
src/config.rs
- or create
src/ast.rs
soft preference for the third option.
crates/wasm-smith/src/lib.rs
Outdated
let ty = self.func_type(idx).clone(); | ||
EntityType::Func(idx, ty) | ||
} | ||
None => return Ok(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we can create a type section and push it onto self.initial_sections
.
df30acf
to
3def5d7
Compare
Making some progress here! it seems to work without module linking, and module linking currently fails with a bunch of validation errors ("aliased name |
Finally got to publishing fuzzing infra we have been using in ad-hoc ways before. This just runs the contracts to make sure that VM doesn't crash. The code is setup to easily compare execution with different VM Kinds. Note that at the moment fuzzing *does not* generally exercise host function calls. I need to finish bytecodealliance/wasm-tools#286 for that 😅
This can be closed now. |
fix #276