Repository navigation
Problematic sentinel value with table.grow #40
Description
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:
-
Leave as is. Pro: symmetry with
memory.grow; cons: quite a wart and sets a bad precedent. -
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
-
Use zero as a sentinel value. Pro: less arbitrary, in practice you never want to grow by 0; cons: still a bit hacky.
-
Always return previous size. Pro: no sentinel; cons: checking for error (via comparing with
table.sizeafterwards) requires more work and is potentially racy in a future with shared tables. -
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 -
Use multiple results, success status + old size. Pro: serves all purposes; cons: more complex, creates dependency on multi-value proposal
-
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.
Activity
AndrewScheidecker commentedon Apr 23, 2019
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.growto yield 2 results in this proposal.lars-t-hansen commentedon Apr 23, 2019
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 commentedon Apr 23, 2019
@lars-t-hansen, that's a web-specific limit, not one inherent to Wasm per se.
lars-t-hansen commentedon Apr 23, 2019
OK, fair point about the web embedding.
The size of a table is limited to what
table.sizecan return, ie, 2^32-1. The only timetable.growcan 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 commentedon Apr 23, 2019
I prefer (1) or (6) (w/ @AndrewScheidecker's modification). Mostly because I prefer the symmetry w/
memory.growand want to avoid raciness.titzer commentedon Apr 29, 2019
rossberg commentedon May 2, 2019
@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 commentedon Jan 16, 2020
The majority voice seems to be to leave the wart alone, so I'm closing this.