CABI mismatch when no parameters or results

[lukewagner]

In the general case, an async-lowered import has two core wasm params: one for the params, one for the results. But, at least as currently described in CanonicalABI.md as well as this section in the Async explainer, when there are no parameters, the first core wasm param is elided, and when there are no results, the second core wasm core wasm param is elided, such that there can be 0, 1 or 2 core wasm params. (Also, if there is only 1 flattened core parameter, that flattened core type, whatever it is, is used and the parameter is passed by value instead of by pointer.)

At the moment, based on my incomplete testing, it looks like wasip3-prototyping always takes 2 parameters, so it seems like we should reconcile the two, one way or another.

Also, fwiw, there is an open issue asking whether we could extend the abovementioned flattened-param optimization to >1 core params.

WDYT?

[alexcrichton]

Oh I'm glad you caught this! It's expected yeah that everywhere currently requires 2 parameters, but only insofar as I know everything is consistent internally but I would like to get it consistent with the spec! I'd agree that if we can using direct results would be nice, so that seems like a reasonable change to make at this time as well.

That being said changing ABIs is gnarly and this'll probably require some time to implement and percolate, there's a lot of places to update

[lukewagner]

(sorry, getting back to this)

Updating the spec to match the implementation seems worth considering. Of the various cases optimized, removing unused i32 params and passing non-i32s by value instead of by reference seems negligible, so always having the lowered type be (func (param i32 i32) (result i32)) seems like a good idea. Passing a flattened i32 parameter by value instead of by reference seems like a nice optimization, although it makes me wonder if it'll end up being a likely source of confusion that'll cause headache. If we allowed passing the first N by value (instead of just 1, as CM/#434 suggests) that would perhaps catch the issue quicker and also make the optimization more worthwhile, but that's an even bigger change. So unless we want to do that, then I think it'd make sense to always pass params by pointer, unconditionally.

[alexcrichton]

Personally I wouldn't place too much weight on the existing implementation, it's early enough that we should prioritize spec-level consistency IMO.

Handling the signature of lifted/lowered async functions currently has special cases to handle async-lowered imports, while otherwise the only distinction is async-lifted exports which have different handling of results. From that perspective it's arguably a simplification to handle arguments to imports the same way, regardless of whether the import is async lowered or not. We'd still have the MAX_FLAT_PARAMS value to limit the number of arguments and cap the amount of data an engine would have to schlep around.

To play both sides though, here's a reason to not change things. Currently read/write intrinsics for futures/streams take a pointer parameter even when the future/stream has no payload, and this is arguably more consistent with that. Additionally writing a value into a future is done by-pointer rather than by-value. Now arguably we could change all that too though perhaps, but it does mean that spec-wise being internally consistent may require more changes?

[lukewagner]

That's a good point regarding consistency with future/stream ABIs. Using MAX_FLAT_PARAMS would be nice and consistent, but I worry that it would require each waitable table element to have 16*8 bytes of inline buffer space (or otherwise have to resort to a more dynamic allocation scheme to store intermediate flat values). We could use a smaller maximum (like 2 or 4), but then there's less consistency.

Based on that, I'm leaning towards the current impl behavior (always two i32 pointers). Since the transition to the lazy lowering ABI is our chance to making a breaking ABI change (and adopt multi-return), we could reconsider this choice as part of that (and based on real workloads). WDYT?

[alexcrichton]

Sounds reasonable to me 👍

[lukewagner]

Working through the spec, I remembered that one place where we currently pass "by flat value" for async lowering is async resource.drop (which is sorta defined in terms of canon lower-ing a function of type func(h: u32)). Adding a pointer indirection to resource.drop seems unnecessarily wasteful/weird, so it should probably stay as-is. If we're going for consistency, then it wouldn't be the worst thing to change future to take flattened values instead of always taking a pointer. (For stream, I think always taking a (pointer,length) is consistent with list always taking a (pointer,length).) If we did this, though, I think we shouldn't use MAX_FLAT_PARAMS (for the inline-storage reason noted above) but instead define a MAX_ASYNC_PARAMS = 2 (or maybe 4). WDYT?

[alexcrichton]

Resolved in #171