feat: implement PartialEq/Eq/Hash for Func

[smarcd]

Adds a host-only inverse to Instance::debug_function for debugging tools that need to serialize a same-instance funcref as a Wasm function index. The lookup never exposes VM pointers and returns None when guest debugging is disabled or the function is not part of the instance.\n\nTests cover private functions, imports, an unrelated host function, and disabled guest debugging.

[github-actions]

Subscribe to Label Action

cc @cfallin, @fitzgen

Details

This issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:module", "isle", "wasmtime:c-api", "wasmtime:docs"

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

• cfallin: isle
• fitzgen: isle

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

Learn more.

[pchickey]

It looks like a bunch of commits from the 47 release branch got included in your PR branch for some reason - can you please rebase this cleanly on main?

[alexcrichton]

Could you detail your use case a bit more here? This is a pretty powerful debugging capability which also sort of inherently can't be efficient (e.g. the linear search here) and may also not hold up in future possible refactorings. Given the cost of supporting such an API I'd like to better understand the intended use case.

[smarcd]

This is for a host-side debugging workflow. The debugger captures an instance’s private mutable state at a breakpoint/checkpoint, then materializes that state into an isolated Store so it can inspect or continue the snapshot without mutating the original execution.

This is deliberately not production runtime functionality. The path is only enabled with guest debugging, runs at debug snapshot boundaries rather than during normal execution, and is allowed to trade efficiency for a small, well-contained API.

The specific need for debug_function_index is that function references are store-local. To restore a captured table/global funcref in the isolated debugging store, the debugger needs to serialize the same-module function identity as an index and resolve it through debug_function in the destination instance.

[cfallin]

The specific need for debug_function_index is that function references are store-local. To restore a captured table/global funcref in the isolated debugging store, the debugger needs to serialize the same-module function identity as an index and resolve it through debug_function in the destination instance.

But given that the implementation here is linear in the number of functions, your whole-store snapshot is going to run in quadratic time overall, which does not seem workable for anything semi-large. I'll second Alex's point that "linear search for this function" is not something we want to support.

I could see a Func::eq implementation making sense (because the primitive is harder to argue against -- it may be independently useful); if we also had Func::hash, then you could build a hashtable of funcrefs to defining instance and index within that instance in a single linear pass, then rename through that hashtable -- asymptotically better. I am not sure if I'm missing anything that would prevent us from providing Func::eq though (@alexcrichton ?).

[smarcd]

Fair point, and I think it's addressable without needing Func::eq/Func::hash — the actual complaint is the O(n) work per lookup (and thus the O(n²) whole-store cost), not the shape of the API. I pushed a rewrite that makes debug_function_index itself O(1) instead of scanning every function.

Both an instance's imported-function table and its defined-function funcref table are contiguous, fixed-stride arrays at statically known VMContext offsets, so a VMFuncRef pointer's position in either array is computable directly via pointer arithmetic instead of a linear scan:

• Imported functions are indexed by FuncIndex directly, so that's a direct offset computation.
• Defined ("escaped") functions are indexed by a compact FuncRefIndex. That index is not assigned in FuncIndex order (slots are handed out in the order functions are discovered to escape during translation — e.g. export declaration order — not declaration order), so resolving it back to a FuncIndex needs an explicit reverse table rather than arithmetic or a binary search.

That reverse table only depends on compiled module metadata, not on any particular Instance, so it's built once, lazily, and cached on Module — shared by every Instance and every debug snapshot of that module, rather than rebuilt per lookup or per instance. So a whole-store snapshot doing this once per captured funcref is now O(n) total (amortized), not O(n²).

Added a regression test (debug_function_index_with_non_monotonic_escape_order) that deliberately scrambles escape order via a table elem segment, to pin down that the reverse table is actually used correctly rather than assuming escape order tracks function-index order.

Also took the opportunity to rebase cleanly on main per @pchickey's comment above.

[cfallin]

I think that is still the sort of complexity that we would rather not take on if we don't have to: it makes the code more entangled (we now have a dependency on the vmctx layout; if the scheme ever needs to change we are now more restricted because we need to provide this property), and it's just a lot of delicate logic.

For reference, the "debug" variants of accessors have geneally been simple O(1) holes in the encapsulation, where under-the-hood Wasmtime already has the appropriate private accessors. This is a whole lot of new functionality instead for a niche use-case.

Is there a reason that Func::eq / Func::hash and then an external implementation of your snapshot/clone algorithm couldn't work?

[smarcd]

That's a fair concern — coupling to VMContext's internal layout is more entanglement than this is worth. I dropped debug_function_index and pushed Func::eq/Func::hash instead, per your suggestion.

Func now implements PartialEq/Eq/Hash as pointer-identity equality (same store, same underlying VMFuncRef). Comparing/hashing only reads the StoreId and the raw pointer bits — no dereference, no dependency on VMContext offsets, no assumptions about FuncRefIndex assignment order.

