search

WebAssembly / stack-switching

A repository for the stack switching proposal.
Other
146 stars 13 forks source link

[Binary format] Encode continuation type index as an `s33` #89

Closed dhil closed 1 month ago

dhil commented 2 months ago

There is a small inconsistency between the reference implementation in #84 and the explainer. In the explainer we write that the type index $ft on cont $ft should be encoded as an u32 in the binary format. However, the reference interpreter actually encodes it as s33, similarly to how concrete heap types are encoded.

I do not have a strong preference for either encoding, but I suppose one can argue that the s33 approach is more forward compatible in the sense that we can encode additional metadata.

tlively commented 2 months ago

I think typeidx makes more sense here, since it is supposed to be the index of a type. Tags also use typeidx in a very similar situation. If we need to encode more information in the future, we can use a different introductory encoding, just like we do for shared heap types, for example. s33 (or rather heaptype) would only make sense if we were going to allow abstract heap types like func in this location, but that's not the case.

rossberg commented 2 months ago

@tlively, tag types have a reserved zero byte as an extension point. Of course, we could do the same here. The space for type codes is already becoming somewhat crowded, so we may want to avoid more duplicate/flattened type codes where we can.

dhil commented 2 months ago

FWIW, I used the s33 encoding in the implementation in wasm-tools (https://github.com/bytecodealliance/wasm-tools/pull/1802) to stay in sync with the reference interpreter (which I currently use as an assembler for my experiments).

tlively commented 1 month ago

I have a slight preference for adding the zero byte for future extension, but I wouldn't want to block progress on this.

slindley commented 1 month ago

@tlively how about we go ahead and merge this PR in order to maintain consistency between the spec and the various implementations, but also create an issue so that we don't forget to return to this question later?

tlively commented 1 month ago

Sounds perfect, thanks!

dhil commented 1 month ago

@tlively how about we go ahead and merge this PR in order to maintain consistency between the spec and the various implementations, but also create an issue so that we don't forget to return to this question later?

I've opened issue #97 to track this question.