Problematic sentinel value with table.grow

[rossberg]

Surprise, using a sentinel value (-1) for memory.grow is coming back to bite us. For table.grow should preferably behave analogously. and the proposal currently says that table.grow also returns -1 (i.e., 2^32-1) in case of error. However, that does not really make sense, because we allow table sizes to span the entire u32 value range.

I can see these possible options:

1. Leave as is. Pro: symmetry with memory.grow; cons: quite a wart and sets a bad precedent.

2. Keep -1 but disallow 2^32-1 as a table size. Pro: symmetry with memory.grow; cons: weird discontinuity, arguably backwards as a "fix", technically a breaking change

3. Use zero as a sentinel value. Pro: less arbitrary, in practice you never want to grow by 0; cons: still a bit hacky.

4. Always return previous size. Pro: no sentinel; cons: checking for error (via comparing with table.size afterwards) requires more work and is potentially racy in a future with shared tables.

5. Return success status instead of previous size. Pro: simple and simplest to use; cons: getting old size would require separate call to table.size, which again is potentially racy

6. Use multiple results, success status + old size. Pro: serves all purposes; cons: more complex, creates dependency on multi-value proposal

7. Return an i64. Pro: solves problem, but only for the "wasm32" equivalent of tables; cons: does not avoid sentinel, coherence would suggest to make table size an i64 everywhere

@binji, @lukewagner, @lars-t-hansen, @titzer, I am probably leaning towards option 5 (Boolean result), but would like to hear your opinions. Is there a strong use case for getting the old size atomically? In general, I would be inclined to avoid sentinel values.

[AndrewScheidecker]

Regarding option 6: instead of adding a dependency on the multi-value proposal, this proposal could just clone the part of the multi-value proposal that generalizes instructions to yield more than one value; that's a simple and non-controversial part of that proposal, and all that's necessary for table.grow to yield 2 results in this proposal.

[lars-t-hansen]

So... we have a MaxTableLength of 1e7. I thought this limit was generally agreed upon since WebAssembly/spec#873. Doesn't that prevent this from being an issue?

[rossberg]

@lars-t-hansen, that's a web-specific limit, not one inherent to Wasm per se.

[lars-t-hansen]

OK, fair point about the web embedding.

The size of a table is limited to what table.size can return, ie, 2^32-1. The only time table.grow can be called on a table and return 2^32-1 and that can be not-an-error is when the table size is already 2^32-1 and the delta is zero.

My preference here would actually be to choose (1) because it fits the current design, and then, once we have multi-value and/or exceptions and/or shared tables, revisit both memory.grow and table.grow to see if there are more principled solutions.

In a similar but not quite analogous situation, we chose to trap (WebAssembly/threads#72). We could do that here too.

[binji]

I prefer (1) or (6) (w/ @AndrewScheidecker's modification). Mostly because I prefer the symmetry w/ memory.grow and want to avoid raciness.

[titzer]

I too prefer symmetry with memory.grow. The difference there is that the units are pages, and so far with memories limited to 32-bit sizes, that sentinel is OOB. I think (1) is my leading choice. Note that (6) is a bit weird because a memory.grow of 0, though it has no visible side effects in wasm, in the JS embedding, it still detaches a nonshared array buffer backing the memory. AFAIR that's how we ended up with memory.size, to discourage the slightly surprising effect of detachment.

…

On Tue, Apr 23, 2019 at 8:01 PM Ben Smith ***@***.***> wrote: I prefer (1) or (6) (w/ @AndrewScheidecker <https://github.com/AndrewScheidecker>'s modification). Mostly because I prefer the symmetry w/ memory.grow and want to avoid raciness. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC46VVGYG7ZSHC4FHSSPN2DPR5FHHANCNFSM4HHXB5SA> .

[rossberg]

@titzer, I'm not sure I understand your comment re option (6). It does not change the definition of what is a "successful" grow nor whether Wasm code can observe that result, so any such weirdness is no different than with the other options.

[rossberg]

The majority voice seems to be to leave the wart alone, so I'm closing this.