Stream: git-wasmtime

Topic: wasmtime / PR #1781 Initial, partial support for `externref`

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 18:18):

fitzgen requested alexcrichton for a review on PR #1781.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 18:18):

fitzgen opened PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 18:34):

fitzgen updated PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 19:39):

fitzgen updated PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 20:25):

fitzgen updated PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Since data isn't an owned value this doesn't actually do anything, right?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

.unwrap()?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Perhaps defer to https://doc.rust-lang.org/std/alloc/fn.handle_alloc_error.html?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

This looks like it's sharing a lot of code with allocation below, could that be refactored out to a shared function?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Is this a place in the C API where the store should be passed in?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Looks like this file became executable?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

This can probably just be deleted now, it doesn't seem to serve too much purpose any more

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

This and traits below seem dangerous since I'd naively expect it to defer to the underlying type (which can't be done here b/c it's not generic). Could this be exposed as a free-function?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

This seems like a fine API to expose though, especially because Caller below exposes it too

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

out of date comment?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Ideally we'd move this out of the wasmtime crate entirely. Can all the host_info stuff move to wasm_store_t in the C API? I think the C API could store a Rc<wasm_store_t> on all the exported types, right?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

The only reason this is here is for the host_info APIs, right? If so I would personally prefer that we just jettison that support from the C API for now. I think we should find a better way for implementing that, if necessary, instead of altering the design of the wasmtime crate itself.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Can this structure move to the C API?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:38):

alexcrichton created PR Review Comment:

Could this pass in self.store() and defer to Debug for Store?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:39):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:39):

fitzgen created PR Review Comment:

Yeah, this is more of a doing-everything-by-the-book and crossing-out-ts-and-dotting-our-is sort of thing for avoiding UB, but it is maybe overly conservative.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:25):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:25):

fitzgen created PR Review Comment:

This is needed for the wasi_instance_bind_import WASI C API function -- do you happen to know if this is part of the C API or part of wasi?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:36):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:36):

fitzgen created PR Review Comment:

As mentioned in chat, it is useful to have this built into the wasmtime crate so that host infos can be cleaned up when their associated externref goes away.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:37):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:37):

fitzgen created PR Review Comment:

Store doesn't implement Debug, but I can add an impl.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:52):

fitzgen updated PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:54):

fitzgen updated PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:55):

fitzgen requested alexcrichton for a review on PR #1781.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 22:10):

fitzgen updated PR #1781 from externref to master:

This is enough to get an externref -> externref identity function passing.

However, externrefs that are dropped by compiled Wasm code are (safely) leaked. Follow up work will leverage cranelift's stack maps to resolve this issue.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 16:41):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 16:41):

alexcrichton created PR Review Comment:

Ah I think that's just part of WASI, but this seems fine to add anyway

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 16:45):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 16:47):

fitzgen merged PR #1781.

Last updated: Dec 23 2024 at 13:07 UTC