Cranelift: Harvest each Souper LHS into its own file (#5649)

* Cranelift: Harvest each Souper LHS into its own file

Souper only handles one input LHS at a time, so this makes it way easier to
script. Don't need to try and parse each LHS.

* Add audit of `arrayref` version 0.3.6

* Add audit of `constant_time_eq` version 0.2.4

Cargo.lock

@@ -754,6 +754,7 @@ dependencies = [
  "cranelift-reader",
  "cranelift-wasm",
  "filecheck",
+ "fxhash",
  "indicatif",
  "log",
  "pretty_env_logger",

cranelift/Cargo.toml

@@ -48,6 +48,7 @@ clap = { workspace = true }
 similar = { workspace = true }
 toml = { workspace = true }
 serde = { workspace = true }
+fxhash = "0.2.1"
 
 [features]
 default = ["disas", "wasm", "cranelift-codegen/all-arch", "cranelift-codegen/trace-log", "souper-harvest"]

cranelift/src/souper_harvest.rs

@@ -4,6 +4,8 @@ use cranelift_codegen::Context;
 use cranelift_reader::parse_sets_and_triple;
 use cranelift_wasm::DummyEnvironment;
 use rayon::iter::{IntoParallelIterator, ParallelIterator};
+use std::collections::HashSet;
+use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::{fs, io};
 
@@ -18,9 +20,10 @@ pub struct Options {
     /// Specify an input file to be used. Use '-' for stdin.
     input: PathBuf,
 
-    /// Specify the output file to be used. Use '-' for stdout.
+    /// Specify the directory where harvested left-hand side files should be
-    #[clap(short, long, default_value("-"))]
+    /// written to.
-    output: PathBuf,
+    #[clap(short, long)]
+    output_dir: PathBuf,
 
     /// Configure Cranelift settings
     #[clap(long = "set")]
@@ -29,6 +32,12 @@ pub struct Options {
     /// Specify the Cranelift target
     #[clap(long = "target")]
     target: String,
+
+    /// Add a comment from which CLIF variable and function each left-hand side
+    /// was harvested from. This prevents deduplicating harvested left-hand
+    /// sides.
+    #[clap(long)]
+    add_harvest_source: bool,
 }
 
 pub fn run(options: &Options) -> Result<()> {
@@ -47,13 +56,25 @@ pub fn run(options: &Options) -> Result<()> {
         ))
     };
 
-    let mut output: Box<dyn io::Write + Send> = if options.output == Path::new("-") {
+    match std::fs::create_dir_all(&options.output_dir) {
-        Box::new(io::stdout())
+        Ok(_) => {}
-    } else {
+        Err(e)
-        Box::new(io::BufWriter::new(
+            if e.kind() == io::ErrorKind::AlreadyExists
-            fs::File::create(&options.output).context("failed to create output file")?,
+                && fs::metadata(&options.output_dir)
+                    .with_context(|| {
+                        format!(
+                            "failed to read file metadata: {}",
+                            options.output_dir.display(),
+                        )
+                    })?
+                    .is_dir() => {}
+        Err(e) => {
+            return Err(e).context(format!(
+                "failed to create output directory: {}",
+                options.output_dir.display()
             ))
-    };
+        }
+    }
 
     let mut contents = vec![];
     input
@@ -77,13 +98,33 @@ pub fn run(options: &Options) -> Result<()> {
 
     let (send, recv) = std::sync::mpsc::channel::<String>();
 
-    let writing_thread = std::thread::spawn(move || -> Result<()> {
+    let writing_thread = std::thread::spawn({
+        let output_dir = options.output_dir.clone();
+        let keep_harvest_source = options.add_harvest_source;
+        move || -> Result<()> {
+            let mut already_harvested = HashSet::new();
             for lhs in recv {
-            output
+                let lhs = if keep_harvest_source {
-                .write_all(lhs.as_bytes())
+                    &lhs
-                .context("failed to write to output file")?;
+                } else {
+                    // Remove the first `;; Harvested from v12 in u:34` line.
+                    let i = lhs.find('\n').unwrap();
+                    &lhs[i + 1..]
+                };
+                let hash = fxhash::hash(lhs.as_bytes());
+                if already_harvested.insert(hash) {
+                    let output_path = output_dir.join(hash.to_string());
+                    let mut output =
+                        io::BufWriter::new(fs::File::create(&output_path).with_context(|| {
+                            format!("failed to create file: {}", output_path.display())
+                        })?);
+                    output.write_all(lhs.as_bytes()).with_context(|| {
+                        format!("failed to write to output file: {}", output_path.display())
+                    })?;
+                }
             }
             Ok(())
+        }
     });
 
     funcs
@@ -92,9 +133,8 @@ pub fn run(options: &Options) -> Result<()> {
             let mut ctx = Context::new();
             ctx.func = func;
 
-            ctx.compute_cfg();
+            ctx.optimize(fisa.isa.unwrap())
-            ctx.preopt(fisa.isa.unwrap())
+                .context("failed to run optimizations")?;
-                .context("failed to run preopt")?;
 
             ctx.souper_harvest(send)
                 .context("failed to run souper harvester")?;

supply-chain/audits.toml

@@ -24,6 +24,15 @@ criteria = "safe-to-deploy"
 version = "1.1.4"
 notes = "I am the author of this crate."
 
+[[audits.arrayref]]
+who = "Nick Fitzgerald <fitzgen@gmail.com>"
+criteria = "safe-to-deploy"
+version = "0.3.6"
+notes = """
+Unsafe code, but its logic looks good to me. Necessary given what it is
+doing. Well tested, has quickchecks.
+"""
+
 [[audits.arrayvec]]
 who = "Nick Fitzgerald <fitzgen@gmail.com>"
 criteria = "safe-to-deploy"
@@ -167,6 +176,12 @@ criteria = "safe-to-deploy"
 version = "0.11.1"
 notes = "This library uses `forbid(unsafe_code)` and has no filesystem or network I/O."
 
+[[audits.constant_time_eq]]
+who = "Nick Fitzgerald <fitzgen@gmail.com>"
+criteria = "safe-to-deploy"
+version = "0.2.4"
+notes = "A few tiny blocks of `unsafe` but each of them is very obviously correct."
+
 [[audits.criterion]]
 who = "Alex Crichton <alex@alexcrichton.com>"
 criteria = "safe-to-run"