Extend re-encode functionality to support components #1656
Conversation
itowlson
force-pushed
the
component-reencode
branch
from
16de3fb to
2662e16
Compare
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
itowlson
force-pushed
the
component-reencode
branch
from
2662e16 to
8d8eefc
Compare
crates/wasm-encoder/src/reencode.rs
Outdated
| instance_index, | ||
| name, | ||
| } => crate::Alias::CoreInstanceExport { | ||
| instance: instance_index, |
There was a problem hiding this comment.
I had some similar comments on #1655, but for indices like this could you also add a methods such as component_instance_index which default is a noop but allows the reencode trait to remap the indices if desired?
There was a problem hiding this comment.
How sophisticated should this be? For example, in the case of wasmparser::ComponentAlias::Outer, the index seems like it refers to a different type of item depending on kind (and, per count, in a different scope, if that would affect the transformation) - should I therefore pass kind and count into the remapping function? Sorry, I'm flying a bit blind here in terms of familiarity with the underlying concepts...!
Another example is cases such as TypeBounds where we are dealing with enums of indexes. E.g. for TypeBounds the reencode logic is:
match bounds {
wasmparser::TypeBounds::Eq(index) => crate::TypeBounds::Eq(index),
wasmparser::TypeBounds::SubResource => crate::TypeBounds::SubResource,
}
Do I need to offer a separate hook to remap the index in cases like this, or would a consumer override the entire logic?
There was a problem hiding this comment.
For exports/aliases I'm thinking something like this which would take the kind into account to determine which index function to call. Otherwise you're right that this probably won't be too too useful in general due to how components can reach upwards, but I'm thinking there should be a foo_index method for each index space within a component but no need to take extra arguments like depth, just the index is fine.
For the TypeBounds case what I'm thinking is that for the index you'd call reencoder.component_type_index(index). Definitely no need for a hook-per-use-site, my thinking is basically being able to get a "typed" view of all the indices within the component.
crates/wasm-encoder/src/reencode.rs
Outdated
| let mut is = crate::InstanceSection::new(); | ||
| for inst in section { | ||
| match inst? { | ||
| wasmparser::Instance::Instantiate { module_index, args } => { | ||
| let aa = args | ||
| .iter() | ||
| .map(|a| (a.name, crate::ModuleArg::Instance(a.index))); | ||
| is.instantiate(module_index, aa); | ||
| } | ||
| wasmparser::Instance::FromExports(exports) => { | ||
| let ee = exports | ||
| .iter() | ||
| .map(|e| (e.name, reencoder.export_kind(e.kind), e.index)); | ||
| is.export_items(ee); | ||
| } | ||
| }; | ||
| } | ||
| component.section(&is); |
There was a problem hiding this comment.
Mind splitting these section handlers into components such as:
- A
parse_instance_sectionmethod which takes the wasmparser-based reader - A
Fromand conversion method such asparse_instancefor each item in the section
That'll help keep these sorts of impls a bit neater and additionally enable implementors of Reencode to be able to customize the smallest portion available (and or call the "biggest" portion they want)
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
itowlson
force-pushed
the
component-reencode
branch
from
60ba05f to
a721450
Compare
|
Thank you for working on this! I feel like the module and trait are becoming a bit big. Perhaps the core module and component functionalities in the untold module could be split into two submodules? If all methods go into the same trait, having a consistent prefix for core vs component functions might be useful. If they could be split, would a subtrait for component encoding make sense? |
|
Thanks @juntyr - there may be something to be said for that. Right now, however, I'm not sure if I'm going to be pushing this to completion, as I'm now looking at a different approach to the problem I was trying to solve. I'm not yet ready to close it for definite, though, and if I do pick it up again I'll explore that suggestion for sure! |
This commit is an extension of bytecodealliance#1656 to support components in the reencoding support of wasm-encoder. The precise shape of the trait will probably change over time but this is at least a first stab at getting things working to map all the wasmparser types to wasm-encoder for components. All existing support for components in `reencode.rs` was moved to a new `ReencodeComponent` trait which inherits from the already-existing `Reencode` trait. This is all located in a new `component.rs` file beneath `reencode.rs` although crate-wise it's all showing up in the same `wasm_encoder::reencode` module.
This commit is an extension of bytecodealliance#1656 to support components in the reencoding support of wasm-encoder. The precise shape of the trait will probably change over time but this is at least a first stab at getting things working to map all the wasmparser types to wasm-encoder for components. All existing support for components in `reencode.rs` was moved to a new `ReencodeComponent` trait which inherits from the already-existing `Reencode` trait. This is all located in a new `component.rs` file beneath `reencode.rs` although crate-wise it's all showing up in the same `wasm_encoder::reencode` module.
|
I'm attempting to help push this over the finish line in #1722 |
* Extend re-encode functionality to support components Signed-off-by: itowlson <ivan.towlson@fermyon.com> * WIP: indexes, reorg Signed-off-by: itowlson <ivan.towlson@fermyon.com> * Finish reencoding support for components to wasm-encoder This commit is an extension of #1656 to support components in the reencoding support of wasm-encoder. The precise shape of the trait will probably change over time but this is at least a first stab at getting things working to map all the wasmparser types to wasm-encoder for components. All existing support for components in `reencode.rs` was moved to a new `ReencodeComponent` trait which inherits from the already-existing `Reencode` trait. This is all located in a new `component.rs` file beneath `reencode.rs` although crate-wise it's all showing up in the same `wasm_encoder::reencode` module. * Fix compat with Rust 1.78 --------- Signed-off-by: itowlson <ivan.towlson@fermyon.com> Co-authored-by: itowlson <ivan.towlson@fermyon.com>
This is WIP at the moment (worked in one simple test case but has certainly not been properly tested!).