Implement variant translation in fused adapters #4534
Conversation
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 bytecodealliance#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 `u8` translation 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.
Currently memory64 isn't supported elsewhere in the component model implementation of Wasmtime but the trampoline compiler seems as good a place as any to ensure that it at least works in isolation. This plumbs through fuzz input into a `memory64` boolean which gets fed into compilation. Some miscellaneous bugs were fixed as a result to ensure that memory64 trampolines all validate correctly.
| //! A simple fuzzer for FACT | ||
| //! | ||
| //! This is an intentionally small fuzzer which is intended to only really be | ||
| //! used during the development of FACT itself when generating adapter modules. | ||
| //! This creates arbitrary adapter signatures and then generates the required | ||
| //! trampoline for that adapter ensuring that the final output wasm module is a | ||
| //! valid wasm module. This doesn't actually validate anything about the | ||
| //! correctness of the trampoline, only that it's valid wasm. |
There was a problem hiding this 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?
There was a problem hiding this 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)
|
|
||
| #[derive(Arbitrary, Debug)] | ||
| struct FuncType { | ||
| params: Vec<ValType>, |
There was a problem hiding this comment.
Might not want to derive(Arbitary) for FuncType as the params are potentially unbounded in length here, and I assume we have an implementation limit we want to stay below, right?
There was a problem hiding this comment.
Alternatively, you could add a wrapper type for bounding Vec lengths similar to what you did for NonZeroLenVec.
There was a problem hiding this 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.
| self.stack_get(stack, ValType::I32); | ||
| self.instruction(I32Const(0xffff)); | ||
| self.instruction(I32And); |
There was a problem hiding this comment.
Huh. I didn't realize there wasn't an i32.extend8_u instruction. 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?
There was a problem hiding this comment.
Double-checking again to make sure I didn't miss something, apparently there's i64.extend32_u but that's the only one.
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 whereupper 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.