Use a union for VMContext for functions #1753
Conversation
This makes the code more self documenting and correctly uses `unsafe` to communicate that it's the user's responsibility to ensure that the code paths that can lead to the access can only write the value they expect to be there.
lib/vm/src/vmcontext.rs
Outdated
| /// We stop lying about what this daat is | ||
| /// TODO: | ||
| #[derive(Copy, Clone)] | ||
| pub union VMFunctionExtraData { |
There was a problem hiding this comment.
Optional: consider the name VMFunctionContextParameter or VMFunctionEnvParameter, or possibly CallbackParameter. Whether it's a wasm function or a host function this is giving that function its context/env.
There was a problem hiding this comment.
I'm down for renaming, but I'll wait for more feedback before renaming! I like your suggestions though
There was a problem hiding this comment.
I also like VMFunctionContextParameter. I would have use VMFunctionEnvironment first, but it creates a naming conflict with “host environment” or similar.
There was a problem hiding this comment.
Don't forget to rename the extra_data field everywhere accordingly too :-).
There was a problem hiding this comment.
I like VMFunctionEnvironment
lib/vm/src/vmcontext.rs
Outdated
| /// We stop lying about what this daat is | ||
| /// TODO: | ||
| #[derive(Copy, Clone)] | ||
| pub union VMFunctionExtraData { |
There was a problem hiding this comment.
I also like VMFunctionContextParameter. I would have use VMFunctionEnvironment first, but it creates a naming conflict with “host environment” or similar.
lib/vm/src/vmcontext.rs
Outdated
| /// We stop lying about what this daat is | ||
| /// TODO: | ||
| #[derive(Copy, Clone)] | ||
| pub union VMFunctionExtraData { |
There was a problem hiding this comment.
Don't forget to rename the extra_data field everywhere accordingly too :-).
| /// Wasm functions take a pointer to [`VMContext`]. | ||
| pub vmctx: *mut VMContext, | ||
| /// Host functions can have custom environments. | ||
| pub host_env: *mut std::ffi::c_void, |
There was a problem hiding this comment.
As far as I understand this PR (which backport and enhance (!) a pre-1.0 feature), host_env can hold 2 kind of pointers: VMContextWithoutEnv and VMContextWithEnv. To clarify things, wouldn't it be possible to write this union as follows:
pub enum Either<L, R> {
Left(L),
Right(R),
}
pub union VMFunctionContextParameter {
pub vmctx: *mut VMContext,
pub host_env: Either<*mut VMContextWithEnv, *mut VMContextWithoutEnv>,
}mem::size_of::<VMFunctionContextParameter> would be around 16 bytes, which is quite “heavy”. Without Either, it's 8 bytes.
If we don't really care about the size of this pointer, I would suggest to use Either or a similar enum, to be super explicity about what kind of pointer host_env can hold.
There was a problem hiding this comment.
My understanding is that there are 3 states that can exist on this single pointer:
- The pointer is used in the context of a compiled Wasm function. The pointer is a
VMContext. - The pointer is used in the context of a host function with an environment. The pointer is a pointer to user data.
- The pointer is used in the context of a host function without an environment. The pointer is never used and we usually set it to null.
I don't think we need to represent the difference between 2 and 3 really, but I think the difference between 1 and 2 can be quite important. Please let me know if my mental model of this is not the same as yours!
There was a problem hiding this comment.
Your mental model matches mine. I don't know if that's a good news though 🤔.
😄
|
bors r+ |
|
Build succeeded: |
This makes the code more self documenting and correctly uses
unsafeto communicate that it's the caller's responsibility to ensure that the
code paths that can lead to the access can only write the value they
expect to be there.
spun off of #1739
vmctxandextra_datamight be good to unify this more.