Stream: git-wasmtime

Topic: wasmtime / PR #8243 Migrate all Winch filetests to `tests...


view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):

alexcrichton requested saulecabrera for a review on PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):

alexcrichton requested elliottt for a review on PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):

alexcrichton opened PR #8243 from alexcrichton:migrate-winch-tests to bytecodealliance:main:

This commit builds on https://github.com/bytecodealliance/wasmtime/pull/8237 to migrate all Winch filetests to the top-level tests/disas/ test suite. All files are copied to a tests/disas/winch folder.

This PR has a number of commit splitting out the various stages of this migration. The main changes are:

The next follow-up I'd like to do is to remove the filetest infrastructure for Winch, which will probably also remove the Winch executable itself. I'll do that as a follow-up though.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):

alexcrichton requested fitzgen for a review on PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 17:57):

elliottt commented on PR #8243:

One change that we made to the winch disasm tests that I liked was to only print offsets after branch instructions (and always after a return). This made updates to filetests easier to read, as the diff was usually more precise. Would that be easy to add back in before this migration, or should we do that as a separate update?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 19:22):

alexcrichton commented on PR #8243:

I saw that yeah from Winch but I ended up leaving it out in favor of printing all offsets. I was a bit confused as to why the address after a jump instruction was printed because that didn't account for fall-through-style jumps (e.g. it didn't reliably catch all jump targets). For after-ret I was also not sure that would handle the case of multiple ret instructions in a function.

I can add it back in though if desired!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 19:36):

saulecabrera submitted PR review:

Looks good to me, thanks! I'd be in favor of considering adding back the file test formatting; even though not all cases are covered (as you've pointed out) the original motivation that Trevor and I discussed was to make any diffs easier to read.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 19:36):

saulecabrera submitted PR review:

Looks good to me, thanks! I'd be in favor of considering adding back the file test formatting; even though not all cases are covered (as you've pointed out) the original motivation that Trevor and I discussed was to make any diffs easier to read.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 19:36):

saulecabrera created PR review comment:

                "The Winch calling convention is only implemented for x86_64 and aarch64"

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 21:30):

alexcrichton updated PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 21:31):

alexcrichton updated PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 21:32):

alexcrichton updated PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 21:32):

alexcrichton commented on PR #8243:

Ah ok that makes sense. I've updated with that added back in plus some comments

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 00:03):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 00:04):

elliottt commented on PR #8243:

Thanks for changing the output back!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 14:28):

alexcrichton updated PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 14:28):

alexcrichton has enabled auto merge for PR #8243.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 27 2024 at 15:10):

alexcrichton merged PR #8243.


Last updated: Dec 23 2024 at 12:05 UTC