With that, our debugger can build the Func -> index map itself: walk the instance's function index space once with the existing Instance::debug_function, insert each (Func, index) pair into a HashMap, and invert a captured funcref through that map at snapshot time. Same result as debug_function_index, but the O(n) work (and any layout assumptions) live in our code instead of wasmtime's.

Added a regression test (debug_function_identity_round_trips_through_a_caller_built_map) that builds exactly that map over a module whose functions are placed into a table out of index order, to make sure the identity is real pointer/store identity and not something that only happens to work when escape order matches function-index order.

Much smaller diff now — just the two trait impls and a test.

[cfallin]

Thanks -- a few comments below.

Please feel free to change the title of this PR as well -- you're really adding Eq and Hash to Func, rather than adding any debug-specific mechanisms at all.

[cfallin]

No need for this narrative paragraph -- we generally don't have comments describing very specific single use-cases like this, nor a comparison to an alternative that used to (or in this case, never did) exist in the code.

The bit about shallow equality is also self-evident and probably not needed (a reasonable user would not expect f1 == f2 to, say, prove equivalence of two different algorithms).

The paragraph below about only comparing raw pointer bits is fine (a pseudo "safety comment" even though there's no literal unsafe block) I think.

[cfallin]

This unit test is fairly excessive -- again the narrative comment is unnecessary, and the test itself is testing the actual Wasmtime functionality in a fairly baroque way. All we really need to test is that f1 == f2 works when functions are fetched in different ways and/or carried through imports/exports between modules in the store, etc (and likewise for the hash value). Can you do that instead, and put it alongside other tests of the Func-related APIs rather than in debug?

[smarcd]

Done: title renamed, doc comment trimmed, test moved to tests/all/func.rs and simplified to fetch-multiple-ways equality (name, exports(), table).

One flag: Eq doesn't hold across an import/export boundary between two instances (each instance copies its own VMFunctionImport record, so the pointer differs even though it calls the same code). Left the test scoped to one instance. Let me know if cross-instance identity was actually expected (can compare through to the underlying code/vmctx pointer instead if so).

[cfallin]

Hmm, that's actually somewhat surprising API behavior, IMHO -- equality (and hashing) should hold when a Func refers to the same function within a store, regardless how it's reached.

I think this means we need to take a Store borrow so we can safely reach inside the raw pointers. That means we can't implement the Eq and Hash traits literally -- we need to provide separate methods on the Func for this. I'd imagine something like Func::is_same(&store, &f1, &f2) or similar. cc @alexcrichton for thoughts on this as well since it's a fairly conspicuous API surface...

[smarcd]

Agreed, and that tracks with what I found — pushed Func::is_same(store, a, b) and Func::identity_key(store) in place of the trait impls.

Both take a store borrow and dereference into the VMFuncRef's own identity (vmctx, wasm_call, array_call) rather than comparing the wrapper pointer's own address, so they see through the import/export copy: the underlying entry points and vmctx are identical across instances even though each instance holds its own VMFunctionImport record at a different address. identity_key returns an opaque Hash + Eq value for HashMap use, consistent with is_same.

Extended the test to cover the case that broke last time: a function imported into a second instance and re-exported from there now correctly reports is_same and equal identity_key.

[alexcrichton]

This seems reasonable to me, yeah, thanks! I'd probably go ahead and throw the type index into the hashed key as well to be safe, although it's probably not strictly necessary either

[cfallin]

Thanks @smarcd -- another nit below, and see Alex's point. If you can address those and get the CI green, I'm happy to approve and merge.

[cfallin]

It's probably sufficient to return just the wasm_call field here: each exported function will have the two entry points but either one will uniquely identify the function.

[smarcd]

Checked wasm_call before committing to it: it's None for a host Func::wrap until that Func is later paired with a module that has a matching trampoline, and it's filled in in place at that point (confirmed with a throwaway probe: None before instantiation, Some(..) after).

That's two problems for using it alone as the key:

• Two never-imported host functions both read wasm_call = None, so they'd collide.
• Even a single Func's key would change over the store's lifetime as it flips None → Some, which breaks HashMap use outright, not just an occasional collision.

array_call doesn't have either issue (always present, never mutates), so I kept (vmctx, array_call) and folded in type_index per Alex - dropped wasm_call instead of keeping only it.

Also fixed the regression test along the way: it wrapped two different || {} closures to check that distinct host functions aren't is_same, but two separate closure expressions are two distinct Rust types, so that assertion happened to pass for the wrong reason (different array_call, not different vmctx). Now wraps the same closure value twice, so array_call is shared and only vmctx discriminates, plus a new test asserting identity_key is stable across the wasm_call transition.

[cfallin]

Thanks -- just one nit then I'm happy to merge.

[cfallin]

No need for the narrative comment here (which bits are exported, etc) -- that's also something we can easily change if needed.

Better to explain why we choose these keys. We include vmctx to disambiguate statically-same functions from different instances of its module; we include the array-call pointer because it's always present and there is one per separate function; and we include the type-index as insurance to ensure that we disambiguate functions based on their Wasm-level signatures.

[smarcd]

Done.