jameysharp commented on issue #5319:
Ooh, I missed an important detail!
args
andrets
are both in the sameabi_args
array, so the first one doesn't necessarily start at 0.
jameysharp commented on issue #5319:
More precisely,
SigSet::abi_args
is laid out with each signature havingrets
first and thenargs
. This is ensured byfrom_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 previousSigData
.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 oneu32
offset in eachSigData
, but I don't think we should try to do that until we've fixed this version using twou32
offsets. Anyway, if I'm counting right, reducing au32
to au16
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 theargs
andrets
methods rely on the order in which they're added toabi_args
; and add a comment to theargs
andrets
methods saying to seefrom_func_sig
for details.
tnachen commented on issue #5319:
@jameysharp updated!
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: Dec 23 2024 at 12:05 UTC