Stream: git-wasmtime

Topic: wasmtime / PR #9697 cranelift-entity: Add optional suppor...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2024 at 23:44):

caelunshun opened PR #9697 from caelunshun:cranelift-entity-bytemuck to bytecodealliance:main:

Adds optional implementations of Pod and Zeroable for EntityList and PackedOption.

This helps avoid some unsafe code in my downstream crates.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2024 at 23:44):

caelunshun requested abrown for a review on PR #9697.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2024 at 23:44):

caelunshun requested wasmtime-compiler-reviewers for a review on PR #9697.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2024 at 23:44):

caelunshun requested wasmtime-default-reviewers for a review on PR #9697.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2024 at 23:49):

caelunshun updated PR #9697.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2024 at 18:15):

abrown commented on PR #9697:

@cfallin, you want to take this review?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2024 at 18:19):

cfallin requested cfallin for a review on PR #9697.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2024 at 18:19):

cfallin commented on PR #9697:

@abrown Will do!

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2024 at 23:53):

cfallin submitted PR review:

Thanks for the PR, @caelunshun!

I have a specific comment below, but also a higher-level thought: in general we try to avoid taking on dependencies, even optional ones, because it adds to our vetting overhead (you can see the cargo vet failure in CI here; one of us would have to go and read the bytemuck sources to vet it) and just in general adds complexity. In particular here I'm somewhat concerned that this crate permits unsafe low-level casting and is claiming additional properties about our data structures; they may be true today, or may not (see comment), but we're also taking on additional burden reasoning about and upholding these properties going forward.

For your particular use-case, would it be a possible alternative for you to wrap whatever types need these trait impls in your own types, and impl them locally? That way you could carry the maintenance task of ensuring the properties still hold, and of vetting dependencies, without adding those overheads here. Curious what you think.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 02 2024 at 23:53):

cfallin created PR review comment:

This is a valid argument for the soundness of Pod, but not of Zeroable, as far as I can tell, right? In fact an all-zero-bits EntityList is not guaranteed to be valid at all, and we don't otherwise allow the existence of EntityList values that are "dangling"...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 04:00):

caelunshun created PR review comment:

For this specific comment, an all-zero-bits EntityList is sound to create; in fact the existing Default implementation already does so (the docs there say it creates an empty list).

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 04:00):

caelunshun submitted PR review:

Thanks for the review. Yes, not wanting to take on extra dependencies makes sense; I wasn't aware of the vetting policy.

From my impression bytemuck is a fairly central ecosystem crate, seeing use in security-conscious projects like Servo and wgpu, and is also vetted for use in Google's projects. And given cranelift-entity's applications outside of Wasmtime, integrating with the broader ecosystem would make sense to me. But it's not a big deal to me one way or the other.

As for your comment about guaranteeing a particular representation, the docs already do so implicitly, with EntityList being described as a single u32 and PackedOption<T> being a trivial wrapper around T. So implementing these traits in cranelift-entity just allows downstream crates to take advantage of this layout without unsafe code that could silently break with a version bump.

The reason I couldn't write a newtype around cranelift-entity types and implement Pod is because their layout is not currently guaranteed; the upstream types would need to be repr(C) or repr(transparent) (as changed in this PR) for that to be sound. I could change the PR to simply add these repr annotations and not the bytemuck dependency if you like.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 07:12):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 07:12):

cfallin created PR review comment:

Ah, yes, you're right; I had forgotten that we use zero as an empty-list sentinel here. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 07:15):

cfallin commented on PR #9697:

Thanks for the additional detail! Given all that, I think the deciding factor for me is the vetting overhead: we apparently don't pull in that Google vet (or it's not sufficient for our required level?) and I don't have cycles to go read all of bytemuck and convince myself of its correctness right now, so I think I'd prefer to see this be a minimal PR that adds the necessary repr directives to structs instead and documents invariants (zero-valued sentinels, etc) appropriately. I'd be happy to approve and merge such a PR.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2024 at 02:46):

caelunshun updated PR #9697.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2024 at 02:48):

caelunshun updated PR #9697.

Last updated: Dec 23 2024 at 12:05 UTC