Replace FunctionBuilder's Drop impl with a finalize function. (#193)

* Replace FunctionBuilder's Drop impl with a finalize function.

This has the advantage of not triggering assertion failures in the event
of abandoning a partially-built function. It has the disadvantage of
requiring users to call finalize() explicitly.

lib/frontend/src/frontend.rs

@@ -8,6 +8,7 @@ use cretonne::ir::function::DisplayFunction;
 use cretonne::isa::TargetIsa;
 use ssa::{SSABuilder, SideEffects, Block};
 use cretonne::entity::{EntityRef, EntityMap, EntitySet};
+use cretonne::packed_option::PackedOption;
 
 /// Structure used for translating a series of functions into Cretonne IL.
 ///
@@ -48,8 +49,28 @@ struct EbbData {
 }
 
 struct Position {
-    ebb: Ebb,
+    ebb: PackedOption<Ebb>,
-    basic_block: Block,
+    basic_block: PackedOption<Block>,
+}
+
+impl Position {
+    fn at(ebb: Ebb, basic_block: Block) -> Self {
+        Self {
+            ebb: PackedOption::from(ebb),
+            basic_block: PackedOption::from(basic_block),
+        }
+    }
+
+    fn default() -> Self {
+        Self {
+            ebb: PackedOption::default(),
+            basic_block: PackedOption::default(),
+        }
+    }
+
+    fn is_default(&self) -> bool {
+        self.ebb.is_none() && self.basic_block.is_none()
+    }
 }
 
 impl<Variable> ILBuilder<Variable>
@@ -149,7 +170,7 @@ impl<'short, 'long, Variable> InstBuilderBase<'short> for FuncInstBuilder<'short
                                     .filter(|dest_ebb| unique.insert(*dest_ebb)) {
                             self.builder.builder.ssa.declare_ebb_predecessor(
                                 dest_ebb,
-                                self.builder.position.basic_block,
+                                self.builder.position.basic_block.unwrap(),
                                 inst,
                             )
                         }
@@ -213,10 +234,7 @@ where
             func: func,
             srcloc: Default::default(),
             builder: builder,
-            position: Position {
+            position: Position::default(),
-                ebb: Ebb::new(0),
-                basic_block: Block::new(0),
-            },
         }
     }
 
@@ -245,13 +263,12 @@ where
     /// successor), the block will be declared filled and it will not be possible to append
     /// instructions to it.
     pub fn switch_to_block(&mut self, ebb: Ebb) {
-        if !self.builder.ebbs[self.position.ebb].pristine {
         // First we check that the previous block has been filled.
         debug_assert!(
-                self.is_unreachable() || self.builder.ebbs[self.position.ebb].filled,
+            self.position.is_default() || self.is_unreachable() || self.is_pristine() ||
+                self.is_filled(),
             "you have to fill your block before switching"
         );
-        }
         // We cannot switch to a filled block
         debug_assert!(
             !self.builder.ebbs[ebb].filled,
@@ -260,7 +277,7 @@ where
 
         let basic_block = self.builder.ssa.header_block(ebb);
         // Then we change the cursor position.
-        self.position = Position { ebb, basic_block };
+        self.position = Position::at(ebb, basic_block);
     }
 
     /// Declares that all the predecessors of this block are known.
@@ -299,7 +316,7 @@ where
             self.func,
             var,
             ty,
-            self.position.basic_block,
+            self.position.basic_block.unwrap(),
         );
         self.handle_ssa_side_effects(side_effects);
         val
@@ -311,7 +328,7 @@ where
         self.builder.ssa.def_var(
             var,
             val,
-            self.position.basic_block,
+            self.position.basic_block.unwrap(),
         );
     }
 
@@ -354,13 +371,13 @@ where
     /// Returns an object with the [`InstBuilder`](../cretonne/ir/builder/trait.InstBuilder.html)
     /// trait that allows to conveniently append an instruction to the current `Ebb` being built.
     pub fn ins<'short>(&'short mut self) -> FuncInstBuilder<'short, 'a, Variable> {
-        let ebb = self.position.ebb;
+        let ebb = self.position.ebb.unwrap();
         FuncInstBuilder::new(self, ebb)
     }
 
     /// Make sure that the current EBB is inserted in the layout.
     pub fn ensure_inserted_ebb(&mut self) {
-        let ebb = self.position.ebb;
+        let ebb = self.position.ebb.unwrap();
         if self.builder.ebbs[ebb].pristine {
             if !self.func.layout.is_ebb_inserted(ebb) {
                 self.func.layout.append_ebb(ebb);
@@ -382,7 +399,7 @@ where
         self.ensure_inserted_ebb();
         FuncCursor::new(self.func)
             .with_srcloc(self.srcloc)
-            .at_bottom(self.position.ebb)
+            .at_bottom(self.position.ebb.unwrap())
     }
 
     /// Append parameters to the given `Ebb` corresponding to the function
@@ -410,6 +427,33 @@ where
             self.func.dfg.append_ebb_param(ebb, argtyp.value_type);
         }
     }
