refactor: improve user facing errors as well as assertions (#24)

 - use anyhow to give users more feedback about errors
 - replace unwrap() with expect() to document assumptions in code

closes #24

Cargo.toml

@@ -12,7 +12,7 @@ pictures = []
 goblin = "~0.2.3"
 byteorder = "~1.3.4"
 clap = "3.0.0-beta.2"
-riscu = "~0.3.0"
+riscu = "~0.3.1"
 petgraph = "~0.5.1"
 rand = "~0.7.3"
 boolector = { version = "0.4", git = "https://github.com/finga/boolector-rs", branch = "boolector-src", features = ["vendored"] }

src/boolector.rs

@@ -45,7 +45,15 @@ impl Solver for Boolector {
             let assignments = graph
                 .node_indices()
                 .filter(|i| matches!(graph[*i], Input(_)))
-                .map(|i| BitVector(bvs.get(&i).unwrap().get_a_solution().as_u64().unwrap()))
+                .map(|i| {
+                    let bv = bvs.get(&i).expect("every input must be part of bvs");
+
+                    BitVector(
+                        bv.get_a_solution()
+                            .as_u64()
+                            .expect("BV always fits in 64 bits for our machine"),
+                    )
+                })
                 .collect();
 
             Some(assignments)

src/cfg.rs

@@ -13,6 +13,7 @@
 //!   - `jal`: when link is used (=> `rd` is `ra`)
 //!   - `jalr`
 
+use anyhow::{ensure, Context, Error, Result};
 use byteorder::{ByteOrder, LittleEndian};
 use bytesize::ByteSize;
 use log::info;
@@ -23,7 +24,7 @@ use petgraph::{
     Graph,
 };
 use riscu::{decode, load_object_file, Instruction, Program, Register};
-use std::{fs::File, io::prelude::*, path::Path, process::Command, vec::Vec};
+use std::{fs::File, io::prelude::*, mem::size_of, path::Path, vec::Vec};
 
 type Edge = (NodeIndex, NodeIndex, Option<ProcedureCallId>);
 
@@ -52,16 +53,25 @@ fn allocate_procedure_call_id() -> u64 {
 }
 
 /// Create a `ControlFlowGraph` from an `u8` slice without fixing edges
-fn create_instruction_graph(binary: &[u8]) -> ControlFlowGraph {
+fn create_instruction_graph(binary: &[u8]) -> Result<ControlFlowGraph> {
+    ensure!(
+        binary.len() % size_of::<u32>() == 0,
+        "RISC-U instructions are 32 bits, so the length of the binary must be a multiple of 4"
+    );
+
+    let mut g = ControlFlowGraph::new();
+
     binary
-        .chunks_exact(4)
+        .chunks_exact(size_of::<u32>())
         .map(LittleEndian::read_u32)
-        .map(decode)
-        .map(Result::unwrap)
-        .fold(ControlFlowGraph::new(), |mut g, i| {
-            g.add_node(i);
-            g
-        })
+        .try_for_each(|raw| {
+            decode(raw).and_then(|i| {
+                g.add_node(i);
+                Ok(())
+            })
+        })?;
+
+    Ok(g)
 }
 
 /// Compute trivial edges
@@ -115,10 +125,17 @@ fn compute_return_edge_position(graph: &ControlFlowGraph, idx: NodeIndex) -> Nod
             graph
                 .edges(idx)
                 .find(|e| e.target().index() != idx.index() + 1)
-                .unwrap()
+                .expect("all BEQ edges are constructed already")
                 .target()
         }),
-        _ => compute_return_edge_position(graph, graph.edges(idx).next().unwrap().target()),
+        _ => compute_return_edge_position(
+            graph,
+            graph
+                .edges(idx)
+                .next()
+                .expect("all trivial edges are constructed already")
+                .target(),
+        ),
     }
 }
 
@@ -174,7 +191,7 @@ fn find_possible_exit_edge(graph: &ControlFlowGraph, idx: NodeIndex) -> Option<E
 }
 
 /// Fix the exit ecall edge
