Wasmtime GitHub notifications bot (Jul 23 2022 at 00:16): Wasmtime GitHub notifications bot (Jul 23 2022 at 00:16):abrown opened PR #4515 from meta-diff to main:
This fuzz target combines past efforts to fuzz Wasmtime against various engines and against various kinds of modules. Once complete, it is intended to replace the other differential targets. Some notable changes in this PR include the addition of interfaces for each Wasm engine we want to fuzz against and the ability to evaluate a function multiple times with different randomly-generated inputs. See the commit messages for more details.
Wasmtime GitHub notifications bot (Jul 23 2022 at 00:16):abrown requested fitzgen for a review on PR #4515.
Wasmtime GitHub notifications bot (Jul 23 2022 at 00:18):jameysharp requested jameysharp for a review on PR #4515.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
This might be better modeled as
TryFromto have callers acknowledge that theFromis fallible.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
One thing that might be good to add here is "known values" for each lane width like (1u64, 1u64), (-1i64, -1i64), (1u32, 2u32, 3u32, 4u32), etc.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
I think if this is
&[u8]instead of[u8; 1024]it's whatever the fuzzer gives, and otherwise it's required to be 1kb?
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
Instead of having a separate structure for this couldd all of
ModuleConfigbe passed around? Otherwise this seems a bit odd since it's missing a number of other proposals which could perhaps be relevant.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
I think that this is worth implementing but all that we can really say about these is "null" or "non-null" so I think we could have
DiffValue::ExternRef { is_null: bool }perhaps
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
Personally I get pretty easily lost in the maze of
Arbitraryimplementations and methods of configuration. Would it be possible to avoid having this andarbitrary_with_compatible_configbelow? Could some higher-up generate theConfig/Engineand be responsible for passing it down?Or otherwise it seems like with Wasmtime we'd want a constant
ModuleConfigwith possibly differingWasmtimeConfigor something like that.If this is too difficult though or otherwise requires complications then I think that removing differential-wasmtime-with-itself is pretty reasonable. 90% of the value comes from differential execution with _something_ so adding more engines IMO doesn't increase our coverage really all that much.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
This loop may not play nicely with fuel right now since if the module has wasm-smith-injected fuel the fuel is never reset and otherwise with Wasmtime-injected fuel I'm not sure that's ever reset either.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
Reading over the implementations of this could this trait perhaps get changed to have something different like "get the hashable export named
foo" to avoid requiring hardcoding the hash algorithm between all engines? That would also allow more direct comparisons and better errors along the lines of "the state of this global/memory is different" rather than "some state is different"
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
Could this be
&mut selfto avoid theRefCellneeded for Wasmtime?
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
Although as I say that I remember that different engines implement different features which is often the hard part and nothing implements Wasmtime's feature set other than Wasmtime exactly.
Wasmtime GitHub notifications bot (Jul 25 2022 at 14:54):alexcrichton created PR review comment:
One thing that may be helpful here is to use the concrete Wasmtime rhs to use Wasmtime's embedding API to determine this rather than using the
wasmparsercrate.I also think it's reasonable for the
differentialfunction itself to just force the rhs to the Wasmtime concrete type because I don't think there's much value in having two 100% generic engines since we always want one to be Wasmtime anyway.
Wasmtime GitHub notifications bot (Jul 25 2022 at 20:50):fitzgen created PR review comment:
This stuff should probably be in
generators/value.rs.
Wasmtime GitHub notifications bot (Jul 25 2022 at 20:50):fitzgen created PR review comment:
Ditto
Wasmtime GitHub notifications bot (Jul 25 2022 at 20:50):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2022 at 20:50):fitzgen created PR review comment:
Yeah, we should reset fuel for each call.
Wasmtime GitHub notifications bot (Jul 25 2022 at 20:50):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2022 at 20:50):fitzgen created PR review comment:
Running the exported functions multiple times will -- in general, between single-instruction modules and fuel limits -- be a lot cheaper than compiling and instantiating the module itself. Therefore, we should make sure we take as much advantage of the work we already did to get here by at least using each of our known values for each argument, and then whatever the fuzzer gives us that is left over can also be used to generate new inputs, but at least this way we know we are exercising every module with a bunch of different interesting argument values.
Wasmtime GitHub notifications bot (Jul 25 2022 at 21:20):abrown created PR review comment:
It could... but actually it feels like the Wasmtime API shouldn't require a mutable store when we're only reading values.
Wasmtime GitHub notifications bot (Jul 25 2022 at 21:20):abrown submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2022 at 21:50):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 25 2022 at 21:50):alexcrichton created PR review comment:
Wasmtime has its own internal reasons for this, namely that extracting items from an instance is done lazily and may insert items into the store. There's other possibilities like reading an externref global could trigger a gc or similar situations like that.
Overall I think it's best to stick with
&muthere.
Wasmtime GitHub notifications bot (Jul 28 2022 at 22:24):alexcrichton closed without merge PR #4515.
Wasmtime GitHub notifications bot (Jul 28 2022 at 22:24):alexcrichton reopened PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 05 2022 at 20:49):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 13:42):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 13:48):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 15:29):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 18:41):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 21:36):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 22:53):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 23:07):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 10 2022 at 23:40):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 10 2022 at 23:40):jameysharp created PR review comment:
Is this newer version of
anyhownecessary? If so, I think it should probably be reflected in the minimum version requirement in the appropriateCargo.toml. Otherwise can you revert this change toCargo.lock?If you do need the upgrade, I don't think we should be adding cargo-vet exceptions. I hope a delta certification isn't a big deal for that crate.
If you're just adding the exception as a temporary thing to get CI past that check, could you clearly mark that commit so we don't forget to remove it again before merging?
Wasmtime GitHub notifications bot (Aug 10 2022 at 23:40):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 03:11):abrown created PR review comment:
No, sorry. I was just trying to make the CI green but was not reflecting too much on why this was even necessary. I think at one point in the lifetime of this PR I had brought anyhow to the fuzz targets but that is no longer necessary (besides, someone else has already brought that dependency in since this PR was opened). I'll fix up the lock file instead.
Wasmtime GitHub notifications bot (Aug 11 2022 at 03:11):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 16:24):jameysharp created PR review comment:
That's totally understandable!
On the other PR where you (briefly) added this exemption, I was worried we'd merged a lockfile change to
mainthat somehow slipped past the cargo-vet CI checks. That's what I meant by wondering if we'd screwed up: it wouldn't be hugely surprising if either cargo-vet or the CI integration had a bug, and besides, as a community our processes around this are largely untested still. But it's nice to know that this particular case has a simpler explanation.
Wasmtime GitHub notifications bot (Aug 11 2022 at 16:24):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 16:38):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 11 2022 at 18:07):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 11 2022 at 18:08):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 18:08):abrown created PR review comment:
Well, thanks for catching this (and the other PR!). This commit should be gone now as well as the unintential
Cargo.lockupgrade.
Wasmtime GitHub notifications bot (Aug 11 2022 at 19:22):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 19:22):abrown created PR review comment:
I initially designed it like that but then moved to this when I realized that
SwarmConfig(thewasm-smithstruct whatModuleConfigwraps) is intended to "generate a module this way" and what I mean is "does a module use this feature?" So a lot ofSwarmConfig, e.g., the minimum/maximums of various kinds, don't really apply. They don't matter to the engines for the most part (an engine can handle 10 instructions just as well as 100 instructions) and they don't make sense for the single-instruction modules (they always only have 1 instruction). That's when I switched from adapting a single-instance module to aModuleConfig/SwarmConfigand instead started adapting aModuleConfig/SwarmConfigto this newModuleFeatures. Would you be more in favor of this struct if I added the remaining Wasm features to it?
Wasmtime GitHub notifications bot (Aug 11 2022 at 19:54):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:18):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:23):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:27):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:27):abrown created PR review comment:
I don't see how this would change things much: we still want a separate function to gather up the signatures of the functions to execute.
wasmparseris already a dependency and it doesn't seem to me that switching to Wasmtime would reduce much code (?).
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:27):abrown edited PR review comment.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:28):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:28):abrown created PR review comment:
I was dividing the code up by engine, so I chose to put all these engine-specific conversions together with the engine that needs them.
Wasmtime GitHub notifications bot (Aug 11 2022 at 20:45):abrown has marked PR #4515 as ready for review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 21:00):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 21:00):abrown created PR review comment:
The single-instruction modules that this PR uses don't exercise reference types but I agree that in the future, e.g., with
wasm-smith-generated modules, this is probably what we want to do.
Wasmtime GitHub notifications bot (Aug 11 2022 at 21:49):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2022 at 21:49):abrown created PR review comment:
Right now, with only single-instruction modules, this shouldn't be too much of an issue but once we add in the wasm-smith modules this will be necessary; what is the right way to reset this?
Wasmtime GitHub notifications bot (Aug 11 2022 at 21:50):abrown created PR review comment:
Yeah, this probably does need some changes but I'm not yet entirely clear what they are; I think looking at what is necessary for the OCaml spec interpreter might make things more clear (as mentioned here).
Wasmtime GitHub notifications bot (Aug 11 2022 at 21:50):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 15 2022 at 21:28):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 15 2022 at 21:28):alexcrichton created PR review comment:
I ideally want to centralize on feature management instead of having multiple avenues and tests in different places for features. Fuzzing all the various features gets nontrivial in management complexity pretty quickly and I always get lost a few lines in whenever I try to bring myself back up to speed on the fuzzers.
Is it a problem to take
&ModuleConfiginstead of&ModuleFeatures? I presume that something motivated you to add this but I don't know what that is. I'd ideally like to tackle this problem if we can.
Wasmtime GitHub notifications bot (Aug 15 2022 at 21:29):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 15 2022 at 21:29):alexcrichton created PR review comment:
Currently there's no way to reset the fuel, we'd have to add an exported function in the
wasm-smithgenerated module (or export the global) for it to get modified by the embedder.
Wasmtime GitHub notifications bot (Aug 15 2022 at 21:31):alexcrichton created PR review comment:
Using the Wasmtime embedding API is much more natural in my opinion, and the Wasmtime API is much more stable than the
wasmparserAPI. I don't really want to keep up with randomwasmparserchanges way over here in the fuzzing code when one of the engines we're fuzzing against should always be Wasmtime anyway.(I also don't think it's useful to be so abstract here that we could fuzz v8 against wasmi here, I would rather have "here's the wasmtime config" and "here's the other engine config" and the fuzzing is entirely driven by the Wasmtime embedding API plus requests to the engine config through a trait)
Wasmtime GitHub notifications bot (Aug 15 2022 at 21:31):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 15 2022 at 22:06):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 15 2022 at 22:06):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 15 2022 at 22:06):alexcrichton created PR review comment:
This has found bugs in the past, so I think that it would be good to get a jump-start on this. There's some code in the v8 differential bits comparing traps, and I think that could broadly be used to get adapted here. We can't expect precisely the same errors all the time but if neither stack overflows for example then I think it's fair to ensure that both divided by zero or something like that.
Wasmtime GitHub notifications bot (Aug 18 2022 at 21:11):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 18 2022 at 21:58):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 18 2022 at 22:22):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 18 2022 at 23:12):abrown updated PR #4515 from meta-diff to main.
Wasmtime GitHub notifications bot (Aug 19 2022 at 00:22):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2022 at 00:23):alexcrichton merged PR #4515.
Last updated: Dec 23 2024 at 12:05 UTC