Rec struct #11605
Rec struct #11605
Conversation
khagankhan
requested review from
alexcrichton
and removed request for
a team
| }; | ||
|
|
||
| const NUM_PARAMS_RANGE: RangeInclusive<u32> = 0..=10; | ||
| const NUM_TYPES_RANGE: RangeInclusive<u32> = 0..=32; |
There was a problem hiding this comment.
new ranges for Types
| vec![ValType::EXTERNREF, ValType::EXTERNREF, ValType::EXTERNREF], | ||
| ); | ||
|
|
||
| let groups = self.limits.num_rec_groups as usize; |
There was a problem hiding this comment.
Here I tried to distribute structs over different recs.
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
requested review from
fitzgen
and removed request for
alexcrichton
There was a problem hiding this comment.
Good start, although I think we need some tweaks before this can land.
One thing I don't see is any mutators for our types and rec groups (e.g. add/remove types, switch a type from one rec group to another, etc). Is the plan that we will add mutators in follow up PRs? That is fine, but I just want to make sure it isn't overlooked.
Thanks!
| let groups = self.limits.num_rec_groups as usize; | ||
| let total = self.limits.num_types as usize; | ||
|
|
||
| if groups == 0 { | ||
| // Nothing to emit. |
There was a problem hiding this comment.
Instead of num_{rec_groups,types} can these be max_{rec_groups,types}? We already have an (implicit) count of rec groups and types via the size of the Types::{type_defs,rec_groups} fields. Then we would define a Types::fixup method that takes a reference to the limits and removes any entries beyond the given limit, and finally we would call Types::fixup from TableOps::fixup:
impl Types {
// ...
fn fixup(&mut self, limits: &TableOpsLimits) {
while self.rec_groups.len() > limits.max_rec_groups {
self.rec_groups.pop_last();
}
while self.type_defs.len() > limits.max_types {
self.type_defs.pop_last();
}
}
}This ensures that we always have a valid number of types and rec groups by the time we get to emitting a Wasm binary, which is similar to how we treat everything else (assume/assert it is valid in to_wasm_binary, enforce validity in fixup).
(Aside: eventually we will probably want custom serialization/deserialization with #[serde(serialize_with = "my_serialization_func")] for all of our container types so that we never try to allocate a huge Vec/BTreeSet/BTreeMap during deserialization of untrusted fuzzer data, leading to OOMs.)
| // Distribute total types across groups: q each, first r get +1. | ||
| let q = total / groups; | ||
| let r = total % groups; |
There was a problem hiding this comment.
We specifically do not want to evenly distribute types between rec groups. We want to explore the weird edge cases where all types are in one rec group and all other rec groups are empty, etc. Also, we already have a mapping from type to rec group in the form of the SubType::rec_group field and we should use that mapping, not ignore it.
Instead, I would expect something like this:
// Gather all rec groups and types by rec group.
let mut rec_groups = self.types
.rec_groups
.iter()
.map(|id| (id, vec![]))
.collect::<BTreeMap<RecGroupId, Vec<TypeId>>();
for (id, ty) in self.types.type_defs.iter() {
rec_groups.entry(ty.rec_group).or_default().push(id);
}
// Encode those rec groups and their types into the types section.
for type_ids in rec_groups.values() {
types.ty().rec(
type_ids
.iter()
.map(|ty_id| todo!("create a `wasm_encoder::SubType` for the type with id `ty_id`"))
.collect::<Vec<_>>(),
);
}| /// Add an empty struct type to a given group | ||
| pub fn insert_empty_struct(&mut self, id: TypeId, group: RecGroupId) { | ||
| self.type_defs.insert( | ||
| id, | ||
| SubType { | ||
| rec_group: group, | ||
| composite_type: CompositeType::Struct(StructType::default()), | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| /// Return true if there is at least one struct type defined. | ||
| pub fn has_structs(&self) -> bool { | ||
| self.type_defs | ||
| .values() | ||
| .any(|t| matches!(t.composite_type, CompositeType::Struct(_))) | ||
| } | ||
|
|
||
| /// Iterate over the IDs of all struct type definitions. | ||
| pub fn struct_ids(&self) -> impl Iterator<Item = &TypeId> { | ||
| self.type_defs | ||
| .iter() | ||
| .filter(|(_, t)| matches!(t.composite_type, CompositeType::Struct(_))) | ||
| .map(|(id, _)| id) | ||
| } |
There was a problem hiding this comment.
None of these methods seem to be used?
|
Thanks for the comments @fitzgen! Yes mutators are coming next. I just want to go step by step so addressing them is easy |
| use super::*; | ||
|
|
||
| use rand::rngs::StdRng; | ||
| use rand::{Rng, SeedableRng}; |
There was a problem hiding this comment.
Style nitpick: newline between imports and declarations
| let encode_ty_id = |ty_id: &TypeId| -> wasm_encoder::SubType { | ||
| let def = &self.types.type_defs[ty_id]; | ||
| match &def.composite_type { | ||
| CompositeType::Struct(_s) => wasm_encoder::SubType { |
There was a problem hiding this comment.
Nitpick: lets exhaustively match on the struct definition here so that as we expand that type we are reminded to update this code by the compiler:
| CompositeType::Struct(_s) => wasm_encoder::SubType { | |
| CompositeType::Struct(StructType {}) => wasm_encoder::SubType { |
|
cc @eeide |
Open to review
cc @fitzgen