`wasm-smith` does not support `--available-imports` with GC types

[mobsceneZ]

Hi developers, I try to leverage wasm-smith to generate random Wasm modules with the specified imports and exports. However, when passing the following Wasm module to the --available-imports option, wasm-smith (commit 4355604) panicked:

thread 'main' panicked at crates/wasm-smith/src/core.rs:1309:37:
could not parse type section: BinaryReaderError { inner: BinaryReaderErrorInner { message: "gc proposal not supported", kind: Custom, offset: 11, needed_hint: None } }
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

(module
  (rec
    (type (sub final (array (mut i32))))
    (type (sub final (func (param i32) (result i32))))
  )
  (import "a" "f1" (func (type 1)))
  (import "a" "m1" (memory (;0;) 43153 57698))
)

The provided module itself is valid, thus I manually checked out the panicked location. As it turns out, the into_iter_err_on_gc_types() public function of the wasmparser crate assumes each recursive group contains exactly one function type, which IMO makes no sense for wasm-smith since GC proposal is now standardized:

wasm-tools/crates/wasmparser/src/readers/core/types.rs

Lines 1952 to 1973 in 4355604

	pub fn into_iter_err_on_gc_types(self) -> impl Iterator<Item = Result<FuncType>> + 'a {
	self.into_iter_with_offsets().map(|item| {
	let (offset, group) = item?;
	let mut types = group.into_types();
	let ty = match (types.next(), types.next()) {
	(Some(ty), None) => ty,
	_ => bail!(offset, "gc proposal not supported"),
	};
	if !ty.is_final || ty.supertype_idx.is_some() {
	bail!(offset, "gc proposal not supported");
	}
	match ty.composite_type.inner {
	CompositeInnerType::Func(f) => Ok(f),
	CompositeInnerType::Array(_) | CompositeInnerType::Struct(_) => {
	bail!(offset, "gc proposal not supported");
	}
	CompositeInnerType::Cont(_) => {
	bail!(offset, "stack switching proposal not supported");
	}
	}
	})
	}

I think maybe we should update the related code logic for wasm-smith crate to better support the GC proposal? If you think this is appropriate but don't have time to implement these changes yet, I can submit a PR.

[mobsceneZ]

Besides, it looks like the support for generating exports for tag, memory, and table is still missing, maybe wasm-smith can directly copy the corresponding elements from the user-provided module 🤔?

[alexcrichton]

Thanks! This is a "correct panic" in the sense that it's correctly indicating that this isn't implemented yet, but otherwise is also very much indeed a bug and worth fixing. I believe what's necessary here is that the original module's types will have to be reflected into the wasm_smith::Module being constructed, so sort of converting wasmparser to wasm-smith/wasm-encoder. That shouldn't be too too hard but will likely be a bit nontrivial.

If you're up for sending a PR that'd be most welcome! And supporting other types like tag/memory/table makes sense to me too

[mobsceneZ]

Thanks! This is a "correct panic" in the sense that it's correctly indicating that this isn't implemented yet, but otherwise is also very much indeed a bug and worth fixing. I believe what's necessary here is that the original module's types will have to be reflected into the wasm_smith::Module being constructed, so sort of converting wasmparser to wasm-smith/wasm-encoder. That shouldn't be too too hard but will likely be a bit nontrivial.

If you're up for sending a PR that'd be most welcome! And supporting other types like tag/memory/table makes sense to me too

Thanks for your quick and kind response and I'm happy to submit a PR! But I don't quite understand the meaning of "so sort of converting wasmparser to wasm-smith/wasm-encoder", can you clarify this a bit?

[alexcrichton]

Processing of --available-imports happens here and effectively needs to translate the input types to the data structures that wasm-smith maintains here. For example when parsing GC types you'd have to push all type information into the wasm-smith module as-is which would involve converting from the wasmparser representation to the wasm-smith representation. The wasm-smith crate pretty heavily uses wasm-encoder so such a conversion would likely be able to use the Reencode trait.

[mobsceneZ]

Hi @alexcrichton, I'm currently working on adding support for the export specifications of tag, memory, and table in wasm-smith. While the implementation itself is quite straightforward, I ran into some issues when testing the added functionalities. I'm not sure whether they're related to my implementation or the wasm-tools, so I post these issues here and would like to get your thoughts on them:

(1) The entity_type_from_export() function inside crates/wasmparser/src/validator/types.rs is responsible for retrieving the actual type information of the passed-in export object. However, its implementation for tag kind is confusing, which queries the functions field instead of the tags field, leading to an assertion failure when testing.

wasm-tools/crates/wasmparser/src/validator/types.rs

Lines 558 to 580 in 375d1ea

	pub fn entity_type_from_export(&self, export: &Export) -> Option<EntityType> {
	match &self.kind {
	TypesRefKind::Module(module) => Some(match export.kind {
	ExternalKind::Func => EntityType::Func(
	module.types[*module.functions.get(export.index as usize)? as usize],
	),
	ExternalKind::Table => {
	EntityType::Table(*module.tables.get(export.index as usize)?)
	}
	ExternalKind::Memory => {
	EntityType::Memory(*module.memories.get(export.index as usize)?)
	}
	ExternalKind::Global => {
	EntityType::Global(*module.globals.get(export.index as usize)?)
	}
	ExternalKind::Tag => EntityType::Tag(
	module.types[*module.functions.get(export.index as usize)? as usize],
	),
	}),
	#[cfg(feature = "component-model")]
	TypesRefKind::Component(_) => None,
	}
	}

(2) wasm-smith accidentally generates a Wasm module that fails the validation, which, after minimization, looks like the following:

(module
  (global (ref null extern) ref.null extern)
  (table 5 10 (ref null extern) global.get 0)
)

As per the specification, Constant expressions occurring in tables may only have global.get instructions that refer to imported globals. Thus, I wonder if the following code logic inside wasm-smith needs adjustment:

wasm-tools/crates/wasm-smith/src/core.rs

Lines 1705 to 1714 in 375d1ea

	fn arbitrary_const_expr(&mut self, ty: ValType, u: &mut Unstructured) -> Result<ConstExpr> {
	let mut choices = mem::take(&mut self.const_expr_choices);
	choices.clear();
	// MVP wasm can `global.get` any immutable imported global in a
	// constant expression, and the GC proposal enables this for all
	// globals, so make all matching globals a candidate.
	for i in self.globals_for_const_expr(ty) {
	choices.push(Box::new(move |_, _| Ok(ConstExpr::global_get(i))));
	}

If you deem these two issues as worth fixing, I can take care of them by the way.

[alexcrichton]

However, its implementation for tag kind is confusing, which queries the functions field instead of the tags field, leading to an assertion failure when testing.

Ah yep, that's a bug!

wasm-smith accidentally generates a Wasm module that fails the validation

To clarify, this is before or after your changes? It looks like globals_for_const_expr is doing the right thing, so if it's after your changes are you updating globals and defined_globals in the right way? (e.g. exported globals go in both, but imported globals only go in globals. Also all imports must come first.