asm: re-allow prop-testing with cargo test (#10185)

In #10110, I originally intended to use `arbitrary` implementations in
two ways: for long-running fuzz testing (e.g., with OSS-Fuzz) but also
for quick property testing with `cargo test`. This latter use case could
replace the tedious emit tests we had to write in `cranelift-codegen`
_and_ find corner cases that we otherwise might not explore. It helped
me during development: just run `cargo test` to check if anything is
obviously wrong. `arbtest` seemed to be able to run ~1000 test cases and
found mistakes well within the one second time limit I gave it.

@alexcrichton improved #10110 by avoiding `Arbitrary` implementations
everywhere and unconditionally depending on the `arbitrary` crate. This
was the right change, but it removed the ability to property test using
`cargo test`. What this change does is retain the general intent of his
change (no extra dependencies) but add `Arbitrary` implementations for
`cfg(test)` as well to run property tests during `cargo test`.

The only downside I see here is the added complexity when conditionally
compiling the fuzz-related bits: `#[cfg(any(test, feature = "fuzz"))]`.
Perhaps there is a better way to do this, but this seemed to work fine.
Let me know what you think.

Author: abrown

Cargo.lock

@@ -8,9 +8,12 @@ rust-version.workspace = true
 
 [dependencies]
 arbitrary = { workspace = true, features = ["derive"], optional = true }
+capstone = { workspace = true, optional = true }
 
 [dev-dependencies]
+arbitrary = { workspace = true, features = ["derive"] }
 arbtest = "0.3.1"
+capstone = { workspace = true }
 
 [build-dependencies]
 cranelift-assembler-x64-meta = { path = "meta", version = "0.118.0" }
@@ -23,4 +26,4 @@ similar_names = { level = "allow", priority = 1 }
 wildcard_imports = { level = "allow", priority = 1 }
 
 [features]
-arbitrary = ['dep:arbitrary']
+fuzz = ['dep:arbitrary', 'dep:capstone']

cranelift/assembler-x64/Cargo.toml

@@ -10,9 +10,7 @@ cargo-fuzz = true
 
 [dependencies]
 libfuzzer-sys = { workspace = true }
-cranelift-assembler-x64 = { path = "..", features = ['arbitrary'] }
-capstone = { workspace = true }
-arbitrary = { workspace = true, features = ['derive'] }
+cranelift-assembler-x64 = { path = "..", features = ['fuzz'] }
 
 [[bin]]
 name = "roundtrip"

cranelift/assembler-x64/fuzz/Cargo.lock

@@ -1,120 +1,8 @@
 #![no_main]
 
-use arbitrary::Arbitrary;
-use capstone::arch::{BuildsCapstone, BuildsCapstoneSyntax};
-use cranelift_assembler_x64::{AsReg, Inst, Registers};
+use cranelift_assembler_x64::{fuzz, Inst};
 use libfuzzer_sys::fuzz_target;
 
-// Generate a random assembly instruction and check its encoding and
-// pretty-printing against a known-good disassembler.
-//
-// # Panics
-//
-// This function panics to express failure as expected by the `arbitrary`
-// fuzzer infrastructure. It may fail during assembly, disassembly, or when
-// comparing the disassembled strings.
-fuzz_target!(|inst: Inst<FuzzRegs>| {
-    // Check that we can actually assemble this instruction.
-    let assembled = assemble(&inst);
-    let expected = disassemble(&assembled);
-
-    // Check that our pretty-printed output matches the known-good output.
-    let expected = expected.split_once(' ').unwrap().1;
-    let actual = inst.to_string();
-    if expected != actual {
-        println!("> {inst}");
-        println!("  debug: {inst:x?}");
-        println!("  assembled: {}", pretty_print_hexadecimal(&assembled));
-        assert_eq!(expected, &actual);
-    }
+fuzz_target!(|inst: Inst<fuzz::FuzzRegs>| {
+    fuzz::roundtrip(&inst);
 });
-
-/// Use this assembler to emit machine code into a byte buffer.
-///
-/// This will skip any traps or label registrations, but this is fine for the
-/// single-instruction disassembly we're doing here.
-fn assemble(insn: &Inst<FuzzRegs>) -> Vec<u8> {
-    let mut buffer = Vec::new();
-    let offsets: Vec<i32> = Vec::new();
-    insn.encode(&mut buffer, &offsets);
-    buffer
-}
-
-/// Building a new `Capstone` each time is suboptimal (TODO).
-fn disassemble(assembled: &[u8]) -> String {
-    let cs = capstone::Capstone::new()
-        .x86()
-        .mode(capstone::arch::x86::ArchMode::Mode64)
-        .syntax(capstone::arch::x86::ArchSyntax::Att)
-        .detail(true)
-        .build()
-        .expect("failed to create Capstone object");
-    let insns = cs
-        .disasm_all(assembled, 0x0)
-        .expect("failed to disassemble");
-    assert_eq!(insns.len(), 1, "not a single instruction: {assembled:x?}");
-    let insn = insns.first().expect("at least one instruction");
-    assert_eq!(assembled.len(), insn.len());
-    insn.to_string()
-}
-
-fn pretty_print_hexadecimal(hex: &[u8]) -> String {
-    use std::fmt::Write;
-    let mut s = String::with_capacity(hex.len() * 2);
-    for b in hex {
-        write!(&mut s, "{b:02X}").unwrap();
-    }
-    s
-}
-
-/// Fuzz-specific registers.
-///
-/// For the fuzzer, we do not need any fancy register types; see [`FuzzReg`].
-#[derive(Arbitrary, Debug)]
-pub struct FuzzRegs;
-
-impl Registers for FuzzRegs {
-    type ReadGpr = FuzzReg;
-    type ReadWriteGpr = FuzzReg;
-}
-
-/// A simple `u8` register type for fuzzing only
-#[derive(Clone, Copy, Debug)]
-pub struct FuzzReg(u8);
-
-impl<'a> Arbitrary<'a> for FuzzReg {
-    fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
-        Ok(Self::new(u.int_in_range(0..=15)?))
-    }
-}
-
-impl AsReg for FuzzReg {
-    fn new(enc: u8) -> Self {
-        Self(enc)
-    }
-    fn enc(&self) -> u8 {
-        self.0
-    }
-}
-
-#[cfg(test)]
-mod test {
-    use super::*;
-    use arbtest::arbtest;
-    use std::sync::atomic::{AtomicUsize, Ordering};
-
-    #[test]
-    fn smoke() {
-        let count = AtomicUsize::new(0);
-        arbtest(|u| {
-            let inst: Inst<FuzzRegs> = u.arbitrary()?;
-            roundtrip(&inst);
-            println!("#{}: {inst}", count.fetch_add(1, Ordering::SeqCst));
-            Ok(())
-        })
-        .budget_ms(1_000);
-
-        // This will run the `roundtrip` fuzzer for one second. To repeatably
-        // test a single input, append `.seed(0x<failing seed>)`.
-    }
-}

cranelift/assembler-x64/fuzz/Cargo.toml

@@ -86,13 +86,16 @@ fn generate_inst_enum(f: &mut Formatter, insts: &[dsl::Inst]) {
 /// `#[derive(...)]`
 fn generate_derive(f: &mut Formatter) {
     f.line("#[derive(Clone, Debug)]", None);
-    f.line("#[cfg_attr(feature = \"arbitrary\", derive(arbitrary::Arbitrary))]", None);
+    f.line("#[cfg_attr(any(test, feature = \"fuzz\"), derive(arbitrary::Arbitrary))]", None);
 }
 
 /// Adds a custom bound to the `Arbitrary` implementation which ensures that
 /// the associated registers are all `Arbitrary` as well.
 fn generate_derive_arbitrary_bounds(f: &mut Formatter) {
-    f.line("#[cfg_attr(feature = \"arbitrary\", arbitrary(bound = \"R: crate::arbitrary_impls::RegistersArbitrary\"))]", None);
+    f.line(
+        "#[cfg_attr(any(test, feature = \"fuzz\"), arbitrary(bound = \"R: crate::fuzz::RegistersArbitrary\"))]",
+        None,
+    );
 }
 
 /// `impl std::fmt::Display for Inst { ... }`

cranelift/assembler-x64/fuzz/fuzz_targets/roundtrip.rs

@@ -67,17 +67,17 @@ impl CodeSink for Vec<u8> {
 
 /// Wrap [`CodeSink`]-specific labels.
 #[derive(Debug, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
+#[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Label(pub u32);
 
 /// Wrap [`CodeSink`]-specific constant keys.
 #[derive(Debug, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
+#[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct Constant(pub u32);
 
 /// Wrap [`CodeSink`]-specific trap codes.
 #[derive(Debug, Clone, Copy)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
+#[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))]
 pub struct TrapCode(pub NonZeroU8);
 
 /// A table mapping `KnownOffset` identifiers to their `i32` offset values.