Conversation
guybedford
left a comment
There was a problem hiding this comment.
AI-Assisted Review: PR #304 — Full GC Extension Support
Note: This review was generated with AI assistance (Claude). Findings should be verified by a human reviewer.
Build & Test Status
All tests pass locally:
- 30 unit tests
- 105 round-trip tests (35 GC-specific)
- 267 spec tests (6 known-failing, all justified)
- 18 doc-tests
Overall Assessment
This is a high-quality, well-structured PR. The type system overhaul, rec group handling (placeholder-then-finalize pattern), and three-phase GC liveness pass are architecturally sound. Test coverage is excellent with 17+ dedicated GC round-trip WAT files, thorough builder API tests, and a new wasm-smith fuzzer. The removal of ~30 entries from the known-failing list is well-justified.
See inline comments for specific findings, summarized here by severity:
Bugs (should fix):
ConstExpr::Extendedsilently ignored in element segments, data/elem offsets, and table init (GC liveness -- types/funcs/globals may be incorrectly collected)Ordimpl onTypeis inconsistent withPartialEq/Hash(missingis_for_function_entry)delete()doesn't clean up rec groups (partial deletion silently drops live siblings)
Design concerns (should address or document):
4. HeapType::Exact collapsed into Concrete -- loses exactness info on round-trip
5. shared flag silently discarded -- shared composite types accepted but emitted as unshared
6. #[walrus(skip_visit)] on HeapType fields is fragile -- new instructions need manual visitor overrides
Nits:
RecGroupIddefined/exported but never usedrec_group_for_typeis O(n*m) -- consider reverse index- Stale "Phase 3" comment in gc-types-parse.rs
- Fuzzer fixed seed limits exploration;
max_types=10may be small for GC
|
Split the fuzzer runs into two jobs since adding wasm-smith caused it to consistently time out. On the bright side, |
|
Our very old version of WABT had a memory corruption bug with recursive calls, which I believe was causing watgen CI to run forever. I bumped the dependency version and added a manual timeout in Rust. |
Type System Overhaul
Type was previously a flat wrapper around function signatures. It's now a subtype node wrapping a CompositeType enum.
Type::params()andType::results()still work but panic on non-function types. New code should usekind(),as_function(),as_struct(),as_array().Types can now reference each other by type index, or by rec group relative type index.
HeapType::Concretenow holds aTypeId(arena allocated) instead of just au32. Most functions that convert types now must pass aIdsToIndicies/IndicesToId, and sometimes arec_group_startu32 index.Recursive Type Groups
Rec groups are the hardest part of GC — types within a group can reference each other, requiring forward references. The solution is a placeholder-then-finalize pattern:
TypeIdswith dummy placeholder valuesArenaSetgained new methods to support this:alloc_unique()(allocate without dedup),replace_and_register()(overwrite + register in dedup map),find()(lookup without insert).Singletons (every non-recursive type) are deduplicated, but explcit multi-type groups are never deduplicated per the spec. Emission is now rec-group-ordered instead of flat/sorted.
ModuleType::deletenow has a warning that the user is responsible for checking if types in the same rec group reference the deleted type. Added two new delete methods:try_delete- Same as delete, but fails if another type in the same rec group depends on the type to be deleteddelete_entire_group- Helper function that deletes a type, along with every other type in its rec group.GC Liveness Pass
Significantly extended with three new phases:
Tests
All GC spec tests (except those that rely on other proposals) are now enabled. A new fuzzer
wasm-smithhas been added, since WABT does not have proper support for GC yet.