winch(fuzz): Refactor Winch's fuzzing (#6432)

* winch(fuzz): Refactor Winch's fuzzing

This change is a follow-up to the discussion in
https://github.com/bytecodealliance/wasmtime/pull/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`

.github/workflows/main.yml

@@ -357,8 +357,6 @@ jobs:
     - run: cargo fuzz build --dev -s none
     # Check that the ISLE fuzz targets build too.
     - run: cargo fuzz build --dev -s none --fuzz-dir ./cranelift/isle/fuzz
-    # Check that winch fuzzing builds too.
-    - run: cargo fuzz build --dev -s none --features fuzz-winch
 
     # common logic to cancel the entire run if this job fails
     - run: gh run cancel ${{ github.run_id }}

crates/fuzzing/Cargo.toml

@@ -19,7 +19,7 @@ target-lexicon = { workspace = true }
 tempfile = "3.3.0"
 wasmparser = { workspace = true }
 wasmprinter = { workspace = true }
-wasmtime = { workspace = true, features = ['default'] }
+wasmtime = { workspace = true, features = ['default', 'winch'] }
 wasmtime-wast = { workspace = true }
 wasm-encoder = { workspace = true }
 wasm-smith = { workspace = true }
@@ -46,4 +46,3 @@ wasm-spec-interpreter = { path = "./wasm-spec-interpreter", optional = true, fea
 
 [features]
 fuzz-spec-interpreter = ['wasm-spec-interpreter']
-winch = ["wasmtime/winch"]

crates/fuzzing/src/generators.rs

@@ -23,7 +23,6 @@ pub mod table_ops;
 mod value;
 
 pub use codegen_settings::CodegenSettings;
-#[cfg(feature = "winch")]
 pub use config::CompilerStrategy;
 pub use config::{Config, WasmtimeConfig};
 pub use instance_allocation_strategy::InstanceAllocationStrategy;

crates/fuzzing/src/generators/config.rs

@@ -168,37 +168,42 @@ impl Config {
             .allocation_strategy(self.wasmtime.strategy.to_wasmtime())
             .generate_address_map(self.wasmtime.generate_address_map);
 
-        #[cfg(feature = "winch")]
+        let compiler_strategy = &self.wasmtime.compiler_strategy;
+        let cranelift_strategy = *compiler_strategy == CompilerStrategy::Cranelift;
         cfg.strategy(self.wasmtime.compiler_strategy.to_wasmtime());
 
         self.wasmtime.codegen.configure(&mut cfg);
 
-        // If the wasm-smith-generated module use nan canonicalization then we
-        // don't need to enable it, but if it doesn't enable it already then we
-        // enable this codegen option.
-        cfg.cranelift_nan_canonicalization(!self.module_config.config.canonicalize_nans);
-
-        // Enabling the verifier will at-least-double compilation time, which
-        // with a 20-30x slowdown in fuzzing can cause issues related to
-        // timeouts. If generated modules can have more than a small handful of
-        // functions then disable the verifier when fuzzing to try to lessen the
-        // impact of timeouts.
-        if self.module_config.config.max_funcs > 10 {
-            cfg.cranelift_debug_verifier(false);
-        }
+        // Only set cranelift specific flags when the Cranelift strategy is
+        // chosen.
+        if cranelift_strategy {
+            // If the wasm-smith-generated module use nan canonicalization then we
+            // don't need to enable it, but if it doesn't enable it already then we
+            // enable this codegen option.
+            cfg.cranelift_nan_canonicalization(!self.module_config.config.canonicalize_nans);
+
+            // Enabling the verifier will at-least-double compilation time, which
+            // with a 20-30x slowdown in fuzzing can cause issues related to
+            // timeouts. If generated modules can have more than a small handful of
+            // functions then disable the verifier when fuzzing to try to lessen the
+            // impact of timeouts.
+            if self.module_config.config.max_funcs > 10 {
+                cfg.cranelift_debug_verifier(false);
+            }
 
-        if self.wasmtime.force_jump_veneers {
-            unsafe {
-                cfg.cranelift_flag_set("wasmtime_linkopt_force_jump_veneer", "true");
+            if self.wasmtime.force_jump_veneers {
+                unsafe {
+                    cfg.cranelift_flag_set("wasmtime_linkopt_force_jump_veneer", "true");
+                }
             }
-        }
 
-        if let Some(pad) = self.wasmtime.padding_between_functions {
-            unsafe {
-                cfg.cranelift_flag_set(
-                    "wasmtime_linkopt_padding_between_functions",
-                    &pad.to_string(),
-                );
+            if let Some(pad) = self.wasmtime.padding_between_functions {
+                unsafe {
+                    cfg.cranelift_flag_set(
+                        "wasmtime_linkopt_padding_between_functions",
+                        &pad.to_string(),
+                    );
+                }
             }
         }
 
@@ -406,7 +411,6 @@ pub struct WasmtimeConfig {
     padding_between_functions: Option<u16>,
     generate_address_map: bool,
     native_unwind_info: bool,
-    #[cfg(feature = "winch")]
     /// Configuration for the compiler to use.
     pub compiler_strategy: CompilerStrategy,
     bb_padding_log2: u8,
@@ -454,8 +458,7 @@ impl OptLevel {
     }
 }
 
-#[cfg(feature = "winch")]
-#[derive(Arbitrary, Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 /// Compiler to use.
 pub enum CompilerStrategy {
     /// Cranelift compiler.
@@ -464,7 +467,6 @@ pub enum CompilerStrategy {
     Winch,
 }
 
-#[cfg(feature = "winch")]
 impl CompilerStrategy {
     fn to_wasmtime(&self) -> wasmtime::Strategy {
         match self {
@@ -473,3 +475,13 @@ impl CompilerStrategy {
         }
     }
 }
+
+// Unconditionally return `Cranelift` given that Winch is not ready to be
+// enabled by default in all the fuzzing targets. Each fuzzing target is
+// expected to explicitly override the strategy as needed. Currently only the
+// differential target overrides the compiler strategy.
+impl Arbitrary<'_> for CompilerStrategy {
+    fn arbitrary(_: &mut Unstructured<'_>) -> arbitrary::Result<Self> {
+        Ok(Self::Cranelift)
+    }
+}

fuzz/Cargo.toml

@@ -21,8 +21,8 @@ cranelift-control = { workspace = true }
 libfuzzer-sys = { workspace = true, features = ["arbitrary-derive"] }
 target-lexicon = { workspace = true }
 smallvec = { workspace = true }
-wasmparser = { workspace = true, optional = true }
-wasmtime = { workspace = true }
+wasmparser = { workspace = true }
+wasmtime = { workspace = true, features = ["winch"] }
 wasmtime-fuzzing = { workspace = true }
 component-test-util = { workspace = true }
 component-fuzz-util = { workspace = true }
@@ -36,7 +36,6 @@ quote = "1.0"
 component-fuzz-util = { workspace = true }
 
 [features]
-fuzz-winch = ["wasmtime/winch", "wasmtime-fuzzing/winch", "dep:wasmparser"]
 default = ['fuzz-spec-interpreter']
 fuzz-spec-interpreter = ['wasmtime-fuzzing/fuzz-spec-interpreter']
 chaos = ["cranelift-control/chaos"]

fuzz/fuzz_targets/differential.rs

@@ -5,14 +5,11 @@ use libfuzzer_sys::fuzz_target;
 use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering::SeqCst;
 use std::sync::Once;
-#[cfg(feature = "fuzz-winch")]
 use wasmtime_fuzzing::generators::CompilerStrategy;
 use wasmtime_fuzzing::generators::{Config, DiffValue, DiffValueType, SingleInstModule};
 use wasmtime_fuzzing::oracles::diff_wasmtime::WasmtimeInstance;
 use wasmtime_fuzzing::oracles::engine::{build_allowed_env_list, parse_env_list};
 use wasmtime_fuzzing::oracles::{differential, engine, log_wasm, DiffEqResult};
-#[cfg(feature = "fuzz-winch")]
-use wasmtime_fuzzing::wasm_smith::{InstructionKind, InstructionKinds};
 
 // Upper limit on the number of invocations for each WebAssembly function
 // executed by this fuzz target.
@@ -26,8 +23,10 @@ static SETUP: Once = Once::new();
 // - ALLOWED_ENGINES=wasmi,spec cargo +nightly fuzz run ...
 // - ALLOWED_ENGINES=-v8 cargo +nightly fuzz run ...
 // - ALLOWED_MODULES=single-inst cargo +nightly fuzz run ...
+// - FUZZ_WINCH=1 cargo +nightly fuzz run ...
 static mut ALLOWED_ENGINES: Vec<&str> = vec![];
 static mut ALLOWED_MODULES: Vec<&str> = vec![];
+static mut FUZZ_WINCH: bool = false;
 
 // Statistics about what's actually getting executed during fuzzing
 static STATS: RuntimeStats = RuntimeStats::new();
@@ -38,25 +37,26 @@ fuzz_target!(|data: &[u8]| {
         // `setup_ocaml_runtime`.
         engine::setup_engine_runtimes();
 
-        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"), &default_engines);
-        let allowed_modules =
-            build_allowed_env_list(parse_env_list("ALLOWED_MODULES"), &default_modules);
+        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 fuzz_winch = match std::env::var("FUZZ_WINCH").map(|v| v == "1") {
+            Ok(v) => v,
+            _ => false,
+        };
 
         unsafe {
             ALLOWED_ENGINES = allowed_engines;
             ALLOWED_MODULES = allowed_modules;
+            FUZZ_WINCH = fuzz_winch;
         }
     });
 
@@ -69,6 +69,7 @@ fn execute_one(data: &[u8]) -> Result<()> {
     STATS.bump_attempts();
 
     let mut u = Unstructured::new(data);
+    let fuzz_winch = unsafe { FUZZ_WINCH };
 
     // Generate a Wasmtime and module configuration and update its settings
     // initially to be suitable for differential execution where the generated
@@ -77,14 +78,11 @@ fn execute_one(data: &[u8]) -> Result<()> {
     let mut config: Config = u.arbitrary()?;
     config.set_differential_config();
 
-    #[cfg(feature = "fuzz-winch")]
-    {
-        // When fuzzing Winch:
-        // 1. Explicitly override the compiler strategy.
-        // 2. Explicitly set the allowed instructions for `wasm-smith`.
+    // When fuzzing Winch, explicitly override the compiler strategy, which by
+    // default its arbitrary implementation unconditionally returns
+    // `Cranelift`.
+    if fuzz_winch {
         config.wasmtime.compiler_strategy = CompilerStrategy::Winch;
-        config.module_config.config.allowed_instructions =
-            InstructionKinds::new(&[InstructionKind::Numeric, InstructionKind::Variable]);
     }
 
     // Choose an engine that Wasmtime will be differentially executed against.
@@ -120,8 +118,7 @@ fn execute_one(data: &[u8]) -> Result<()> {
         _ => unreachable!(),
     };
 
-    #[cfg(feature = "fuzz-winch")]
-    if !winch_supports_module(&wasm) {
+    if fuzz_winch && !winch_supports_module(&wasm) {
         return Ok(());
     }
 
@@ -277,7 +274,6 @@ impl RuntimeStats {
     }
 }
 
-#[cfg(feature = "fuzz-winch")]
 // Returns true if the module only contains operators supported by
 // Winch. Winch's x86_64 target has broader support for Wasm operators
 // than the aarch64 target. This list assumes fuzzing on the x86_64