-fn fix_exit_ecall(graph: &mut ControlFlowGraph) -> NodeIndex {
+fn fix_exit_ecall(graph: &mut ControlFlowGraph) -> Result<NodeIndex> {
     graph
         .node_indices()
         .find(|idx| {
@@ -186,12 +203,12 @@ fn fix_exit_ecall(graph: &mut ControlFlowGraph) -> NodeIndex {
             }
             false
         })
-        .unwrap()
+        .ok_or(Error::msg("Could not find exit ecall in binary"))
 }
 
 /// Create a ControlFlowGraph from `u8` slice.
-fn build(binary: &[u8]) -> (ControlFlowGraph, NodeIndex) {
-    let mut graph = create_instruction_graph(binary);
+fn build(binary: &[u8]) -> Result<(ControlFlowGraph, NodeIndex)> {
+    let mut graph = create_instruction_graph(binary)?;
 
     fn add_edges(graph: &mut ControlFlowGraph, edges: &[Edge]) {
         edges.iter().for_each(|e| {
@@ -208,34 +225,32 @@ fn build(binary: &[u8]) -> (ControlFlowGraph, NodeIndex) {
     let jump_edges = compute_stateful_edges(&graph);
     add_edges(&mut graph, &jump_edges);
 
-    let exit_node = fix_exit_ecall(&mut graph);
+    let exit_node = fix_exit_ecall(&mut graph)?;
 
-    (graph, exit_node)
+    Ok((graph, exit_node))
 }
 
 pub type DataSegment = Vec<u8>;
 
 /// Create a ControlFlowGraph from Path `file`.
-// TODO: only tested with Selfie RISC-U file and relies on that ELF format
-pub fn build_cfg_from_file<P>(
-    file: P,
-) -> Result<((ControlFlowGraph, NodeIndex), Program), &'static str>
+pub fn build_cfg_from_file<P>(file: P) -> Result<((ControlFlowGraph, NodeIndex), Program)>
 where
     P: AsRef<Path>,
 {
     reset_procedure_call_id_seed();
 
-    let program = load_object_file(file).map_err(|_| "Cannot load RISC-U ELF file")?;
+    let program = load_object_file(file).context("Failed to load object file")?;
 
     let cfg = time_info!("generate CFG from binary", {
         build(program.code.content.as_slice())
+            .context("Could not build a control flow graph from loaded file")?
     });
 
     Ok((cfg, program))
 }
 
 /// Write ControlFlowGraph `graph` to dot file at `file` Path.
-pub fn write_to_file<P>(graph: &ControlFlowGraph, file_path: P) -> Result<(), std::io::Error>
+pub fn write_to_file<P>(graph: &ControlFlowGraph, file_path: P) -> Result<()>
 where
     P: AsRef<Path>,
 {
@@ -248,22 +263,9 @@ where
 
     info!(
         "{} written to file {}",
-        ByteSize(file.metadata().unwrap().len()),
-        file_path.as_ref().to_str().unwrap(),
+        ByteSize(file.metadata()?.len()),
+        file_path.as_ref().display(),
     );
 
     Ok(())
 }
-
-/// Convert a dot file into a png file (depends on graphviz)
-pub fn convert_dot_to_png(source: &Path, output: &Path) -> Result<(), &'static str> {
-    Command::new("dot")
-        .arg("-Tpng")
-        .arg(source.to_path_buf())
-        .arg("-o")
-        .arg(output.to_path_buf())
-        .output()
-        .map_err(|_| "Cannot convert CFG to png file (is graphviz installed?)")?;
-
-    Ok(())
-}

src/cli.rs

@@ -1,8 +1,14 @@
+use clap::ArgMatches;
 use clap::{crate_authors, crate_description, crate_name, crate_version, App, AppSettings, Arg};
 
 pub const LOGGING_LEVELS: [&str; 5] = ["trace", "debug", "info", "warn", "error"];
 pub const SOLVER: [&str; 3] = ["monster", "boolector", "z3"];
 
+pub fn expect_arg<'a>(m: &'a ArgMatches, arg: &str) -> &'a str {
+    m.value_of(arg)
+        .expect(format!("argument \"{}\" has to be set in CLI at all times", arg).as_str())
+}
+
 pub fn args() -> App<'static> {
     App::new(crate_name!())
         .version(crate_version!())

src/engine.rs

@@ -7,10 +7,11 @@ use crate::{
     symbolic_state::{Query, SymbolId, SymbolicState},
     z3::Z3,
 };
+use anyhow::Result;
 use byteorder::{ByteOrder, LittleEndian};
 use bytesize::ByteSize;
 use log::{debug, info, trace};
-use riscu::{decode, types::*, DecodingError, Instruction, Program, ProgramSegment, Register};
+use riscu::{decode, types::*, Instruction, Program, ProgramSegment, Register};
 use std::{cell::RefCell, collections::HashMap, fmt, mem::size_of, path::Path, rc::Rc};
 
 #[allow(dead_code)]
@@ -29,7 +30,7 @@ pub enum Backend {
 }
 
 // TODO: What should the engine return as result?
-pub fn execute<P>(input: P, with: Backend) -> Result<(), String>
+pub fn execute<P>(input: P, with: Backend) -> Result<()>
 where
     P: AsRef<Path>,
 {
@@ -44,27 +45,25 @@ where
 
             let mut executor = Engine::new(ByteSize::mib(1), &program, &mut strategy, state);
 
-            executor.run();
+            executor.run()
         }
         Backend::Boolector => {
             let solver = Rc::new(RefCell::new(Boolector::new()));
             let state = Box::new(SymbolicState::new(solver));
 
             let mut executor = Engine::new(ByteSize::mib(1), &program, &mut strategy, state);
 
-            executor.run();
+            executor.run()
         }
         Backend::Z3 => {
             let solver = Rc::new(RefCell::new(Z3::new()));
             let state = Box::new(SymbolicState::new(solver));
 
             let mut executor = Engine::new(ByteSize::mib(1), &program, &mut strategy, state);
 
-            executor.run();
+            executor.run()
         }
     }
-
-    Ok(())
 }
 
 #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
@@ -116,7 +115,7 @@ where
         let sp = memory_size.as_u64() - 8;
         regs[Register::Sp as usize] = Value::Concrete(sp);
 
-        // TOOD: Init main function arguments
+        // TODO: Init main function arguments
         let argc = 0;
         memory[sp as usize / size_of::<u64>()] = Value::Concrete(argc);
 
@@ -169,17 +168,16 @@ where
             .for_each(|(x, i)| memory[i] = Value::Concrete(x));
     }
 
