Make the verifier output even prettier (#599)

* Fix verifier printing to print instruction encodings consistently.

Use `FuncWriter::write_instruction` for all instructions so that
encodings are printed consistently.

* Make use-before-def errors mention the relevant value.

* When there are verifier errors, print a message at the end.

* Make verifier errors prettier.

Fix the length of the "^~~~~" to match the printed entity, and print the
error messsage on its own line.

* Clean up "test verifier" failure messages.

* Tidy the uses-value-from-itself error.

The use instruction is the same as the def instruction, so don't print
both. Also, the use instruction is already being printed at the
beginning, so don't print it again at the end.

cranelift/filetests/verifier/defs_dominates_uses.clif

@@ -4,13 +4,13 @@ test verifier
 
 function %non_dominating(i32) -> i32 system_v {
 ebb0(v0: i32):
-    v1 = iadd.i32 v2, v0   ; error: uses value from non-dominating
+    v1 = iadd.i32 v2, v0   ; error: uses value v2 from non-dominating
     v2 = iadd.i32 v1, v0
     return v2
 }
 
 function %inst_uses_its_own_values(i32) -> i32 system_v {
 ebb0(v0: i32):
-    v1 = iadd.i32 v1, v0   ; error: uses value from itself
+    v1 = iadd.i32 v1, v0   ; error: uses value v1 from itself
     return v1
 }

lib/codegen/src/print_errors.rs

@@ -23,6 +23,7 @@ pub fn pretty_verifier_error<'a>(
 ) -> String {
     let mut errors = errors.0;
     let mut w = String::new();
+    let num_errors = errors.len();
 
     decorate_function(
         &mut PrettyVerifierError(func_w.unwrap_or_else(|| Box::new(PlainWriter)), &mut errors),
@@ -30,6 +31,14 @@ pub fn pretty_verifier_error<'a>(
         func,
         isa,
     ).unwrap();
+
+    writeln!(
+        w,
+        "\n; {} verifier error{} detected (see above). Compilation aborted.",
+        num_errors,
+        if num_errors == 1 { "" } else { "s" }
+    ).unwrap();
+
     w
 }
 
