wasmtime / issue #5319 Remove sig data arg and ret fields... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #5319 Remove sig data arg and ret fields...

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:59):

Ooh, I missed an important detail! args and rets are both in the same abi_args array, so the first one doesn't necessarily start at 0.

Wasmtime GitHub notifications bot (Nov 23 2022 at 18:53):

jameysharp commented on issue #5319:

More precisely, SigSet::abi_args is laid out with each signature having rets first and then args. This is ensured by from_func_sig, which has a comment saying "Handle retvals first, because we may need to add a return-area arg to the args."

As a result,

self.args_start == self.rets_end.

self.rets_start == prev.args_end, or 0 if there is no previous SigData.

I believe that's the cause of the test failures. They were all off-by-one because the inputs that failed had exactly one return value.

I think this means we could save some more space by keeping one u16 length and one u32 offset in each SigData, but I don't think we should try to do that until we've fixed this version using two u32 offsets. Anyway, if I'm counting right, reducing a u32 to a u16 won't make the struct smaller right now because of padding and alignment, but it could help later combined with other changes.

I'm not totally happy with how confusing this will be for anyone trying to maintain this code in the future, but I think it's worth doing if we add some comments. In particular, extend the comment at the beginning of from_func_sig to mention that the args and rets methods rely on the order in which they're added to abi_args; and add a comment to the args and rets methods saying to see from_func_sig for details.

Wasmtime GitHub notifications bot (Nov 25 2022 at 17:40):

tnachen commented on issue #5319:

@jameysharp updated!

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:06):

jameysharp commented on issue #5319:

I believe this is correct and a good change, but it's tricky enough that I'd like to get a second review from either @cfallin or @fitzgen.

Last updated: Nov 22 2024 at 16:03 UTC