Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: limit flags to 64 labels #370

Open
ydnar opened this issue Jun 19, 2024 · 9 comments
Open

proposal: limit flags to 64 labels #370

ydnar opened this issue Jun 19, 2024 · 9 comments

Comments

@ydnar
Copy link

ydnar commented Jun 19, 2024

The current specification for flags types allows an arbitrary (unlimited) number of labels.

For languages without a bit vector types or operator overloading (like Go), flags types with <= 64 labels can be represented as an unsigned integer type u8, u16, u32, u64. Flags can be composed with simple boolean operations, e.g.: f |= FlagFoo. For flags types with > 64 labels, the representation and user-facing API must change to a less ergonomic form. This can be seen in the Canonical ABI flattening rules for flags types, which require lowering into a sequence of i32.

If there exist limited or no current or practical applications for flags with > 64 labels, I propose we constrain flags types to at most 64 labels, and change the Canonical ABI form for types with > 32 and <= 64 labels to an i64.

Would changing the flattening rules be a breaking ABI change?

@ydnar ydnar changed the title proposal: limit flags to 64 cases proposal: limit flags to 64 labels Jun 19, 2024
@lukewagner
Copy link
Member

Adding an implementation limit of 32 flags makes sense to me, since we could relax it later and there's a good chance noone is actually using >32 flags. Changing the current flattening rules for the [32, 64]-flag case seems like maybe more of a recipe for bugs and accidental incompatibility in the rare case that anyone uses [32, 64] flags. Perhaps then later, when we relax the flag limit based on motivating use cases, we could relax it to a different ABI than is currently proposed. WDYT?

@ydnar
Copy link
Author

ydnar commented Jun 20, 2024

Sure. Is there a mechanism to solicit feedback from community on a change like this?

@lukewagner
Copy link
Member

Beyond discussing it in this repo, for a tweak like this, I suppose the best way to get additional feedback is to land the change and see if anything breaks (that's what nightlies and previews are for).

@alexcrichton
Copy link
Collaborator

Bindgen for Rust panics for >128 flags but that's only because u128 is convenient in Rust. Bindgen for C already panics if there's more than 64 flags, but again that's only because 64-bits is easy to support. That's to say I'd be in favor of limiting the maximal number of flags definitely to 64, possibly to 32 too. Languages generally don't have a great way of representing >=64 flags for sure. In the long term given that wasm has a primitive i64 datatype I think it's best to cap the flags at 64 and use an i64 as the lowering when there's >32 flags.

As to how to get to that world since it's naturally a breaking change from today I'm not so sure. I wouldn't say that we have a great nigthly-style channel to see the breakage before it happens, folks tend to just get broken and not be able to upgrade a dependency they're using. The best way forward might be to implement a warning in WIT pointing to this issue if folks have >32 flags and if no one comments for awhile we can assume it never comes up and then clamp to 32 with the option of increasing to 64 in the future.

@ydnar
Copy link
Author

ydnar commented Jun 20, 2024

If there is currently no implementation in the wild with (32,64] labels, then changing the flatting rules to an i64 seems possible in the short term?

@alexcrichton
Copy link
Collaborator

Yeah I'd agree with that myself. I'd be ok either being conservative and limiting it at 32 for a bit or just going straight to 64 with an unconditional single-element flattening.

@lukewagner
Copy link
Member

It's quite possible that literally noone is using >32 flags, so perhaps we could get away with going straight to the goal state without breaking anyone.

@pchickey
Copy link

I also suspect that noone is using >32 flags. Lets push out this restriction in wasm-tools's validator, and if nobody complains within a month or so, we have confirmed its true.

alexcrichton added a commit to alexcrichton/wasm-tools that referenced this issue Jun 24, 2024
This commit is an attempt to explore the design space of
WebAssembly/component-model#370. This limits, by default, the number of
flags in a component model type to 32 by default. The hope of this issue
is to be able to ratchet the maximum number of flags to make it easier
on bindings generators to not need to work with arbitrary numbers of
flags. The secondary hope is that we can ratchet flags straight to 32
instead of 64 due to it being unlikely that more than 32 flags are in
use. Once this percolates there can then be a separate feature for
enabling 33-64 flags.
@alexcrichton
Copy link
Collaborator

I've prototyped this at bytecodealliance/wasm-tools#1635. I'll reiterate that we don't have a great staging mechanism for things like this, so my current plan is:

  • Turn off support for 33+ flags by default.
  • Add a flag to enable support for 33+ flags using the current ABI (sequences of 32-bit values)
  • Document that if anyone enables the flag to please leave a comment on this issue

If no one comments then we're then in a free spot to change things without breaking anyone since no one has 33+ flags. At that point we can change the canonical ABI to allow up to 64 flags and use a single i64 to represent them in flat lowering.

If someone comments then I'm hopeful we can find a workaround in the meantime, but that highly depends on specifics.

github-merge-queue bot pushed a commit to bytecodealliance/wasm-tools that referenced this issue Jun 25, 2024
* Limit component model flags to 32

This commit is an attempt to explore the design space of
WebAssembly/component-model#370. This limits, by default, the number of
flags in a component model type to 32 by default. The hope of this issue
is to be able to ratchet the maximum number of flags to make it easier
on bindings generators to not need to work with arbitrary numbers of
flags. The secondary hope is that we can ratchet flags straight to 32
instead of 64 due to it being unlikely that more than 32 flags are in
use. Once this percolates there can then be a separate feature for
enabling 33-64 flags.

* Drop a link
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Jun 27, 2024
This notably brings in a limitation where component model flags types
must have 32 or fewer flags in accordance with the transition plan of
WebAssembly/component-model#370. A feature
flag is added to go back to the previous behavior to avoid breaking
anyone too much.

This additionally brings in a fix for a panic when validating invalid
modules with tail calls.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Jun 27, 2024
This notably brings in a limitation where component model flags types
must have 32 or fewer flags in accordance with the transition plan of
WebAssembly/component-model#370. A feature
flag is added to go back to the previous behavior to avoid breaking
anyone too much.

This additionally brings in a fix for a panic when validating invalid
modules with tail calls.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Jun 27, 2024
This notably brings in a limitation where component model flags types
must have 32 or fewer flags in accordance with the transition plan of
WebAssembly/component-model#370. A feature
flag is added to go back to the previous behavior to avoid breaking
anyone too much.

This additionally brings in a fix for a panic when validating invalid
modules with tail calls.
github-merge-queue bot pushed a commit to bytecodealliance/wasmtime that referenced this issue Jul 8, 2024
* Update the wasm-tools family of crates

This notably brings in a limitation where component model flags types
must have 32 or fewer flags in accordance with the transition plan of
WebAssembly/component-model#370. A feature
flag is added to go back to the previous behavior to avoid breaking
anyone too much.

This additionally brings in a fix for a panic when validating invalid
modules with tail calls.

* Add vet entries
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Jul 8, 2024
* Update the wasm-tools family of crates

This notably brings in a limitation where component model flags types
must have 32 or fewer flags in accordance with the transition plan of
WebAssembly/component-model#370. A feature
flag is added to go back to the previous behavior to avoid breaking
anyone too much.

This additionally brings in a fix for a panic when validating invalid
modules with tail calls.

* Add vet entries
alexcrichton added a commit to bytecodealliance/wasmtime that referenced this issue Jul 8, 2024
* Update the wasm-tools family of crates

This notably brings in a limitation where component model flags types
must have 32 or fewer flags in accordance with the transition plan of
WebAssembly/component-model#370. A feature
flag is added to go back to the previous behavior to avoid breaking
anyone too much.

This additionally brings in a fix for a panic when validating invalid
modules with tail calls.

* Add vet entries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants