-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wasm GC: Fix an incorrect assertion and canonicalize types for runtime usage in ExternType::from_wasmtime #10223
Wasm GC: Fix an incorrect assertion and canonicalize types for runtime usage in ExternType::from_wasmtime #10223
Conversation
It is possible for two `WasmSubType`s to be equal to each other, as far as `derive(PartialEq)` is concerned, but still different from each other if they are in different rec groups or even if they are at different indices within the same rec group. The assertion mistakenly did not permit either of these, however. Fixes bytecodealliance#9714
Nick and I talked a bit about this in person and the conclusion was that we'll instead try canonicalizing |
…dule,Component}`s Rather than canonicalizing them on demand in functions like `{Func,Global,Table}Type::from_wasmtime` and other places. Instead, we do it in one place, up front, so that it is very unlikely we miss anything. Doing this involves changing some things from `ModuleInternedTypeIndex`es to `EngineOrModuleTypeIndex`es in `wasmtime_environ`, which means that a bunch of uses of those things need to unwrap the appropriate kind of type index at usage sites (e.g. compilation uses will unwrap `ModuleInternedTypeIndex`es, runtime uses usage will unwrap `VMSharedTypeIndex`es). And it additionally required implementing the `TypeTrace` trait for a handful of things to unlock the provided `canonicalize_for_runtime_usage` trait method for those things. All this machinery is required to avoid an assertion failure in the regression test introduced in the previous commit, which was triggered because we were failing to canonicalize type indices inside `ExternType`s for runtime usage on some code paths. We shouldn't have to play that kind of whack-a-mole in the future, thanks to this new approach.
f6a169b
to
12a0dbb
Compare
Subscribe to Label Action
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types", "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
see commit messages for details