wasmtime / PR #12015 Stack types · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #12015 Stack types

Wasmtime GitHub notifications bot (Nov 11 2025 at 01:24):

khagankhan opened PR #12015 from khagankhan:stack-types to bytecodealliance:main:

Stack fixup

We currently have every instruction balanced itself (for example, if an op leaves a struct onto the stack it immediately calls a function that consumes that struct) like we did for our generative fuzzer. However, for mutation based fuzzers this may have some bias.

This PR removes that and fixes the stack in the end. It keeps abstract stack types and check the required types then fixes the actual stack.

+cc @fitzgen @eeide

Wasmtime GitHub notifications bot (Nov 11 2025 at 01:24):

khagankhan requested alexcrichton for a review on PR #12015.

Wasmtime GitHub notifications bot (Nov 11 2025 at 01:24):

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

Wasmtime GitHub notifications bot (Nov 11 2025 at 01:58):

khagankhan updated PR #12015.

Wasmtime GitHub notifications bot (Nov 11 2025 at 02:12):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Nov 11 2025 at 02:12):

khagankhan created PR review comment:

I’d like to remove these “special cases” that require an index. The macro expansion doesn’t accept it and I’d rather not complicate the macro further. If you know a clean way to handle this, please share it with me. I’ll address it in the next PR anyway.

Wasmtime GitHub notifications bot (Nov 11 2025 at 02:12):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Nov 11 2025 at 02:12):

khagankhan created PR review comment:

Same "special case" here

Wasmtime GitHub notifications bot (Nov 11 2025 at 02:12):

khagankhan commented on PR #12015:

@fitzgen Ready for review!

Wasmtime GitHub notifications bot (Nov 11 2025 at 03:54):

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

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 (Nov 14 2025 at 02:51):

alexcrichton commented on PR #12015:

Thanks! Nick's out this week and I so far haven't had a chance to look at this. I may end up deferring this to Nick when he gets back as I'll otherwise have to boot back up on a lot of context here, but do you have other subsequent PRs ready to go which are built on this and so it'd be good to get this in sooner rather than later?

Wasmtime GitHub notifications bot (Nov 14 2025 at 02:57):

khagankhan commented on PR #12015:

Hey Alex! I am mostly working on a repo on GitLab where I am ahead. The Wasmtime PRs tend to lag behind my current work because I address comments, failed tests etc. Since Nick and I meet weekly (except this week) and go over everything, I think it makes sense to defer this to him. Thank you for the comment!

Wasmtime GitHub notifications bot (Nov 14 2025 at 03:00):

alexcrichton requested fitzgen for a review on PR #12015.

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

(ref extern) is a different type hierarchy from (ref any), that is there is no single top type of everything, so there is no function that can take anything and I want to make sure we don't build that assumption in here.

Backing up a bit: I am a bit confused about this function's purpose and why it is necessary. It seems like we shouldn't ever be changing the stack types, we should be only be doing the opposite: fixing up our ops given the types that are actually on the stack. The former doesn't make sense (we can't change what types are on the stack at this point without changing what instructions we emitted earlier).

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

Stylistically, it is quite rare to have the doc comment below the #[...] attributes. Mind reverting this code motion?

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

And let's also do an out parameter here as well.

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

Let's align with the Wasm spec and call this AnyRef.

We will eventually want to differentiate between (ref struct) and (ref any) and (ref eq) as well.

Aside: we will need to track nullability for all our different ref types eventually as well.

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen submitted PR review:

I'm back now, thanks for your patience!

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

Should this still be taking a stack: usize and returning a new usize? Shouldn't it be taking a stack: &mut Vec<StackType> now instead?

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

I think this fixing-up of ops should go in GcOp::fixup. We can add num_types as a parameter there. And if num_types == 0, we should probably just remove this op, no? How do we struct.new or call a function that takes a concrete struct reference if we don't define any types?

Wasmtime GitHub notifications bot (Nov 19 2025 at 19:41):

fitzgen created PR review comment:

To avoid a ton of temporary allocations, and reuse a single allocation instead, let's have this method take a mutable vector as an out parameter:
            pub(crate) fn operand_types(&self, types: &mut Vec<StackType>) {

Wasmtime GitHub notifications bot (Dec 01 2025 at 20:38):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2025 at 20:38):

khagankhan created PR review comment:

I am back!!!

Does this also mean that when we generate a new op, we should also be checking type compatibility, not just maintaining stack depth?

Wasmtime GitHub notifications bot (Dec 01 2025 at 20:39):

khagankhan edited PR review comment.

Wasmtime GitHub notifications bot (Dec 01 2025 at 22:36):

fitzgen created PR review comment:

Does this also mean that when we generate a new op, we should also be checking type compatibility, not just maintaining stack depth?

