Update wasm-smith for recent changes to module-linking#195
Update wasm-smith for recent changes to module-linking#195alexcrichton merged 6 commits intobytecodealliance:mainfrom
Conversation
alexcrichton
commented
Jan 11, 2021
alexcrichton
commented
- Implement exporting modules/instances
- Implement implicit instances from two-level imports
- Update encoding of instance arguments
- Implement outer aliases to types/modules
If they exist, they're candidates for export!
I almost always do this anyway so it's nice to have it already done.
This updates the indexing of the instance index space in wasm-smith to account for the fact that imports from a two-level namespace implicitly generate an instance import. Additionally the encoding of instance arguments is updated to not have a 2-level namespace, but just a single-level namespace.
This commit fixes a TODO tto update wasm-smith to generate modules with outer aliases for types/modules.
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
LGTM modulo a bunch of little nitpicks -- sorry I am a stickler for capitalizing and punctuating code comments!
| let ity = import_types.entry(module.clone()).or_insert_with(|| { | ||
| EntityType::Instance(u32::max_value(), Default::default()) | ||
| }); |
There was a problem hiding this comment.
Is using the entry API to avoid double hash lookups (but needing to clone+allocate a string in this case) faster than doing the double hash lookup and avoiding a string clone+allocation in this case? I guess it will most often be the case that the entry is missing...
There was a problem hiding this comment.
In general there's a fair amount of cloning happening everywhere in wasm-smith wrt the module structure and module linking, so I'm not too worried about this specifically. The default limits in place and SwarmConfig I think makes it so we don't have to worry about this too too much. That being said though I'm mostly relying on oss-fuzz's timeouts to tell us where we need to optimize here, because there's definitely more than one place we can optimize in this file!
For now though I think I'll leave this as-is because I think it's the most readable, but if it becomes problematic I'll probably refactor this out so it's not all in one big blob.
|
Heh no worries, it's high time someone calls out my sloppy comments anyway :) |
I today started looking at updating this project to the new draft of the canonical ABI specified at WebAssembly/component-model#23 and while there's quite a few changes that need to happen the first thing that struck me was that maintaining all of the support for old-style witx files is pretty onerous. Especially the ABI-calculating code has lots of special cases for various witx constructs and the Preview1 ABI, and I wasn't really sure how to update this in many situations. Overall the original purpose of the witx support was to prove out that it's possible to support both the old witx abi and the new canonical ABI in the same generator. The canonical ABI has changed significantly in the meantime however and this doesn't necessarily make sense to do any more. I think it would be best now to reevaluate at the point when WASI is ready to switch to the component model what to do with the old witx support. I no longer think that "build it in here" is the obvious option. As this diff shows there's quite a bit of weight to carry the old witx abis as this commit clocks in at nearly 7000 lines removed. The specifics being dropped here are: * Parsing support for `*.witx` * Support for `Pointer` and `ConstPointer` * Support for `WitxInstruction` * Support for push/pull buffers The push/pull buffer feature was never actually fully implemented, even for Rust hosts they only kind-of worked. Most other generators never even implemented support for them. Additionally support for other `*.witx` constructs was somewhat spotty at best with very few tests. My hope is that there are no existing users of this support. If there are then I think it's best to re-evaluate how best to solve the scenario on-hand.
Further iteration from bytecodealliance#195 and dropping more features that are only needed for `*.witx`, the `usize` and `char8` types were only added for backcompat with witx itself.
2 participants
