feat: add a 'records' example, and some required instructions#116
feat: add a 'records' example, and some required instructions#116sd2k wants to merge 5 commits intoarcjet:mainfrom
Conversation
Prior to this commit there was ostensibly no support for 'record' WIT types, but there wasn't actually much required for them to work. This commit adds an example for use in the test harness and the required instructions to load/store integer record fields. Relates to #4.
| quote_in! { self.body => | ||
| $['\r'] | ||
| $result := $WAZERO_API_DECODE_U32($operand) | ||
| $result := $WAZERO_API_DECODE_U32(uint64($operand)) |
There was a problem hiding this comment.
This may look weird but operand can sometimes be a uint32 now, if this U32FromI32 instruction is called after a PointerLoad, as happens in records, which calls ReadUint32Le. Since DecodeU32 requires the arg to be a uint64, we have to explicitly cast, which should be a no-op since the u32 always fits in the u64.
There was a problem hiding this comment.
I've actually encountered this today and it is a bigger problem than just this fix. I think all of the read/writes need to be converted to the uint64 variant but that large of a change is going to need validation.
blaine-arcjet
left a comment
blaine-arcjet
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing. I have some thoughts inline but I'm happy to apply them so we can land this.
| let operand = &operands[0]; | ||
| quote_in! { self.body => | ||
| $['\r'] | ||
| $(&value) := int64($operand) |
There was a problem hiding this comment.
Is there a Wazero utility for this since a U64 could wrap in an I64?
There was a problem hiding this comment.
Not that I can see 😕 from what I can tell the behaviour is correct though and follows two's complement behaviour that both Go and Wasm expect.
examples/records/wit/records.wit
Outdated
| } | ||
|
|
||
| world records { | ||
| use types.{foo}; |
There was a problem hiding this comment.
If the use is avoided, does this skip the interface generation?
There was a problem hiding this comment.
It would be invalid WIT:
error: failed to resolve directory while parsing WIT for path [/home/ben/repos/rust/gravity/examples/records/wit]
Caused by:
failed to parse package: /home/ben/repos/rust/gravity/examples/records/wit
Caused by:
name `foo` is not defined
--> /home/ben/repos/rust/gravity/examples/records/wit/records.wit:16:30
|
16 | export modify-foo: func(f: foo) -> foo;
| ^--
we could declare the foo type inline though if we want to avoid the interface generation, I've done that in 8544864 👍
Co-authored-by: blaine-arcjet <146491715+blaine-arcjet@users.noreply.github.com>
|
Hey @sd2k, I've taken over maintenace of gravity for Arcjet since @blaine-arcjet has since left the company, and after fixing a couple other issues, I finally had the wherewithal to deal with this one. If you head over to #171, I've created a new PR rebased on top of main and with the additional changes necessary to fix the issues Blaine and I were seeing when using this PR to generate our own bindings. If you could confirm that that change also works for your use case, we can finally get your change merged. If you look at 08b86cc, you'll see that in the end, the solution turned out to be faily simple (until we decide to tackle variant type definition generation, which also doesn't look that difficult, at least until I try to make sure it's adequately tested). Because everything involved follows the same strategies for lifting and lowering scalars (zero extension and truncation, respectively), the worries that Blaine and I had about two's complement confusion were unnecessary. Anyway, thanks for the contribution and your patience. Let me know if #171 works for you, and if it doesn't, how it fails! |
3 participants
Prior to this commit there was ostensibly no support for 'record' WIT
types, but there wasn't actually much required for them to work. This
commit adds an example for use in the test harness and the required
instructions to load/store integer record fields.
Relates to #4.