@@ -80,25 +89,31 @@ fn pretty_ebb_header_error(
     func_w: &mut FuncWriter,
     errors: &mut Vec<VerifierError>,
 ) -> fmt::Result {
-    let mut i = 0;
+    let mut s = String::new();
-    let mut printed_ebb = false;
+    func_w.write_ebb_header(&mut s, func, isa, cur_ebb, indent)?;
+    write!(w, "{}", s)?;
 
-    while i < errors.len() {
+    // TODO: Use drain_filter here when it gets stabilized
+    let mut i = 0;
+    let mut printed_error = false;
+    while i != errors.len() {
         match errors[i].location {
             ir::entities::AnyEntity::Ebb(ebb) if ebb == cur_ebb => {
-                if !printed_ebb {
+                if !printed_error {
-                    func_w.write_ebb_header(w, func, isa, cur_ebb, indent)?;
+                    print_arrow(w, &s)?;
-                    printed_ebb = true;
+                    printed_error = true;
                 }
                 let err = errors.remove(i);
-                print_error(w, indent, cur_ebb.to_string(), err)?;
+                print_error(w, err)?;
-            }
-            _ => {
-                i += 1;
             }
+            _ => i += 1,
         }
     }
 
+    if printed_error {
+        w.write_char('\n')?;
+    }
+
     Ok(())
 }
 
@@ -113,35 +128,29 @@ fn pretty_instruction_error(
     func_w: &mut FuncWriter,
     errors: &mut Vec<VerifierError>,
 ) -> fmt::Result {
+    let mut s = String::new();
+    func_w.write_instruction(&mut s, func, aliases, isa, cur_inst, indent)?;
+    write!(w, "{}", s)?;
+
     // TODO: Use drain_filter here when it gets stabilized
     let mut i = 0;
-    let mut printed_instr = false;
+    let mut printed_error = false;
-
     while i != errors.len() {
         match errors[i].location {
             ir::entities::AnyEntity::Inst(inst) if inst == cur_inst => {
-                if !printed_instr {
+                if !printed_error {
-                    func_w.write_instruction(w, func, aliases, isa, cur_inst, indent)?;
+                    print_arrow(w, &s)?;
-                    printed_instr = true;
+                    printed_error = true;
                 }
                 let err = errors.remove(i);
-                print_error(w, indent, cur_inst.to_string(), err)?;
+                print_error(w, err)?;
             }
-            ir::entities::AnyEntity::Inst(_) => i += 1,
+            _ => i += 1,
-            _ => unreachable!(),
         }
     }
 
-    if printed_instr {
+    if printed_error {
         w.write_char('\n')?;
-    } else {
-        writeln!(
-            w,
-            "{1:0$}{2}",
-            indent,
-            "",
-            func.dfg.display_inst(cur_inst, isa)
-        )?;
     }
 
     Ok(())
@@ -155,45 +164,54 @@ fn pretty_preamble_error(
     func_w: &mut FuncWriter,
     errors: &mut Vec<VerifierError>,
 ) -> fmt::Result {
+    let mut s = String::new();
+    func_w.write_entity_definition(&mut s, func, entity, value)?;
+    write!(w, "{}", s)?;
+
     // TODO: Use drain_filter here when it gets stabilized
-    let indent = 4;
-
     let mut i = 0;
-    let mut printed_entity = false;
+    let mut printed_error = false;
-
     while i != errors.len() {
         if entity == errors[i].location {
-            let err = errors.remove(i);
+            if !printed_error {
-
+                print_arrow(w, &s)?;
-            if !printed_entity {
+                printed_error = true;
-                func_w.write_entity_definition(w, func, entity, value)?;
-                printed_entity = true;
             }
-
+            let err = errors.remove(i);
-            print_error(w, indent, entity.to_string(), err)?;
+            print_error(w, err)?;
         } else {
             i += 1
         }
     }
 
-    if printed_entity {
+    if printed_error {
         w.write_char('\n')?;
-    } else {
-        func_w.write_entity_definition(w, func, entity, value)?;
     }
 
     Ok(())
 }
 
-/// Prints ;   ^~~~~~ verifier [ERROR BODY]
+/// Prints:
-fn print_error(w: &mut Write, indent: usize, s: String, err: VerifierError) -> fmt::Result {
+///    ;   ^~~~~~
-    let indent = if indent < 1 { 0 } else { indent - 1 };
+fn print_arrow(w: &mut Write, entity: &str) -> fmt::Result {
+    write!(w, ";")?;
 
-    write!(w, ";{1:0$}^", indent, "")?;
+    let indent = entity.len() - entity.trim_start().len();
-    for _c in s.chars() {
+    if indent != 0 {
+        write!(w, "{1:0$}^", indent - 1, "")?;
+    }
+
+    for _ in 0..entity.trim().len() - 1 {
         write!(w, "~")?;
     }
-    writeln!(w, " verifier {}", err.to_string())?;
+
+    writeln!(w)
+}
+
+/// Prints:
+///    ; error: [ERROR BODY]
+fn print_error(w: &mut Write, err: VerifierError) -> fmt::Result {
+    writeln!(w, "; error: {}", err.to_string())?;
     Ok(())
 }
 

lib/codegen/src/verifier/mod.rs

@@ -861,18 +861,13 @@ impl<'a> Verifier<'a> {
                         return fatal!(
                             errors,
                             loc_inst,
-                            "uses value from non-dominating {}",
+                            "uses value {} from non-dominating {}",
+                            v,
                             def_inst
                         );
                     }
                     if def_inst == loc_inst {
-                        return fatal!(
+                        return fatal!(errors, loc_inst, "uses value {} from itself", v);
-                            errors,
-                            loc_inst,
-                            "uses value from itself {},  {}",
-                            def_inst,
-                            loc_inst
-                        );
                     }
                 }
             }

lib/filetests/src/test_verifier.rs

@@ -62,7 +62,7 @@ impl SubTest for TestVerifier {
                 let mut errors = errors.0;
                 let mut msg = String::new();
 
-                // for each expected error, find a suitable match
+                // For each expected error, find a suitable match.
                 for expect in expected {
                     let pos = errors
                         .iter()
@@ -70,7 +70,7 @@ impl SubTest for TestVerifier {
 
                     match pos {
                         None => {
-                            write!(msg, "expected error {}", expect.0).unwrap();
+                            writeln!(msg, "  expected error {}: {}", expect.0, expect.1).unwrap();
                         }
                         Some(pos) => {
                             errors.swap_remove(pos);
@@ -78,9 +78,9 @@ impl SubTest for TestVerifier {
                     }
                 }
 
-                // report remaining errors
+                // Report remaining errors.
                 for err in errors {
-                    write!(msg, "unexpected error {}", err).unwrap();
+                    writeln!(msg, "unexpected error {}", err).unwrap();
                 }
 
                 if msg.is_empty() {