wasmtime / PR #11713 Add generic/typed `take_struct` impo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #11713 Add generic/typed `take_struct` impo...

Wasmtime GitHub notifications bot (Sep 18 2025 at 23:04):

khagankhan opened PR #11713 from khagankhan:rec-mutators to bytecodealliance:main:

Add a function import that takes a structref argument to our Wasm binary skeleton

Add a fuzzer op that calls that new function

Add a function import for each defined struct type that takes a reference of that type specifically

Add a fuzzer op that calls these new typed functions

Generic (type (;4;) (func (param (ref any)))) is called after StructNew op instead of Drop to observe that specific struct and also maintain stack discipline.

Typed versions are called with specific structs of the types

cc @fitzgen @eeide

Wasmtime GitHub notifications bot (Sep 18 2025 at 23:04):

khagankhan requested alexcrichton for a review on PR #11713.

Wasmtime GitHub notifications bot (Sep 18 2025 at 23:04):

khagankhan requested wasmtime-fuzz-reviewers for a review on PR #11713.

Wasmtime GitHub notifications bot (Sep 18 2025 at 23:16):

khagankhan updated PR #11713.

Wasmtime GitHub notifications bot (Sep 19 2025 at 02:13):

github-actions[bot] commented on PR #11713:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen submitted PR review:

Looks good with the below addressed, thanks!

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen created PR review comment:

Would be nice to have a single-sentence comment for readers here describing what this code is doing, so that they don't have to scrutinize it to figure out what is going on. Something like

For each of our concrete struct types, define a function import that takes an argument of that concrete type.

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen created PR review comment:

Not necessary in this PR, but it would probably be easier/cleaner down the line to switch from constants to using the wasm_encoder::{Type,Func}Section::len methods to keep track of these things, so that we don't need to update these constants every time we unconditionally add a new type/function.

https://docs.rs/wasm-encoder/latest/wasm_encoder/struct.TypeSection.html#method.len

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen created PR review comment:

You can avoid the wonky .last() stuff by pushing the name after defining the import:

            let name = format!("take_struct_{concrete}");
            imports.import(
                "",
                &name,
                EntityType::Function(ty_idx),
            );
            typed_names.push(name);

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen created PR review comment:

As mentioned earlier, things get a bit simpler and less brittle if we use the len functions on the sections that define index spaces:
        let mut functions = FunctionSection::new();
        let mut exports = ExportSection::new();

        // Define the "run" function export.
        let run_index = functions.len();
        functions.function(1);
        exports.export("run", ExportKind::Func, run_index);
I think it is worth doing this to avoid adding more opaque <constant> + <count> incantations.

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen created PR review comment:

Empty comment at the end of line?

Wasmtime GitHub notifications bot (Sep 30 2025 at 17:28):

fitzgen created PR review comment:

Nothing to change in this PR, but I want to point out that these emitted instruction sequences are not our ideal end state. We want to separate struct.new from various calls and drops and have the existing TableOp::Drop be used to drop struct refs and we want the take_* calls to take a struct ref from the stack, not create a new one to pass to the import. This will require updating the abstract stack representation to track types, however, so it should be left to a follow up PR.

Wasmtime GitHub notifications bot (Sep 30 2025 at 19:19):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2025 at 19:19):

khagankhan created PR review comment:

Yes, I agree!

Wasmtime GitHub notifications bot (Sep 30 2025 at 19:19):

khagankhan commented on PR #11713:

Thanks @fitzgen. Working on them!

Wasmtime GitHub notifications bot (Oct 15 2025 at 22:30):

khagankhan updated PR #11713.

Wasmtime GitHub notifications bot (Oct 18 2025 at 00:12):

khagankhan updated PR #11713.

Wasmtime GitHub notifications bot (Oct 20 2025 at 19:15):

khagankhan commented on PR #11713:

@fitzgen @eeide
Ready for review
Changes

Split table_ops.rs into separate modules and renamed to GcOps where applicable

Addressed prior review feedback

Ran the fuzzer for ~1 hour and fixed generation bugs discovered

Wasmtime GitHub notifications bot (Oct 20 2025 at 20:40):

fitzgen commented on PR #11713:

@khagankhan can you delay the split into multiple modules for a follow up PR that contains nothing but the code motion? Otherwise it makes reading and interpreting the diff very hard, as a reviewer.