winch(fuzz): Initial support for differential fuzzing#6281
winch(fuzz): Initial support for differential fuzzing#6281cfallin merged 3 commits intobytecodealliance:mainfrom
Conversation
This commit introduces initial support for differential fuzzing for Winch. In order to fuzz winch, this change introduces the `winch` cargo feature. When the `winch` cargo feature is enabled the differential fuzz target uses `wasmi` as the differential engine and `wasm-smith` and `single-inst` as the module sources. The intention behind this change is to have a *local* approach for fuzzing and verifying programs generated by Winch and to have an initial implementation that will allow us to eventually enable this change by default. Currently it's not worth it to enable this change by default given all the filtering that needs to happen to ensure that the generated modules are supported by Winch. It's worth noting that the Wasm filtering code will be temporary, until Winch reaches feature parity in terms of Wasm operators.
c31b5c6 to
5a86979
Compare
cfallin
left a comment
There was a problem hiding this comment.
This looks generally good-- I'm reviewing it with the understanding that the WInch-specific config details are temporary (in the long run) so I won't fuss too much about configuration details :-)
One thing I might suggest: the winch feature in a fuzz-build does something subtly different than the winch feature in the rest of Wasmtime, namely it restricts options rather than adds support. It turns the general fuzz target into a Winch-specific thing. So perhaps we could name the feature in the fuzz crate something more specific, like fuzz-winch-only or winch-fuzz or somesuch? (Then the feature can require the winch feature in the rest of the engine.)
LGTM with or without that -- I'll leave it up to you -- but thought it might be clearer. Thanks!
Great catch, yeah. I've renamed the feature to |
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| let (default_engines, default_modules) = if cfg!(feature = "fuzz-winch") { | ||
| (vec!["wasmi"], vec!["wasm-smith", "single-inst"]) | ||
| } else { | ||
| ( | ||
| vec!["wasmtime", "wasmi", "spec", "v8"], | ||
| vec!["wasm-smith", "single-inst"], | ||
| ) | ||
| }; | ||
|
|
||
| // Retrieve the configuration for this fuzz target from `ALLOWED_*` | ||
| // environment variables. | ||
| let allowed_engines = build_allowed_env_list( | ||
| parse_env_list("ALLOWED_ENGINES"), | ||
| &["wasmtime", "wasmi", "spec", "v8"], | ||
| ); | ||
| let allowed_modules = build_allowed_env_list( | ||
| parse_env_list("ALLOWED_MODULES"), | ||
| &["wasm-smith", "single-inst"], | ||
| ); | ||
| let allowed_engines = | ||
| build_allowed_env_list(parse_env_list("ALLOWED_ENGINES"), &default_engines); | ||
| let allowed_modules = | ||
| build_allowed_env_list(parse_env_list("ALLOWED_MODULES"), &default_modules); |
There was a problem hiding this comment.
How come winch changes the selection of default engines?
There was a problem hiding this comment.
Since the objective of this change is mostly to have a way to fuzz locally, my line of thought here was to choose the most lightweight option for differential fuzzing, in that sense, this decision is very specific to Winch and to how I intend to use this fuzz target today. Once Winch is robust enough, such that we are able to enable fuzzing in cluster-fuzz, I think we can revert this change. The other thing that I've discussed briefly with @cfallin is that maybe for Winch, even in cluster-fuzz we could consider only fuzzing Winch against an interpreter like wasmi in order to save some fuzz time in cluster-fuzz, and also because Winch is already partly covered by Cranelift's fuzzing given its dependency on its backends.
There was a problem hiding this comment.
To add a bit, @alexcrichton the intent of the design here is to give temporary support for Winch bringup; as @saulecabrera noted we hope that eventually it's "just another engine in the list". The alternative to this would be to develop a completely separate "bringup fuzz target" doing the targeted comparison that we want, which we decided would be less ideal (code duplication etc).
There was a problem hiding this comment.
That all makes sense to me, but for fuzzing locally the env vars can be set, so I'm not sure why this change was needed? It's not wrong of course, and I agree that there's no need for a whole separate fuzz target for winch. I was expecting, though, that how to fuzz winch would be "probably set some env vars to specific values to avoid known crashes and get quicker test coverage", so that way fuzzing winch in full is "remove the env vars" or similar.
| #[cfg(feature = "fuzz-winch")] | ||
| { | ||
| // When fuzzing Winch: | ||
| // 1. Explicitly override the compiler strategy. | ||
| // 2. Explicitly set the allowed instructions for `wasm-smith`. | ||
| config.wasmtime.compiler_strategy = CompilerStrategy::Winch; | ||
| config.module_config.config.allowed_instructions = | ||
| InstructionKinds::new(&[InstructionKind::Numeric, InstructionKind::Variable]); | ||
| } |
There was a problem hiding this comment.
One thought I had reading over this is that the winch support here seems pretty specific and targeted, which while ok for now may not translate to easier integration in the future. For example having specific paths for winch such as this may not age well as more support is added.
An alternative, though, would be something like:
- Always have
CompilerStrategy - Only generate
CompilerStrategy::Winchwhenfuzz-winchis enabled - Don't configure
allowed_instructions, but allow modules fall through towinch_supports_modulebelow
This would result in a higher "rejection rate" or a smaller rate of coverage for winch since modules won't be tailor-made to run in winch, but I think that could be possibly smoothed over with environment variable based configuration like ALLOWED_ENGINES?
Overall I think it'd be best to integrate without #[cfg] where possible since having special one-off instructions like that tend to accumulate more-complicated logic over time.
To be clear though this is more of a stylistic concern than anything else. I think it's fine to leave this all as-is and it can be revisited when the fuzzing support is updated in the future.
There was a problem hiding this comment.
Agreed with all your points @alexcrichton, thanks for bringing this up; the only thing that I'll note is that the main reason for configuring allowed_instructions as part of this change is to have a higher change of getting a module supported by Winch, which is important for me at the moment, given that I'm only intending to use this locally, I found that configuring wasm-smith to do this would be the easiest for now.
In terms of the cfg complexity, my expectation is that as more features are added to Winch the complexity here shouldn't increase, aside from having to add the supported instructions to winch_supports_module. But in general, my expectation is that once decide to enable fuzzing for Winch by default, the cfg complexity should be minimal or non-existent and my hope if that we can control any Winch specific behaviour through environment variables as you suggested.
…ce#6281) * winch(fuzz): Initial support for differential fuzzing This commit introduces initial support for differential fuzzing for Winch. In order to fuzz winch, this change introduces the `winch` cargo feature. When the `winch` cargo feature is enabled the differential fuzz target uses `wasmi` as the differential engine and `wasm-smith` and `single-inst` as the module sources. The intention behind this change is to have a *local* approach for fuzzing and verifying programs generated by Winch and to have an initial implementation that will allow us to eventually enable this change by default. Currently it's not worth it to enable this change by default given all the filtering that needs to happen to ensure that the generated modules are supported by Winch. It's worth noting that the Wasm filtering code will be temporary, until Winch reaches feature parity in terms of Wasm operators. * Check build targets with the `winch` feature flag * Rename fuzz target feature to `fuzz-winch`
This change is a follow-up to the discussion in bytecodealliance#6281. The most notable characteristic of this change is that it enables `winch` by default in the fuzzers. If compilation time is a big enough concern I can add the cargo feature back. I opted to enable `winch` by default for several reasons: * It substantially reduces the `cfg` complexity -- at first I thought I had covered all the places in which a `cfg` check would be needed, but then I realized that I missed the Cranelift specific compiler flags. * It's the fastest route to enable winch by default in the fuzzers, which we want to do eventually -- the only change we'd need at that point would be to get rid of the winch-specific environment variable. * We can get rid of the winch-specific checks in CI for fuzzing
This change is a follow-up to the discussion in bytecodealliance#6281. The most notable characteristic of this change is that it enables `winch` by default in the fuzzers. If compilation time is a big enough concern I can add the cargo feature back. I opted to enable `winch` by default for several reasons: * It substantially reduces the `cfg` complexity -- at first I thought I had covered all the places in which a `cfg` check would be needed, but then I realized that I missed the Cranelift specific compiler flags. * It's the fastest route to enable winch by default in the fuzzers, which we want to do eventually -- the only change we'd need at that point would be to get rid of the winch-specific environment variable. * We can get rid of the winch-specific checks in CI for fuzzing
* winch(fuzz): Refactor Winch's fuzzing This change is a follow-up to the discussion in #6281. The most notable characteristic of this change is that it enables `winch` by default in the fuzzers. If compilation time is a big enough concern I can add the cargo feature back. I opted to enable `winch` by default for several reasons: * It substantially reduces the `cfg` complexity -- at first I thought I had covered all the places in which a `cfg` check would be needed, but then I realized that I missed the Cranelift specific compiler flags. * It's the fastest route to enable winch by default in the fuzzers, which we want to do eventually -- the only change we'd need at that point would be to get rid of the winch-specific environment variable. * We can get rid of the winch-specific checks in CI for fuzzing * Implement Arbitraty for CompilerStrategy Unconditionally return `Cranelift` for the `Arbitrary` implementation of `CompilerStrategy`. This ensures that `Cranelift` is used as the compiler for all the targets unless explicitly requested otherwise. As of this change, only the differential target overrides the `CompilerStrategy`
This commit introduces initial support for differential fuzzing for Winch. In order to fuzz winch, this change introduces the
winchcargo feature. When thewinchcargo feature is enabled the differential fuzz target useswasmias the differential engine andwasm-smithandsingle-instas the module sources.The intention behind this change is to have a local approach for fuzzing and verifying programs generated by Winch and to have an initial implementation that will allow us to eventually enable this change by default. Currently it's not worth it to enable this change by default given all the filtering that needs to happen to ensure that the generated modules are supported by Winch.
It's worth noting that the Wasm filtering code will be temporary, until Winch
reaches feature parity in terms of Wasm operators.