Yes, we either have to do that (if we continue the existing approach) or else we switch to a new approach and instead just generate a random sequence of arbitrary ops and then rely on the fixup pass to make it valid. The latter might be easier long term, so that there is only one code path we have to maintain.

But also, backing up, we shouldn't really need to generate op sequences from scratch. The whole point of using a mutation-based paradigm is that we can generate arbitrary inputs via a series of mutations over time. So, instead of generating whole op sequences, we _should_ be able to get away with something like
impl Generate<GcOps> for GcOpsMutator {
    fn generate(&mut self, ctx: &mut Context) -> mutatis::Result<GcOps> {
        let mut ops = GcOps::default();
        for _ in 0..N {
            self.mutate(ctx, &mut ops)?;
        }
        Ok(ops)
    }
}
(And we should probably build the generic version of this into mutatis directly eventually)

Wasmtime GitHub notifications bot (Dec 01 2025 at 22:36):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2025 at 22:39):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2025 at 22:39):

khagankhan created PR review comment:

Awesome! That was the answer I needed. After addressing this I will push

Thanks a lot

Wasmtime GitHub notifications bot (Jan 15 2026 at 12:13):

khagankhan updated PR #12015.

Wasmtime GitHub notifications bot (Jan 15 2026 at 17:36):

khagankhan updated PR #12015.

Wasmtime GitHub notifications bot (Feb 02 2026 at 02:41):

khagankhan updated PR #12015.

Wasmtime GitHub notifications bot (Feb 02 2026 at 16:33):

khagankhan commented on PR #12015:

@fitzgen Hi! It is ready for the review

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen created PR review comment:

This should be defined outside the loop, and then .clear()ed before each op.operand_types(...) call so that we are actually reusing the allocation across calls.

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen created PR review comment:

We cannot fixup the instruction if it references a concrete type index but num_types == 0. In that sense, the if num_types == 0 bit feels like it is part of the fixup call, since we cannot do the one without the other. I think this would be more clearly and cleanly modeled if we made it so that GcOp::fixup didn't mutate the existing op in place, but instead optionally returned a new op, using None to mean "this op is not valid in this context":
impl GcOp {
    fn fixup(&self, limits: &GcOpsLimits, num_types: u32) -> Option<Self> {
        ...
    }
}
~~~

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen created PR review comment:

Instead of (Option<StackType>, usize) can the operands and results be [StackType]? That is more flexible and we don't have to have the weird cases where there is a type, but the count is zero or whatever.

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen created PR review comment:

And this check is where we would return None instead of Some.

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen created PR review comment:

We generally try to avoid as conversions, since they can silently lose information:
        let num_types = u32::try_from(self.types.type_defs.len()).unwrap();

Wasmtime GitHub notifications bot (Feb 02 2026 at 23:22):

fitzgen created PR review comment:

Typos and formatting in the doc comment need to be fixed.

But more importantly, as I mentioned previously, we shouldn't have an "any" type that doesn't correspond to the any heap type in the spec. The way that we model polymorphic instructions in other places, such as the validator, we represent expected operand types as Option<Type> and use None to represent "okay with any type". So doing that is one option. Another option is to have a different GcOp for each type hierarchy: GcOp::DropAny to drop something from the any heap type hierarchy vs GcOp::DropExtern to drop something from the extern heap type hierarchy, etc... These would all emit the same Wasm drop instruction, but it would simplify the typing in our mutator. Either of those two approaches is fine, but we shouldn't have misleading "any" types that don't actually exist in the spec.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:42):

khagankhan updated PR #12015.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan created PR review comment:

I initially thought this was just confusing comments and naming, but I see the problem of having non-semantic type that may be confusing. I chose the first suggested approach. operand_types now usesOption<StackType>, and the None acts as a wild card to accept anything for drop.
I also updated the doc comments to clearly state which Wasm types each variant corresponds to.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan created PR review comment:

Updated. It got rid of those cases, and the expansion looks nice this way :) .

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan created PR review comment:

Yess! I remember that from The Book. Updated

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan submitted PR review.

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:53):

khagankhan created PR review comment:

Ouch! Totally missed that

Wasmtime GitHub notifications bot (Feb 03 2026 at 02:54):

khagankhan commented on PR #12015:

@fitzgen Thanks! It is ready for the next round of the reviews. Please let me know if I missed anything. Thanks!

Wasmtime GitHub notifications bot (Feb 03 2026 at 18:21):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Feb 03 2026 at 18:21):

fitzgen added PR #12015 Stack types to the merge queue.

Wasmtime GitHub notifications bot (Feb 03 2026 at 18:48):

fitzgen merged PR #12015.

Wasmtime GitHub notifications bot (Feb 03 2026 at 18:48):

fitzgen removed PR #12015 Stack types from the merge queue.

Last updated: Feb 24 2026 at 05:28 UTC