Skip to content

Introduce wasmtime::ArrayRef and allocating Wasm GC arrays#9145

Merged
fitzgen merged 4 commits intobytecodealliance:mainfrom
fitzgen:host-allocate-gc-arrays
Aug 20, 2024
Merged

Introduce wasmtime::ArrayRef and allocating Wasm GC arrays#9145
fitzgen merged 4 commits intobytecodealliance:mainfrom
fitzgen:host-allocate-gc-arrays

Conversation

@fitzgen
Copy link
Copy Markdown
Member

@fitzgen fitzgen commented Aug 19, 2024

This commit introduces the wasmtime::ArrayRef type and support for allocating Wasm GC arrays from the host. This commit does not add support for the array.new family of Wasm instructions; guests still cannot allocate Wasm GC objects yet, but initial support should be pretty straightforward after this commit lands.

The ArrayRef type has everything you expect from other value types in the wasmtime crate:

  • A method to get its type or check whether it matches a given type

  • An implementation of WasmTy so that it can be used with Func::wrap-style APIs

  • The ability to upcast it into an AnyRef and to do checked downcasts in the opposite direction

There are, additionally, methods for getting, setting, and enumerating a ArrayRef's elements.

Similar to how allocating a Wasm GC struct requires a StructRefPre, allocating a Wasm GC array requires an ArrayRefPre, and this is motivated by the same reasons.

@fitzgen fitzgen requested a review from a team as a code owner August 19, 2024 22:06
@fitzgen fitzgen requested review from alexcrichton and removed request for a team August 19, 2024 22:06
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime labels Aug 20, 2024
@github-actions
Copy link
Copy Markdown

Subscribe to Label Action

cc @fitzgen

Details This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"

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

  • fitzgen: wasmtime:ref-types

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

Learn more.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Just some minor comments 👍

Comment on lines +114 to +116
let offset = usize::try_from(offset).unwrap();
let end = offset.checked_add(N).unwrap();
let bytes = self.data.get(offset..end).expect("out of bounds field");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should these propagate Result to avoid panicking on out-of-bounds things?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(same for writing below)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I realize this is preexisting but having read/write on arrays not actually return a result above felt a bit weird

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This isn't really a user-visible failure case; the only way it can fail is a bug in the runtime or the GC. Additionally, the non-vm layer is responsible for performing the various type checks and such — everything that is fallible and recoverable from the user — and this layer is just handing out bytes from the GC heap, but also makes sure that everything is in bounds to preserve our memory-safety-in-the-face-of-GC-bugs properties. Given all that, I'm inclined to leave things as they are, although I am happy to update docs if you think the docs at the top of this type aren't detailed enough.

@fitzgen fitzgen added this pull request to the merge queue Aug 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 20, 2024
@fitzgen fitzgen force-pushed the host-allocate-gc-arrays branch from fb14b52 to f9a2465 Compare August 20, 2024 16:38
@fitzgen fitzgen requested a review from a team as a code owner August 20, 2024 16:38
@fitzgen fitzgen requested review from abrown and removed request for a team August 20, 2024 16:38
@fitzgen fitzgen enabled auto-merge August 20, 2024 16:38
This commit introduces the `wasmtime::ArrayRef` type and support for allocating
Wasm GC arrays from the host. This commit does *not* add support for the
`array.new` family of Wasm instructions; guests still cannot allocate Wasm GC
objects yet, but initial support should be pretty straightforward after this
commit lands.

The `ArrayRef` type has everything you expect from other value types in the
`wasmtime` crate:

* A method to get its type or check whether it matches a given type

* An implementation of `WasmTy` so that it can be used with `Func::wrap`-style
  APIs

* The ability to upcast it into an `AnyRef` and to do checked downcasts in the
  opposite direction

There are, additionally, methods for getting, setting, and enumerating a
`ArrayRef`'s elements.

Similar to how allocating a Wasm GC struct requires a `StructRefPre`, allocating
a Wasm GC array requires an `ArrayRefPre`, and this is motivated by the same
reasons.
@fitzgen fitzgen force-pushed the host-allocate-gc-arrays branch from 6bbaea6 to 9e8e2f5 Compare August 20, 2024 18:24
@fitzgen fitzgen added this pull request to the merge queue Aug 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 20, 2024
@fitzgen fitzgen enabled auto-merge August 20, 2024 19:25
@fitzgen fitzgen added this pull request to the merge queue Aug 20, 2024
Merged via the queue into bytecodealliance:main with commit c4be2d8 Aug 20, 2024
@fitzgen fitzgen deleted the host-allocate-gc-arrays branch August 20, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants