Stream: git-wasmtime

Topic: wasmtime / PR #5023 Fix StructReturn handling: properly m...


view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58):

cfallin opened PR #5023 from fix-struct-ret to main:

The legalization of StructReturn was causing issues in the new call-handling code: the StructReturn ret was included in the SigData as 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 StructReturn args were thus not known to clobber the return-value register, and values might be corrupted.

This PR updates the code to include a StructReturn ret 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58):

cfallin requested elliottt for a review on PR #5023.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 19:58):

cfallin requested jameysharp for a review on PR #5023.

view this post on Zulip 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()?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 20:52):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 03:33):

cfallin updated PR #5023 from fix-struct-ret to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 03:43):

cfallin updated PR #5023 from fix-struct-ret to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 03:45):

cfallin created PR review comment:

Yup, much better!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 03:46):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 18:35):

cfallin has marked PR #5023 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 22:13):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 22:13):

elliottt submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 22:18):

cfallin submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 22:21):

cfallin updated PR #5023 from fix-struct-ret to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 22:22):

cfallin has enabled auto merge for PR #5023.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2022 at 00:14):

cfallin merged PR #5023.


Last updated: Dec 23 2024 at 12:05 UTC