Wasmtime GitHub notifications bot (Jul 26 2022 at 18:39): Wasmtime GitHub notifications bot (Jul 26 2022 at 18:39):alexcrichton opened PR #4534 from translate-variant to main:
This commit implements the most general case of variants for fused
adapter trampolines. Additionally a number of other primitive types are
filled out here to assist with testing variants. The implementation
internally was relatively straightforward given the shape of variants,
but there's room for future optimization as necessary especially around
converting locals to various types.This commit also introduces a "one off" fuzzer for adapters to ensure
that the generated adapter is valid. I hope to extend this fuzz
generator as more types are implemented to assist in various corner
cases that might arise. For now the fuzzer simply tests that the output
wasm module is valid, not that it actually executes correctly. I hope to
integrate with a fuzzer along the lines of #4307 one day to test the
run-time-correctness of the generated adapters as well, at which point
this fuzzer would become obsolete.Finally this commit also fixes an issue with
u8translation where
upper bits weren't zero'd out and were passed raw across modules.
Instead smaller-than-32 types now all mask out their upper bits and do
sign-extension as appropriate for unsigned/signed variants.<!--
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 (Jul 26 2022 at 18:39):alexcrichton requested fitzgen for a review on PR #4534.
Wasmtime GitHub notifications bot (Jul 26 2022 at 18:39):alexcrichton has marked PR #4534 as ready for review.
Wasmtime GitHub notifications bot (Jul 26 2022 at 19:45):alexcrichton updated PR #4534 from translate-variant to main.
Wasmtime GitHub notifications bot (Jul 26 2022 at 19:55):alexcrichton updated PR #4534 from translate-variant to main.
Wasmtime GitHub notifications bot (Jul 26 2022 at 21:00):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 26 2022 at 21:00):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 26 2022 at 21:00):fitzgen created PR review comment:
Might not want to
derive(Arbitary)forFuncTypeas theparamsare potentially unbounded in length here, and I assume we have an implementation limit we want to stay below, right?
Wasmtime GitHub notifications bot (Jul 26 2022 at 21:00):fitzgen created PR review comment:
Alternatively, you could add a wrapper type for bounding
Veclengths similar to what you did forNonZeroLenVec.
Wasmtime GitHub notifications bot (Jul 26 2022 at 21:00):fitzgen created PR review comment:
Huh. I didn't realize there wasn't an
i32.extend8_uinstruction. Strange there are signed extension instructions where there aren't corresponding unsigned variants. Yeah you can just do the masking, but also you can do the signed version via signed shifting back and forth. So why not omit or include both?@sunfishcode do you happen to know the answer to the above, out of curiosity?
Wasmtime GitHub notifications bot (Jul 26 2022 at 21:00):fitzgen created PR review comment:
Why not add this to the top level
wasmtime/fuzz/*fuzz targets? You don't think it is worth spending oss-fuzz time on until we get the full blown value round tripping stuff working?
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:10):alexcrichton created PR review comment:
Yeah I don't think this really all that high value of a fuzz target since it's purely "is it a valid wasm module" which is a much weaker claim than "hey it actually works". Eventually when we're actually running these I think it makes sense to run on oss-fuzz but for now this is mostly just a development aide which is intended to help suss out corner cases (worked really well for a memory64 patch I'll post after this)
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:10):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:11):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:11):alexcrichton created PR review comment:
While we do have limits in the system they're generally elsewhere at the wasmparser validation layer or the runtime implementation layer. Here in this specific compiler there are few limits imposed and at least so far nothing has blown up. If I run into issues locally with this though running the fuzzer I'll probably do the newtype trick.
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:13):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:13):alexcrichton created PR review comment:
Double-checking again to make sure I didn't miss something, apparently there's
i64.extend32_ubut that's the only one.
Wasmtime GitHub notifications bot (Jul 27 2022 at 14:14):alexcrichton merged PR #4534.
Last updated: Nov 22 2024 at 16:03 UTC