near-vm: remove dynamically typed APIs from near_vm #9214
Conversation
Oringally these have existed to enable switching out the backends in wasmer, however we kept only just the Universal* backend for near-vm, so this dynamicism is unnecessary and overcomplicates the code for no reason (as evidenced by the fact that most of the relevant changes are in the tests!)
nagisa
requested review from
nikurt,
aborg-dev,
jakmeier and
Ekleog-NEAR
and removed request for
nikurt
|
This partially extracts near/wasmer#125 |
|
Note that I chose to not modify code to address Leo’s concerns about soundness of |
| @@ -86,7 +86,8 @@ impl dyn Artifact { | |||
| /// Downcast a dynamic Executable object to a concrete implementation of the trait. | |||
| pub fn downcast_arc<T: Artifact + 'static>(self: Arc<Self>) -> Result<Arc<T>, Arc<Self>> { | |||
| if std::any::TypeId::of::<T>() == Artifact::type_id(&*self, private::Internal(())) { | |||
| // SAFETY: err, its probably sound, we effectively construct a transmute here. | |||
| // SAFETY: The data pointer is the same between the dynamically and statically typed | |||
| // `Arc`s. | |||
There was a problem hiding this comment.
Technically Artifact would need to be an unsafe trait for that statement to have its prerequisites validated… probably not worth the hassle, but maybe add a comment here and one close to the #[doc(hidden)] fn type_id(); in the trait so other people don’t blindly trust the comment and make sure to not override type_id()?
(Also, this relies on the memory layout of fat pointers starting with the data pointer and not with the vtable pointer, but I’d guess this is probably going to stay true even if multi-vtable pointers were to ever see the day)
There was a problem hiding this comment.
As noted in the previous PR, the trait cannot be implemented by external users because private::Internal that it takes as an argument is, well, not public. So you can’t really re-define the type_id function. Is your concern that the near-vm-vm itself implements this trait and does something funky with it?
Also, this relies on the memory layout of fat pointers starting with the data pointer and not with the vtable pointer, but I’d guess this is probably going to stay true even if multi-vtable pointers were to ever see the day
The reliance on the memory layout would be true if this attempted to transmute or something of the similar sort. However Arc::into_raw is guaranteed (though not directly documented) to return the data pointer, and Arc::from_raw is guaranteed to construct an Arc with the argument as a data pointer, regardless of what types are involved, so no implicit reliance on the shape of fat pointers exists here.
That said, it looks like I’m not convincing you here, so I guess I’ll look into removing near-vm/api outright.
There was a problem hiding this comment.
Oh I had forgotten about private::Internal, sorry about that! Then yes it makes sense to me too that the concern is unwarranted.
(That said, I still think that into_raw/from_raw would fail if the order of data and vtable pointers were to be switched in a fat pointer, but it’s a concern that’s only theoretical and doesn’t deserve much thought as I don’t think it’ll ever change either — if it were, we’d have Arc(dyn Trait) at position P, so Arc::into_raw would return *mut dyn Trait P+8, then casting to T: Trait would return P+16, and trying to Arc::from_raw would then build an Arc where the ArcInner would be the vtable pointer at P+8 instead of the actual pointer at P. But who cares anyway, this order will certainly never change.)
So I’m good with merging this, though it’d be even better if we could remove all of near-vm/api altogether :)
| pub fn downcast_arc<T: Engine + 'static>(self: Arc<Self>) -> Result<Arc<T>, Arc<Self>> { | ||
| if std::any::TypeId::of::<T>() == Engine::type_id(&*self, private::Internal(())) { | ||
| // SAFETY: The data pointer is the same between the dynamically and statically typed | ||
| // `Arc`s. |
Oringally these have existed to enable switching out the backends in wasmer, however we kept only just the Universal* backend for near-vm, so this dynamicism is unnecessary and overcomplicates the code for no reason (as evidenced by the fact that most of the relevant changes are in the tests!)