+
+    /// Declare that translation of the current function is complete. This
+    /// resets the state of the `FunctionBuilder` in preparation to be used
+    /// for another function.
+    pub fn finalize(&mut self) {
+        // Check that all the `Ebb`s are filled and sealed.
+        debug_assert!(
+            self.builder.ebbs.keys().all(|ebb| {
+                self.builder.ebbs[ebb].pristine || self.builder.ssa.is_sealed(ebb)
+            }),
+            "all blocks should be sealed before dropping a FunctionBuilder"
+        );
+        debug_assert!(
+            self.builder.ebbs.keys().all(|ebb| {
+                self.builder.ebbs[ebb].pristine || self.builder.ebbs[ebb].filled
+            }),
+            "all blocks should be filled before dropping a FunctionBuilder"
+        );
+
+        // Clear the state (but preserve the allocated buffers) in preparation
+        // for translation another function.
+        self.builder.clear();
+
+        // Reset srcloc and position to initial states.
+        self.srcloc = Default::default();
+        self.position = Position::default();
+    }
 }
 
 /// All the functions documented in the previous block are write-only and help you build a valid
@@ -475,22 +519,25 @@ where
     pub fn is_unreachable(&self) -> bool {
         let is_entry = match self.func.layout.entry_block() {
             None => false,
-            Some(entry) => self.position.ebb == entry,
+            Some(entry) => self.position.ebb.unwrap() == entry,
         };
-        !is_entry && self.builder.ssa.is_sealed(self.position.ebb) &&
+        !is_entry && self.builder.ssa.is_sealed(self.position.ebb.unwrap()) &&
-            self.builder.ssa.predecessors(self.position.ebb).is_empty()
+            self.builder
+                .ssa
+                .predecessors(self.position.ebb.unwrap())
+                .is_empty()
     }
 
     /// Returns `true` if and only if no instructions have been added since the last call to
     /// `switch_to_block`.
     pub fn is_pristine(&self) -> bool {
-        self.builder.ebbs[self.position.ebb].pristine
+        self.builder.ebbs[self.position.ebb.unwrap()].pristine
     }
 
     /// Returns `true` if and only if a terminator instruction has been inserted since the
     /// last call to `switch_to_block`.
     pub fn is_filled(&self) -> bool {
-        self.builder.ebbs[self.position.ebb].filled
+        self.builder.ebbs[self.position.ebb.unwrap()].filled
     }
 
     /// Returns a displayable object for the function as it is.
@@ -501,51 +548,25 @@ where
     }
 }
 
-impl<'a, Variable> Drop for FunctionBuilder<'a, Variable>
-where
-    Variable: EntityRef,
-{
-    /// When a `FunctionBuilder` goes out of scope, it means that the function is fully built.
-    fn drop(&mut self) {
-        // Check that all the `Ebb`s are filled and sealed.
-        debug_assert!(
-            self.builder.ebbs.keys().all(|ebb| {
-                self.builder.ebbs[ebb].pristine || self.builder.ssa.is_sealed(ebb)
-            }),
-            "all blocks should be sealed before dropping a FunctionBuilder"
-        );
-        debug_assert!(
-            self.builder.ebbs.keys().all(|ebb| {
-                self.builder.ebbs[ebb].pristine || self.builder.ebbs[ebb].filled
-            }),
-            "all blocks should be filled before dropping a FunctionBuilder"
-        );
-
-        // Clear the state (but preserve the allocated buffers) in preparation
-        // for translation another function.
-        self.builder.clear();
-    }
-}
-
 // Helper functions
 impl<'a, Variable> FunctionBuilder<'a, Variable>
 where
     Variable: EntityRef,
 {
     fn move_to_next_basic_block(&mut self) {
-        self.position.basic_block = self.builder.ssa.declare_ebb_body_block(
+        self.position.basic_block = PackedOption::from(self.builder.ssa.declare_ebb_body_block(
-            self.position.basic_block,
+            self.position.basic_block.unwrap(),
-        );
+        ));
     }
 
     fn fill_current_block(&mut self) {
-        self.builder.ebbs[self.position.ebb].filled = true;
+        self.builder.ebbs[self.position.ebb.unwrap()].filled = true;
     }
 
     fn declare_successor(&mut self, dest_ebb: Ebb, jump_inst: Inst) {
         self.builder.ssa.declare_ebb_predecessor(
             dest_ebb,
-            self.position.basic_block,
+            self.position.basic_block.unwrap(),
             jump_inst,
         );
     }
@@ -668,6 +689,8 @@ mod tests {
             if lazy_seal {
                 builder.seal_all_blocks();
             }
+
+            builder.finalize();
         }
 
         let flags = settings::Flags::new(&settings::builder());

lib/frontend/src/lib.rs

@@ -128,6 +128,8 @@
 //!         }
 //!         builder.ins().jump(block1, &[]);
 //!         builder.seal_block(block1);
+//!
+//!         builder.finalize();
 //!     }
 //!
 //!     let flags = settings::Flags::new(&settings::builder());

lib/wasm/src/func_translator.rs

@@ -94,7 +94,10 @@ impl FuncTranslator {
         self.state.initialize(&builder.func.signature, exit_block);
 
         parse_local_decls(&mut reader, &mut builder, num_params)?;
-        parse_function_body(reader, &mut builder, &mut self.state, environ)
+        parse_function_body(reader, &mut builder, &mut self.state, environ)?;
+
+        builder.finalize();
+        Ok(())
     }
 }