Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58): Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58):cfallin opened PR #5023 from fix-struct-ret to main:
The legalization of
StructReturnwas causing issues in the new call-handling code: theStructReturnret was included in theSigDataas if it were an actual CLIF-level return value, but it is not.Prior to using regalloc constraints for return values, we unconditionally included rax (or the architecture's usual return register) as a def, so it would be properly handled as "clobbered" by the regalloc. With the new scheme, we include defs on the call only for CLIF-level outputs. Callees with
StructReturnargs were thus not known to clobber the return-value register, and values might be corrupted.This PR updates the code to include a
StructReturnret as a clobber rather than a returned value in the relevant spots. I observed it causing saves/restores of rax in some CLIF that @bjorn3 provided me, but I was having difficulty minimizing this into a test-case that I would be comfortable including as a precise-output case (including the whole thing verbatim would lock down a bunch of other irrelevant details and cause test-update noise later). If we can find a more minimized example I'm happy to include it as a filetest.Fixes? #5018 (cc @bjorn3 to confirm?).
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58):cfallin requested elliottt for a review on PR #5023.
Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58):cfallin requested jameysharp for a review on PR #5023.
Wasmtime GitHub notifications bot (Oct 05 2022 at 20:52):elliottt created PR review comment:
How do you feel about changing the loop bounds to
sigdata.num_rets()-num_rets .. sigdata.num_rets()?
Wasmtime GitHub notifications bot (Oct 05 2022 at 20:52):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2022 at 03:33):cfallin updated PR #5023 from fix-struct-ret to main.
Wasmtime GitHub notifications bot (Oct 06 2022 at 03:43):cfallin updated PR #5023 from fix-struct-ret to main.
Wasmtime GitHub notifications bot (Oct 06 2022 at 03:45):cfallin created PR review comment:
Yup, much better!
Wasmtime GitHub notifications bot (Oct 06 2022 at 03:46):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2022 at 18:35):cfallin has marked PR #5023 as ready for review.
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:13):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:13):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:13):elliottt created PR review comment:
Did this have to be named again to avoid holding a reference to self through the body of the loop?
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:18):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:18):cfallin created PR review comment:
Yep, exactly, this was a borrow-checker appeasement measure. I'll add a comment to that effect.
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:21):cfallin updated PR #5023 from fix-struct-ret to main.
Wasmtime GitHub notifications bot (Oct 06 2022 at 22:22):cfallin has enabled auto merge for PR #5023.
Wasmtime GitHub notifications bot (Oct 07 2022 at 00:14):cfallin merged PR #5023.
Last updated: Nov 22 2024 at 16:03 UTC