Add support for the stack switching proposal #1802
Conversation
dhil
force-pushed
the
stack-switching
branch
from
cca4014
to
088af87
Compare
There was a problem hiding this comment.
Thanks so much again for working on this here and being willing to rebase your work while it was in development, it's much appreciated! I've left an initial round of comments below but I'll want to dig in more later to some of your concerns around duplication of function type subtyping as well after they're handled. Feel free to push back on anything I'm saying below as well that doesn't make sense too
| 0x01 => Handle::OnSwitch { | ||
| tag: reader.read_var_u32()?, | ||
| }, | ||
| x => return reader.invalid_leading_byte(x, "on"), |
There was a problem hiding this comment.
Mind filling out "on" here?
| // Note that this is encoded as a signed type rather than unsigned | ||
| // as it's decoded as an s33 | ||
| i64::from(ty.0).encode(self.bytes); |
There was a problem hiding this comment.
Was this perhaps copy/paste from elsewhere? Or required by the proposal? (ideally this is just ty.0.encode(self.bytes)
|
|
||
| impl<'a> FromReader<'a> for ContType { | ||
| fn from_reader(reader: &mut BinaryReader<'a>) -> Result<Self> { | ||
| let idx = match u32::try_from(reader.read_var_s33()?) { |
There was a problem hiding this comment.
Similar to above in wasm-encoder this'll need updating if it's actually u32 instead of s33
| return Err(BinaryReaderError::new( | ||
| crate::validator::format!("non-function type {}", id.index()), | ||
| offset, | ||
| )) |
There was a problem hiding this comment.
Could this use bail! to return the error here?
| UnpackedIndex::Id(id) => id, | ||
| UnpackedIndex::Module(idx) => self.type_id_at(idx, offset)?, |
There was a problem hiding this comment.
At this point this should always be UnpackedIndex::Id and never UnpackedIndex::Module, so could that be asserted/unwrapped here?
| skipcnt1: usize, | ||
| ts1: &[ValType], | ||
| skipcnt2: usize, | ||
| ts2: &[ValType], |
There was a problem hiding this comment.
Instead of taking a skipcntN could this only take two slices? It should be possible to chop off the front of the slices in callers explicitly if necessary
| /// Returns the `SubType` associated with the given core type id. | ||
| fn sub_type_at_id(&self, id: CoreTypeId) -> Option<&SubType>; |
There was a problem hiding this comment.
Unlike the other method in this trait which are called with possibly-arbitrary indices we know that CoreTypeId is always trusted so it's ok to return &SubType from this instead of Option<_>
| fn cont_type_at(&self, key: TypeKey) -> Result<&ContType> { | ||
| let sub_ty = match key { | ||
| Either::A(at) => self.sub_type_at(at)?, | ||
| Either::B(id) => match self.resources.sub_type_at_id(id) { | ||
| None => bail!(self.offset, "unknown type {}", TypeIdentifier::index(&id)), | ||
| Some(sub_ty) => sub_ty, | ||
| }, | ||
| }; | ||
| if let CompositeInnerType::Cont(cont_ty) = &sub_ty.composite_type.inner { | ||
| if self.inner.shared && !sub_ty.composite_type.shared { | ||
| bail!( | ||
| self.offset, | ||
| "shared continuations cannot access unshared continuations", | ||
| ); | ||
| } | ||
| Ok(cont_ty) | ||
| } else { | ||
| bail!( | ||
| self.offset, | ||
| "non-continuation type {}", | ||
| match key { | ||
| Either::A(at) => at as usize, | ||
| Either::B(id) => TypeIdentifier::index(&id), | ||
| } | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
I might recommend splitting this into cont_type_at which takes a u32 and const_type_at_id which takes a CoreTypeId. I think that might be a bit easier to work with instead of working with Either perhaps? Functions like func_type_of_cont_type_at below could take &ContType instead of TypeKey in that case to let the caller call the appropriate const_type_at* function and then pass the result into later parts of validation.
| let sub_ty = match key { | ||
| Either::A(at) => self.sub_type_at(at)?, | ||
| Either::B(id) => match self.resources.sub_type_at_id(id) { | ||
| None => bail!(self.offset, "unknown type {}", TypeIdentifier::index(&id)), |
There was a problem hiding this comment.
Here we won't want to surface TypeIdentifier::index since that's just a purely internal index during validation. In these cases it's ok to jus say "unknown type" or "non-continuation type" or something like that without mentioning the index.
| fn func_type_of_cont_type_at(&self, key: TypeKey) -> Result<&'resources FuncType> { | ||
| let cont_ty = self.cont_type_at(key)?; | ||
| match cont_ty.0.as_core_type_id() { | ||
| None => bail!(self.offset, "invalid cont type"), | ||
| Some(func_id) => match self.resources.sub_type_at_id(func_id) { | ||
| None => bail!( | ||
| self.offset, | ||
| "unknown type {}", | ||
| TypeIdentifier::index(&func_id) | ||
| ), | ||
| Some(sub_ty) => { | ||
| if let CompositeInnerType::Func(func_ty) = &sub_ty.composite_type.inner { | ||
| if self.inner.shared && !sub_ty.composite_type.shared { | ||
| bail!( | ||
| self.offset, | ||
| "shared functions cannot access unshared functions", | ||
| ); | ||
| } | ||
| Ok(&func_ty) | ||
| } else { | ||
| bail!( | ||
| self.offset, | ||
| "non-function type {}", | ||
| TypeIdentifier::index(&func_id) | ||
| ) | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function, if it takes &ContType, I believe can be infallible (e.g. unwraps/expects/etc). The validation that ContType always points to a function should have already been validated when the type was added to the module, so here's it's safe to assume that invariant is always upheld.
This patch implements the stack switching proposal (which at the time of writing is a phase 2 proposal).
The proposal adds a new composite type:
cont $ftindexed by a function type, which is the type of a continuation.Two new abstract continuation heap types and their subtyping hierarchy are added:
contthe top type;nocontthe bottom type, i.e.nocont <: cont.Six new instructions:
cont.new $ct, parameterised by a continuation type index, which creates a new continuation.cont.bind $ct-0 $ct-1, parameterised by two continuation type indices, which partially applies a continuation of type$ct-0, resulting in a continuation of type$ct-1.suspend $tag, parameterised by a tag, which suspends the current context.resume $ct (on $tag $label)*, parameterised by a continuation type index and a jump table which maps tags to labels (similar to how(catch $tag $label)works in the exception handling proposal); this instruction resumes a continuation.resume_throw $ct $tag (on $tag-1 $label)*, parameterised by both a continuation type index and a tag index (in addition to the jump table). This instruction injects the exception tag$taginto the continuation argument.switch $ct $tag, parameterised by a continuation type index and a tag index. This instruction performs a direct context switch to a given continuation (i.e. a symmetric transfer of control).The wellformedness condition for tags is relaxed, such that they are allowed to have a non-empty result type. This result type is used to validate the types of return values of
suspend $tag.The features of the proposal are gated under a new feature flag
stack-switching(off by default). Most of the additions are test artifacts. The implementation of the proposal itself is modest.There are a few things in this patch that I am not quite sure about:
UnpackedIndexin the representation of the new indexed continuation type. In the validator I sometimes have to work withCoreTypeIdand other times with rawu32indices. I am not convinced this is the best approach.cont.bindand the jump tables (aka resume tables) onresumeandresume_throw, I have to construct and manipulate the underlying function types of continuations, in particular, I have to check subtyping relations between parts of these function types (c.f.cont.bindwhere I have to drop a prefix of the parameter list). So I have had to essentially reimplement function subtype checking. I have done this via a functionis_subtype_manythat applies theis_subtypefunction pointwise to two equally sized collections. I am not particularly fond of this approach.TagKindstructure. Though, an argument can be made for adding a further kind, lets call itControl, to differentiate exception and control tags internally. But I am not really sure whether the distinction is helpful.