Skip to content
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

wasmtime-runtime: Allow tables to internally hold externrefs #1882

Merged

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jun 15, 2020

This commit enables wasmtime_runtime::Table to internally hold elements of
either funcref (all that is currently supported) or externref (newly
introduced in this commit).

This commit updates Table's API, but does NOT generally propagate those
changes outwards all the way through the Wasmtime embedding API. It only does
enough to get everything compiling and the current test suite passing. It is
expected that as we implement more of the reference types spec, we will bubble
these changes out and expose them to the embedding API.

cc #929

This commit enables `wasmtime_runtime::Table` to internally hold elements of
either `funcref` (all that is currently supported) or `externref` (newly
introduced in this commit).

This commit updates `Table`'s API, but does NOT generally propagate those
changes outwards all the way through the Wasmtime embedding API. It only does
enough to get everything compiling and the current test suite passing. It is
expected that as we implement more of the reference types spec, we will bubble
these changes out and expose them to the embedding API.
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 15, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

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

  • peterhuene: wasmtime:api

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

Learn more.

@alexcrichton alexcrichton merged commit 8d671c2 into bytecodealliance:master Jun 15, 2020
@alexcrichton
Copy link
Member

👍

Copy link
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

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

Looks good with Option<VMExternRef> binary representation addressed.

current_elements: x.len().try_into().unwrap(),
},
TableElements::ExternRefs(x) => VMTableDefinition {
base: x.as_ptr() as *const u8 as *mut u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

We are exposing Option<VMExternRef> elements to the CL or other low level compiler. It is hard to guess what type of binary representation we will be dealing with here. For comparison, I see #[repr(C)] near VMCallerCheckedAnyfunc.

Copy link
Member Author

Choose a reason for hiding this comment

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

VMExternRef is repr(transparent) around a NonNull which is repr(transparent) around a pointer. Additionally, Option<NonNull<...>> is guaranteed to have the same representation as a pointer. So we should be good as far as binary representation goes.

@fitzgen fitzgen deleted the tables-can-hold-externref branch June 15, 2020 22:00
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants