Wasm-smith's `--exports` option does not support exporting globals with GC types

[mobsceneZ]

When providing the following module to the --exports option, wasm-smith panicked with the error messsage "called Result::unwrap() on an Err value: CanonicalizedHeapTypeReference":

(module
  (rec (type $t0 (sub final (array i32))))
  (global $g1 (mut (ref null $t0)) ref.null $t0)
  (export "global$1" (global $g1))
)

A quick look at the source code reveals that the problem is with the entity_type_from_export() function:

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

Lines 1971 to 1975 in bc0a785

	let new_index = match exports_types
	.entity_type_from_export(&export)
	.unwrap_or_else(|| {
	panic!("Unable to get type from export {export:?} in `exports` Wasm",)
	}) {

It returns a wasmparser::GlobalType that contains canonicalized CoreTypeId. Then, the TryFrom trait implementation is called to convert the original wasmparser::GlobalType into wasm_encoder::GlobalType, but the conversion demands type space indices rather than core type ids, which eventually results in a panic.

I guess maybe we could remove the validation code inside _required_exports function? The _arbitrary_imports_from_available function doesn't seem to perform any validity checks too, so probably it's ok to do so 🤔.

[alexcrichton]

Yes handling of --exports for wasm GC will need some work. The type section of the input wasm module effectively needs to be copied over into the generated module (as opposed to just function types from today). I think it goes a bit beyond just validating a few things and structurally needs to be implemented. That being said it would be great to have!

[mobsceneZ]

Yes handling of --exports for wasm GC will need some work. The type section of the input wasm module effectively needs to be copied over into the generated module (as opposed to just function types from today). I think it goes a bit beyond just validating a few things and structurally needs to be implemented. That being said it would be great to have!

Thanks for the reply! I just want to check if I understand your point correctly, are you suggesting that we copy the type section from the input wasm module and discard the validation part, similar to what _arbitrary_imports_from_available function does? That seems to be the simplest solution I can think of that doesn't require significant modifications to the source code 🤔.

[fitzgen]

That would make sense except that this would then mean that using both available imports and --exports with GC types would be incompatible with each other, as they would both try to copy over their respective type sections and only one or the other would have its types' indices preserved in the new types index space.

Therefore, one of them would need to instead append the types to the new type index space and remap all type index references to their new indices (which could be as simple as just adding the length of the types section before the new types were appended).

[alexcrichton]

One thing I've considered in the past is to deprecated/remove the separate specifications for available imports and exports and instead have something like --module-shape module.wasm or something like that which simultaneously specifies both imports and exports. The current design is mostly an artifact of how the implementation evolved and I'm not sure it's too too useful to keep it per se (although we probably can't outright remove it immediately lest we break folks)

Otherwise though I'd agree with @fitzgen in that there's probably not a simple solution to this, it's likely going to require some refactoring.

[mobsceneZ]

Thanks @fitzgen @alexcrichton ! I think combining the --available-imports and the --exports option into one --module-shape option would be a really nice idea and tbh I have also considered doing that. From my perspective, it seems there are two potential solutions now:

• Add a new --module-shape option that allows for the simultaneous declaration of imports and exports. The downside, of course, is that this might introduce duplicate code snippets into wasm-smith crate (if we want to preserve the --available-imports and the --exports option);
• Modify the _required_exports() function by copying the entire type section from input module and remapping relevant type indices, which may cause the resulting module to bloat;

Which approach is more appropriate for the current state of wasm-smith? And I'd be happy to contribute a PR for the chosen solution.

[alexcrichton]

How about:

• Add --module-shape. Internally this does everything --available-imports does, but it also supports exports
• Forbid using --module-shape with either --available-imports or --exports
• Refactor --available-imports to use the same implementation as --module-shape
• Basically leave --exports as-is, knowingly failing on GC types