-    pub fn run(&mut self) {
+    pub fn run(&mut self) -> Result<()> {
         while !self.is_exited {
             let word = self.fetch();
 
-            // TODO: handle case where decoding fails
-            let instr = self
-                .decode(word)
-                .expect("instructions have to be always decodable");
+            let instr = decode(word)?;
 
-            self.execute(instr);
+            self.execute(instr)?;
         }
+
+        Ok(())
     }
 
     fn handle_solver_result(&self, reason: &str, solver_result: Option<Assignment<BitVector>>) {
@@ -471,7 +469,7 @@ where
         false_branch: u64,
         lhs: SymbolId,
         rhs: SymbolId,
-    ) {
+    ) -> Result<()> {
         let memory_snapshot = self.memory.clone();
         let regs_snapshot = self.regs;
         let graph_snapshot = Box::new((*self.symbolic_state).clone());
@@ -494,7 +492,7 @@ where
         );
 
         self.pc = next_pc;
-        self.run();
+        self.run()?;
 
         self.is_exited = false;
 
@@ -518,9 +516,11 @@ where
         );
 
         self.pc = next_pc;
+
+        Ok(())
     }
 
-    fn execute_beq(&mut self, btype: BType) {
+    fn execute_beq(&mut self, btype: BType) -> Result<()> {
         let lhs = self.regs[btype.rs1() as usize];
         let rhs = self.regs[btype.rs2() as usize];
 
@@ -540,6 +540,8 @@ where
                     rhs,
                     self.pc
                 );
+
+                Ok(())
             }
             (Value::Symbolic(v1), Value::Concrete(v2)) => {
                 let v2 = self.symbolic_state.create_const(v2);
@@ -559,6 +561,8 @@ where
                 trace!("could not find input assignment => exeting this context");
 
                 self.is_exited = true;
+
+                Ok(())
             }
         }
     }
@@ -737,11 +741,7 @@ where
         }
     }
 
-    fn decode(&self, raw_instr: u32) -> Result<Instruction, DecodingError> {
-        decode(raw_instr)
-    }
-
-    fn execute(&mut self, instruction: Instruction) {
+    fn execute(&mut self, instruction: Instruction) -> Result<()> {
         match instruction {
             Instruction::Ecall(_) => self.execute_ecall(),
             Instruction::Lui(utype) => self.execute_lui(utype),
@@ -758,8 +758,10 @@ where
             Instruction::Sd(stype) => self.execute_sd(instruction, stype),
             Instruction::Jal(jtype) => self.execute_jal(jtype),
             Instruction::Jalr(itype) => self.execute_jalr(itype),
-            Instruction::Beq(btype) => self.execute_beq(btype),
+            Instruction::Beq(btype) => self.execute_beq(btype)?,
         }
+
+        Ok(())
     }
 }