Skip to content

fix: store and load every value type at its own width - #228

Merged
arcjet-rei merged 2 commits into
mainfrom
rei/fix/ENG-1377-memory-store-widths
Oct 3, 2026
Merged

arcjet-rei merged 2 commits into
mainfrom
rei/fix/ENG-1377-memory-store-widths

Conversation

@arcjet-rei

Copy link
Copy Markdown
Contributor

Gravity reads and writes guest memory for record fields, list elements, a host import's result and an export's indirect parameters. Several value types were handled at the wrong width or type there:

  • F32Store and F64Store called WriteUint64Le. Since fix: Handle Wasm ValueTypes returned by imports #141 made import-side floats plain float32/float64, a host import returning a record with a float field generated Go that did not compile. On the export side, F32Store wrote 8 bytes for a 4-byte value and overwrote the memory after it, and F32Load read 8 bytes.
  • I32Store8 accepted only 0 and 1, so storing a u8, s8, enum or option<u8> payload above 1 panicked.
  • I32Store16, I64Store, I32Load8S, I32Load16U and I32Load16S were todo!(), so u16, s16, u64, s64 and s8 values in memory crashed gravity.

Each store now writes exactly its width with an explicit conversion, and the missing loads are implemented. The integer lifts use Go conversions, so they accept both the uint64 that api.Function.Call returns and the narrower values a load produces. Floats use wazero's WriteFloat32Le/ReadFloat32Le and their f64 counterparts for imports, and the IEEE bits for exports.

The new memory example sends a record of every scalar type (17 flattened values, so it always travels through memory) into and out of an export and a host import. It also round-trips a 68-byte record whose trailing f32 sits directly before a string allocation.

Regenerating existing bindings rewrites some lines: stores gain explicit conversions such as WriteByte(p, uint8(0)), and bool stores lose their 0/1 switch. The bytes written are unchanged for every value those stores receive.

🤖 Generated with Claude Code

Values that pass through guest memory (record fields, list elements, a
host import's result, an export's indirect parameters) were written and
read with the wrong width or type:

- F32Store and F64Store called WriteUint64Le. On the import side, where
  #141 made floats plain float32 and float64, the generated
  Go no longer compiled; on the export side, F32Store wrote 8 bytes for
  a 4-byte value and overwrote whatever followed it. Imports now use
  WriteFloat32Le/WriteFloat64Le and exports write the IEEE bits at the
  value's width. F32Load read 8 bytes as well; it now reads 4.
- I32Store8 only accepted 0 and 1 and panicked otherwise, so a u8, s8,
  enum or option<u8> payload above 1 panicked. It now stores the low
  byte of any value.
- I32Store16, I64Store, I32Load8S, I32Load16U and I32Load16S were
  todo!(), so u16, s16, u64, s64 and s8 values in memory crashed
  gravity. They are implemented, with the loads sharing a read_memory
  helper for the failed-read check.
- Each store converts its operand explicitly, and the integer lifts use
  Go conversions instead of api.DecodeI32/DecodeU32, so they accept the
  uint64 CallWasm returns as well as the narrower values loads produce.
  The float lifts pass Go floats through on the import side.

The new memory example passes a record of every scalar type (17
flattened values, so it always travels through memory) to an export,
back from an export, to a host import and back from it, plus a 68-byte
record whose trailing f32 sits directly before a string allocation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@arcjet-rei
arcjet-rei requested a review from a team as a code owner October 3, 2026 02:16

@arcjet-review arcjet-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arcjet Review — 🟡 Medium Risk

Decision: Checked

Rationale: This PR makes meaningful changes to core code generation for Wasm canonical ABI memory loads/stores, so the risk is Medium rather than Low. The changes appear well-scoped and consistent: byte/16/32/64-bit integer stores now explicitly write the correct width, missing 8/16/64-bit memory operations are implemented, and f32/f64 handling now distinguishes host import float values from export-side raw IEEE bits returned by wazero. The new memory example and tests exercise round-trips through both exports and host imports across boundary integer values, strings, chars, bools, options, enums, and floats. Security review found no hardcoded secrets, auth changes, injection paths, or new unsafe command/query construction. The dependency change is limited to a new example crate using exact pinned versions, and does not introduce an apparent application runtime dependency.

Summary of Changes

Fixes Gravity's generated Go bindings so scalar values are loaded from and stored to guest memory at their canonical ABI widths, including f32/f64, i8/u8, i16/u16, and i64/u64 paths. Adds a new memory example and generated stdout fixtures to test indirect record parameters/results and host import memory interactions.

Escalation Triggers

  • Dependency Changes: Adds examples/memory/Cargo.toml with pinned wit-bindgen and wit-component dependencies for the new example crate.

Review Focus Areas

Notes

The diff exceeds the 500-line automated-review threshold, largely because it includes generated stdout fixtures. Automated review still found the implementation coherent, but humans should skim the generated outputs and run the relevant tests before merge.

Path filtering: 1 file excluded by ignore paths. 14 of 15 files included in review.

The AI assessed this PR as approvable, but the trust level (1) does not allow auto-approval. A human reviewer must approve this PR.

Review: 3b69e21a | Model: openai/gpt-5.5 | Powered by Arcjet Review

Comment thread examples/memory/memory_test.go
@arcjet-review arcjet-review Bot removed the needs review Awaiting human review label Oct 3, 2026
Send +Inf, -Inf, NaN and -0 as the f32 and f64 fields through an export's
result, a host import's parameter and a host import's result. The
comparison treats any NaN as equal to any NaN, since reflect.DeepEqual
cannot, and checks the sign so -0 is not mistaken for +0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@arcjet-rei
arcjet-rei merged commit 8119322 into main Oct 3, 2026
4 checks passed
@arcjet-rei
arcjet-rei deleted the rei/fix/ENG-1377-memory-store-widths branch October 3, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants