component-model icon indicating copy to clipboard operation
component-model copied to clipboard

proposal: limit flags to 64 labels

Open ydnar opened this issue 1 year ago • 9 comments

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 avatar Jun 19 '24 17:06 ydnar

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?

lukewagner avatar Jun 20 '24 00:06 lukewagner

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

ydnar avatar Jun 20 '24 03:06 ydnar

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).

lukewagner avatar Jun 20 '24 13:06 lukewagner

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.

alexcrichton avatar Jun 20 '24 18:06 alexcrichton

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?

ydnar avatar Jun 20 '24 22:06 ydnar

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.

alexcrichton avatar Jun 20 '24 22:06 alexcrichton

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.

lukewagner avatar Jun 24 '24 17:06 lukewagner

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.

pchickey avatar Jun 24 '24 17:06 pchickey

I've prototyped this at https://github.com/bytecodealliance/wasm-tools/pull/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.

alexcrichton avatar Jun 24 '24 18:06 alexcrichton

This change will be released with Wasmtime 23 happening this weekend. (gating to 32 flags by default). There's an opt-in to enable the old support for 33+ flags if it's encountered though.

alexcrichton avatar Jul 16 '24 20:07 alexcrichton

PR to update Binary.md/CanonicalABI.md to match in #379.

lukewagner avatar Jul 17 '24 19:07 lukewagner